New issue
Advanced search Search tips

Issue 794205 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocked on:
issue 795026



Sign in to add a comment

Remove Extension::State

Project Member Reported by rdevlin....@chromium.org, Dec 12 2017

Issue description

Extension::State is an enum with the following possible values:

  enum State {
    DISABLED = 0,
    ENABLED,
    // An external extension that the user uninstalled. We should not reinstall
    // such extensions on startup.
    EXTERNAL_EXTENSION_UNINSTALLED,
    // DEPRECATED: Special state for component extensions.
    // Maintained as a placeholder since states may be stored to disk.
    ENABLED_COMPONENT_DEPRECATED,
    // Add new states here as this enum is stored in prefs.
    NUM_STATES
  };

However, all of these states should be determinable by other properties.  For example, an extension should be considered disabled if and only if it has disable reasons (otherwise, there'd be no way to re-enable it or check if it needs to remain disabled).

We should get rid of this enum and the places that use it, because it currently only serves to raise the chances of preferences getting out of sync with each other (e.g. extensions disabled with no disable reasons).
 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7b19341cc0a18d5f4caf4472f0024ba7f1cc641b

commit 7b19341cc0a18d5f4caf4472f0024ba7f1cc641b
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Thu Dec 14 03:46:03 2017

[Extensions Cleanup] Remove state parameter from test UninstallExtension

Remove the Extension::State parameter in
ExtensionServiceTestWithInstall::UninstallExtension. The burden of
verifying the extension's state prior to uninstallation should be part
of the calling tests, rather than part of the UninstallExtension method,
in order to reduce complexity and improve predictability.

Bug: 794205

Change-Id: Ia00758b58e20786d43799ea33a2dd72cfa42c565
Reviewed-on: https://chromium-review.googlesource.com/823011
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523999}
[modify] https://crrev.com/7b19341cc0a18d5f4caf4472f0024ba7f1cc641b/chrome/browser/extensions/extension_service_sync_unittest.cc
[modify] https://crrev.com/7b19341cc0a18d5f4caf4472f0024ba7f1cc641b/chrome/browser/extensions/extension_service_test_with_install.cc
[modify] https://crrev.com/7b19341cc0a18d5f4caf4472f0024ba7f1cc641b/chrome/browser/extensions/extension_service_test_with_install.h

Blockedon: 795026
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0c23f0f8ad24c83df7afb58a2595ed3cc48e7666

commit 0c23f0f8ad24c83df7afb58a2595ed3cc48e7666
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Fri Dec 15 00:10:39 2017

[Extensions] Comment out an old Extension::State value

Comment out Extension::ENABLED_COMPONENT_DEPRECATED to ensure it will
not be used. Also add explicit numbers for the rest of the states to
ensure they are not changed (since they are persisted on disk).

Bug: 794205

Change-Id: I1abaae6190a4e828dfe4768481c611e3ad68ebcd
Reviewed-on: https://chromium-review.googlesource.com/826686
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524245}
[modify] https://crrev.com/0c23f0f8ad24c83df7afb58a2595ed3cc48e7666/extensions/common/extension.h

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/55fab54d24277549a682a1e220faec6c027626eb

commit 55fab54d24277549a682a1e220faec6c027626eb
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Sat Dec 16 02:27:19 2017

[Extensions] Update tests to not explicitly check external uninstall state

Currently, ExtensionService unittests explicitly look at the value of
the "state" pref in an extension's preference entry to check if it was
an externally-uninstalled extension. This is bad because it is fragile
and relies entirely on implementation details of ExtensionPrefs.

Instead, use ExtensionPrefs::IsExternalExtensionUninstalled() to verify
uninstalled external extensions and
ExtensionPrefs::GetInstalledExtensionInfo to look for install state.

Bug: 794205
Change-Id: I14ea2ce0390c37ccdcdbeea4e23b48b968baaebb
Reviewed-on: https://chromium-review.googlesource.com/825943
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524567}
[modify] https://crrev.com/55fab54d24277549a682a1e220faec6c027626eb/chrome/browser/extensions/extension_service_test_with_install.cc
[modify] https://crrev.com/55fab54d24277549a682a1e220faec6c027626eb/chrome/browser/extensions/extension_service_unittest.cc
[modify] https://crrev.com/55fab54d24277549a682a1e220faec6c027626eb/extensions/browser/extension_prefs.cc
[modify] https://crrev.com/55fab54d24277549a682a1e220faec6c027626eb/extensions/browser/extension_prefs.h

Cc: -catmulli...@chromium.org

Sign in to add a comment