extensions: Abstract out install delay strategy

extensions: Abstract out install delay strategy

- Add a InstallGate interface that ExtensionService invokes
  to find out delayed install actions for a pending install;
- Move update-when-idle logic into a UpdateInstallGate class;
- Implement InstallGate in ExtensionGarbageCollector and
  SharedModuleService wit code moved from ExtensionService;
- Register those install gates with ExtensionService in
  ExtensionSystemImpl;
- TestExtensionSysmte is only interested in SharedModuleService
  related install delay strategy so only register that;

BUG=585143
Committed: https://crrev.com/f6a4c6a676ecf8f990e4cbe3a47f6ad955367711
Cr-Commit-Position: refs/heads/master@{#388245}

[xiyuan, 2016-04-15 20:04:22]

xiyuan@chromium.org changed reviewers:
+ asargent@chromium.org

[xiyuan, 2016-04-15 20:04:23]

Following your suggestion in https://codereview.chromium.org/1869483002/, this
CL creates an InstallDelayer interface and moves the impl specifics out of
ExtensionService.

Please take a look. Thanks.

https://codereview.chromium.org/1890163004/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (left):

https://codereview.chromium.org/1890163004/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1881: reason ==
extensions::ExtensionPrefs::DELAY_REASON_NONE) &&
Note the changed behavior here. Before the CL, the delay reason is used to
explicitly check whether the condition that defers the install still stands.
This is actually not 100% correct. 

e.g.

suppose we have a pending install that requires a shared module but the install
is delayed by garbage collector. When gc finishes, we get here and since the
reason is not DELAY_REASON_WAIT_FOR_IMPORTS, shared module deps check is
skipped. This does not sound quite right.

After the CL, all install delayers would be checked. The delayed install only
finishes when all delayers agree to install.

[asargent_no_longer_on_chrome, 2016-04-16 00:20:22]

https://codereview.chromium.org/1890163004/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/install_delayer.h (right):

https://codereview.chromium.org/1890163004/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/install_delayer.h:14: class InstallDelayer {
naming suggestion: I was thinking perhaps "InstallGate" might sound a little
better than "InstallDelayer", what do you think?

https://codereview.chromium.org/1890163004/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/install_delayer.h:28: virtual Action
GetDelayedInstallAction(const Extension* extension,
naming suggestion: consider something shorter like "ShouldDelay"

https://codereview.chromium.org/1890163004/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/update_install_delayer.cc (right):

https://codereview.chromium.org/1890163004/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/update_install_delayer.cc:22: if (install_immediately
|| !service_->is_ready())
It's unintuitive that we'd return INSTALL if service_->is_ready() returns false
- I would expect this to be DELAY or ABORT instead. Can you include a comment
here explaining why we do it this way?

Alternatively, maybe the ExtensionService::is_ready check shouldn't really be
here, but rather should be called inside ExtensionService wherever this method
is called?

[xiyuan, 2016-04-18 20:34:30]

Description was changed from

==========
extensions: Abstract out install delay strategy

- Add a InstallDelayer interface that ExtensionService invokes
  to find out delayed install actions for a pending install;
- Move update-when-idle logic into a UpdateInstallDelayer class;
- Implement InstallDelayer in ExtensionGarbageCollector and
  SharedModuleService wit code moved from ExtensionService;
- Register those install delayers with ExtensionService in
  ExtensionSystemImpl;
- TestExtensionSysmte is only interested in SharedModuleService
  related install delay strategy so only register that;

BUG=585143
==========

to

==========
extensions: Abstract out install delay strategy

- Add a InstallGate interface that ExtensionService invokes
  to find out delayed install actions for a pending install;
- Move update-when-idle logic into a UpdateInstallGate class;
- Implement InstallGate in ExtensionGarbageCollector and
  SharedModuleService wit code moved from ExtensionService;
- Register those install delayers with ExtensionService in
  ExtensionSystemImpl;
- TestExtensionSysmte is only interested in SharedModuleService
  related install delay strategy so only register that;

BUG=585143
==========

[xiyuan, 2016-04-18 20:44:30]

https://codereview.chromium.org/1890163004/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/install_delayer.h (right):

https://codereview.chromium.org/1890163004/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/install_delayer.h:14: class InstallDelayer {
On 2016/04/16 00:20:22, Antony Sargent wrote:
> naming suggestion: I was thinking perhaps "InstallGate" might sound a little
> better than "InstallDelayer", what do you think?
> 

Done.

https://codereview.chromium.org/1890163004/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/install_delayer.h:28: virtual Action
GetDelayedInstallAction(const Extension* extension,
On 2016/04/16 00:20:22, Antony Sargent wrote:
> naming suggestion: consider something shorter like "ShouldDelay" 

Done.

https://codereview.chromium.org/1890163004/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/update_install_delayer.cc (right):

https://codereview.chromium.org/1890163004/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/update_install_delayer.cc:22: if (install_immediately
|| !service_->is_ready())
On 2016/04/16 00:20:22, Antony Sargent wrote:
> It's unintuitive that we'd return INSTALL if service_->is_ready() returns
false
> - I would expect this to be DELAY or ABORT instead. Can you include a comment
> here explaining why we do it this way?
> 
> Alternatively, maybe the ExtensionService::is_ready check shouldn't really be
> here, but rather should be called inside ExtensionService wherever this method
> is called?
> 

Added a comment to explain. The update is blocked because the old version of the
extension is not idle. When ExtensionService is not ready (i.e. during
initialization), the old version is definitely idle and we allow the
installation to proceed to take the opportunity to switch to the new version.
This is the existing behavior.[1]

Checking it on ExtensionService's call site is not good because this logic is
specific to the update. The delay logic for shared module (and incoming kiosk
platform check) does not (and should not) consider the
ExtensionService::is_ready condition.

[1]:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

[xiyuan, 2016-04-18 20:51:57]

Description was changed from

==========
extensions: Abstract out install delay strategy

- Add a InstallGate interface that ExtensionService invokes
  to find out delayed install actions for a pending install;
- Move update-when-idle logic into a UpdateInstallGate class;
- Implement InstallGate in ExtensionGarbageCollector and
  SharedModuleService wit code moved from ExtensionService;
- Register those install delayers with ExtensionService in
  ExtensionSystemImpl;
- TestExtensionSysmte is only interested in SharedModuleService
  related install delay strategy so only register that;

BUG=585143
==========

to

==========
extensions: Abstract out install delay strategy

- Add a InstallGate interface that ExtensionService invokes
  to find out delayed install actions for a pending install;
- Move update-when-idle logic into a UpdateInstallGate class;
- Implement InstallGate in ExtensionGarbageCollector and
  SharedModuleService wit code moved from ExtensionService;
- Register those install gates with ExtensionService in
  ExtensionSystemImpl;
- TestExtensionSysmte is only interested in SharedModuleService
  related install delay strategy so only register that;

BUG=585143
==========

[asargent_no_longer_on_chrome, 2016-04-19 17:29:56]

lgtm

[xiyuan, 2016-04-19 17:32:39]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2016-04-19 17:33:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1890163004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1890163004/80001

[commit-bot: I haz the power, 2016-04-19 18:15:08]

Description was changed from

==========
extensions: Abstract out install delay strategy

- Add a InstallGate interface that ExtensionService invokes
  to find out delayed install actions for a pending install;
- Move update-when-idle logic into a UpdateInstallGate class;
- Implement InstallGate in ExtensionGarbageCollector and
  SharedModuleService wit code moved from ExtensionService;
- Register those install gates with ExtensionService in
  ExtensionSystemImpl;
- TestExtensionSysmte is only interested in SharedModuleService
  related install delay strategy so only register that;

BUG=585143
==========

to

==========
extensions: Abstract out install delay strategy

- Add a InstallGate interface that ExtensionService invokes
  to find out delayed install actions for a pending install;
- Move update-when-idle logic into a UpdateInstallGate class;
- Implement InstallGate in ExtensionGarbageCollector and
  SharedModuleService wit code moved from ExtensionService;
- Register those install gates with ExtensionService in
  ExtensionSystemImpl;
- TestExtensionSysmte is only interested in SharedModuleService
  related install delay strategy so only register that;

BUG=585143
==========

[commit-bot: I haz the power, 2016-04-19 18:15:10]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-22 19:13:46]

Description was changed from

==========
extensions: Abstract out install delay strategy

- Add a InstallGate interface that ExtensionService invokes
  to find out delayed install actions for a pending install;
- Move update-when-idle logic into a UpdateInstallGate class;
- Implement InstallGate in ExtensionGarbageCollector and
  SharedModuleService wit code moved from ExtensionService;
- Register those install gates with ExtensionService in
  ExtensionSystemImpl;
- TestExtensionSysmte is only interested in SharedModuleService
  related install delay strategy so only register that;

BUG=585143
==========

to

==========
extensions: Abstract out install delay strategy

- Add a InstallGate interface that ExtensionService invokes
  to find out delayed install actions for a pending install;
- Move update-when-idle logic into a UpdateInstallGate class;
- Implement InstallGate in ExtensionGarbageCollector and
  SharedModuleService wit code moved from ExtensionService;
- Register those install gates with ExtensionService in
  ExtensionSystemImpl;
- TestExtensionSysmte is only interested in SharedModuleService
  related install delay strategy so only register that;

BUG=585143

Committed: https://crrev.com/f6a4c6a676ecf8f990e4cbe3a47f6ad955367711
Cr-Commit-Position: refs/heads/master@{#388245}
==========

[commit-bot: I haz the power, 2016-04-22 19:13:47]

Patchset 5 (id:??) landed as
https://crrev.com/f6a4c6a676ecf8f990e4cbe3a47f6ad955367711
Cr-Commit-Position: refs/heads/master@{#388245}