New issue
Advanced search Search tips

Issue 770879 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Extract ExtensionService extension lifecycle code to //extensions

Project Member Reported by michae...@chromium.org, Oct 2 2017

Issue description

ExtensionService is a monolithic class with different functions. Good code to target for migration to //extensions shouldn't need to depend on Chrome and could be useful to AppShell or extensions tests.

The code for enabling, disabling, reloading, blocking, blacklisting, removing and terminating extensions largely uses //extensions constructs, like the ExtensionRegistry. These basic extension lifecycle methods should be in a common location.

I am moving these into a new class in //extensions called ExtensionRegistrar. ExtensionsRegistrar uses ExtensionRegistry to track the state of extensions, and will soon be the sole modifier of the Registry in order to ensure it has a consistent state.

Design doc: go/phbin

The initial CL landed as https://chromium-review.googlesource.com/671206.
 
Cc: karandeepb@chromium.org catmulli...@chromium.org lazyboy@chromium.org
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 5 2017

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

commit 5b35a0887e58e206da70945390843f8ed92cfdf6
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Thu Oct 05 20:44:36 2017

[Extensions] Make ExtensionService stack-allocate ExtensionRegistrar

ExtensionService::extension_registrar_ is constructed at
ExtensionService construction time, and is never reset. There's no
reason to have it as a std::unique_ptr<> instead of a stack-allocated
member variable. Make it the latter.

Bug: 770879
Change-Id: I68001b9214b6176fc4703e3c6d2f1cbdcece9dff
Reviewed-on: https://chromium-review.googlesource.com/701398
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506845}
[modify] https://crrev.com/5b35a0887e58e206da70945390843f8ed92cfdf6/chrome/browser/extensions/extension_service.cc
[modify] https://crrev.com/5b35a0887e58e206da70945390843f8ed92cfdf6/chrome/browser/extensions/extension_service.h

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 9 2017

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

commit 6397c59510e8624f906f949d593bc95a4aa9c84c
Author: Michael Giuffrida <michaelpg@chromium.org>
Date: Mon Oct 09 20:46:53 2017

Move more ExtensionService code to ExtensionRegistrar

Adds several functions to ExtensionRegistrar:
* AddExtension
* ReplaceReloadedExtension
* IsExtensionEnabled

EnableExtension and DisableExtension have been augmented to do most of
the work ExtensionService used to do here, and delegate the remaining
work to the new ExtensionRegistrar::Delegate. The delegate can query
policy, update the profile, and other things still part of //chrome.

Design doc: https://goo.gl/trZKep (Google-internal).
First CL:
https://chromium-review.googlesource.com/c/chromium/src/+/671206

Bug: 770879
Change-Id: I7e5f43b458fd722137c0593b826a4b98d115387a
Reviewed-on: https://chromium-review.googlesource.com/677888
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507474}
[modify] https://crrev.com/6397c59510e8624f906f949d593bc95a4aa9c84c/chrome/browser/extensions/extension_service.cc
[modify] https://crrev.com/6397c59510e8624f906f949d593bc95a4aa9c84c/chrome/browser/extensions/extension_service.h
[modify] https://crrev.com/6397c59510e8624f906f949d593bc95a4aa9c84c/extensions/browser/extension_registrar.cc
[modify] https://crrev.com/6397c59510e8624f906f949d593bc95a4aa9c84c/extensions/browser/extension_registrar.h
[modify] https://crrev.com/6397c59510e8624f906f949d593bc95a4aa9c84c/extensions/browser/extension_registrar_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 11 2017

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

commit fcb42036b8a3a9c62fdec2d18f7a638c989f9c55
Author: Michael Giuffrida <michaelpg@chromium.org>
Date: Wed Oct 11 21:50:26 2017

Handle terminated extensions in ExtensionRegistrar

This moves the logic for handling terminated extensions from
ExtensionService to ExtensionRegistrar and simplifies it a bit.

This review began in crrev.com/c/696432 but I was unable to
successfully re-open that CL after temporarily abandoning it.

Bug: 770879
Change-Id: I28e2b0547061f455ed650246849dc757f8b1dd67
Reviewed-on: https://chromium-review.googlesource.com/704175
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508133}
[modify] https://crrev.com/fcb42036b8a3a9c62fdec2d18f7a638c989f9c55/chrome/browser/extensions/extension_service.cc
[modify] https://crrev.com/fcb42036b8a3a9c62fdec2d18f7a638c989f9c55/chrome/browser/extensions/extension_service.h
[modify] https://crrev.com/fcb42036b8a3a9c62fdec2d18f7a638c989f9c55/chrome/browser/extensions/extension_service_test_with_install.cc
[modify] https://crrev.com/fcb42036b8a3a9c62fdec2d18f7a638c989f9c55/chrome/browser/ui/app_list/extension_app_model_builder_unittest.cc
[modify] https://crrev.com/fcb42036b8a3a9c62fdec2d18f7a638c989f9c55/extensions/browser/extension_registrar.cc
[modify] https://crrev.com/fcb42036b8a3a9c62fdec2d18f7a638c989f9c55/extensions/browser/extension_registrar.h
[modify] https://crrev.com/fcb42036b8a3a9c62fdec2d18f7a638c989f9c55/extensions/browser/extension_registrar_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Feb 8 2018

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

commit 573992dbc1e09f463753f6fbeff536d9ca173f33
Author: Michael Giuffrida <michaelpg@chromium.org>
Date: Thu Feb 08 00:48:44 2018

Add reload function to ExtensionRegistrar

This moves some of the extension reload logic from Chrome's
ExtensionService into //extensions via ExtensionRegistrar.
Reloading is the last major extension lifetime function to be moved
from ExtensionService to ExtensionRegistrar.

ExtensionRegistrar now tracks some state that ExtensionService used to
store:
  * extensions being reloaded
  * paths for extensions that have been unloaded (in case they are
    reloaded later)
  * orphaned DevTools instances (to be reattached upon reloading the
    extension)

Bug: 770879
Change-Id: If6eb8abc52df09bb9d15c94df8bb9a570d1f4542
Reviewed-on: https://chromium-review.googlesource.com/855527
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535223}
[modify] https://crrev.com/573992dbc1e09f463753f6fbeff536d9ca173f33/chrome/browser/extensions/extension_service.cc
[modify] https://crrev.com/573992dbc1e09f463753f6fbeff536d9ca173f33/chrome/browser/extensions/extension_service.h
[modify] https://crrev.com/573992dbc1e09f463753f6fbeff536d9ca173f33/extensions/browser/extension_registrar.cc
[modify] https://crrev.com/573992dbc1e09f463753f6fbeff536d9ca173f33/extensions/browser/extension_registrar.h
[modify] https://crrev.com/573992dbc1e09f463753f6fbeff536d9ca173f33/extensions/browser/extension_registrar_unittest.cc

Cc: -catmulli...@chromium.org

Sign in to add a comment