kiosk: Defer app update until platform matches

kiosk: Defer app update until platform matches

- Add KioskAppManager::IsPlatformCompliant(WithApp) helpers;
- Add a KioskAppUpdateInstallDelayer to defer app update
  if the platform version is not compliant;
- Update required platform version meta data when an app update
  is deferred so that update engine can move forward with the new
  platform version;
- Update app meta data only when the app does not have delayed
  installs during launch;

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

[xiyuan, 2016-04-06 16:02:07]

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

[xiyuan, 2016-04-06 16:02:50]

+jinzhang fyi

[asargent_no_longer_on_chrome, 2016-04-06 19:32:23]

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

https://codereview.chromium.org/1869483002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1835: #endif
This function is already getting kind of long and complicated, so instead of
adding another block of ifdef'd code, I'd prefer we do the following:

-Add some sort of "InstallDelayer" interface with a method can be be called to
see if they'd like to delay install of an extension, and also a method that
would be called down in MaybeFinishDelayedInstallation. The ExtensionService
will have a way for objects implementing this interface to register themselves.
You could then have an object that implements this interface for chromeos, and
it would register with the ExtensionService in one of the existing places where
we do similar stuff to setup the ExtensionService, perhaps in
extension_system_impl.cc. 

-We'd consolidate all the checks related to delaying install into
ShouldDelayExtensionUpdate, including checking with all the registered
InstallDelayer's. Also the SharedModule checks above could probably be moved
into the SharedModuleService, and make the SharedModuleService implement the
InstallDelayer interface. 


Basically I think our goal should be to make ExtensionService a general purpose
object that doesn't have any #ifdef OS_CHROMEOS blocks of code in it, but rather
has config values and delegate objects to abstract different behaviors.

[xiyuan, 2016-04-15 21:58:17]

Description was changed from

==========
kiosk: Defer app update until platform matches

- Add KioskAppManager::IsPlatformCompliant(WithApp) helpers;
- Defer app install if the platform version is not compliant;
- Update required platform version meta data when an app install
  is deferred so that update engine can move forward with the new
  platform version;
- Update app meta data only when the app does not have delayed
  installs during launch;

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

to

==========
kiosk: Defer app update until platform matches

- Add KioskAppManager::IsPlatformCompliant(WithApp) helpers;
- Add a KioskAppUpdateInstallDelayer to defer app update
  if the platform version is not compliant;
- Update required platform version meta data when an app update
  is deferred so that update engine can move forward with the new
  platform version;
- Update app meta data only when the app does not have delayed
  installs during launch;

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

[xiyuan, 2016-04-19 18:38:53]

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

[xiyuan, 2016-04-19 18:38:54]

+jenny

CL rebased on top of the InstallGate change. PTAL. Thanks.

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

https://codereview.chromium.org/1869483002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1835: #endif
On 2016/04/06 19:32:23, Antony Sargent wrote:
> This function is already getting kind of long and complicated, so instead of
> adding another block of ifdef'd code, I'd prefer we do the following:
> 
> -Add some sort of "InstallDelayer" interface with a method can be be called to
> see if they'd like to delay install of an extension, and also a method that
> would be called down in MaybeFinishDelayedInstallation. The ExtensionService
> will have a way for objects implementing this interface to register
themselves.
> You could then have an object that implements this interface for chromeos, and
> it would register with the ExtensionService in one of the existing places
where
> we do similar stuff to setup the ExtensionService, perhaps in
> extension_system_impl.cc. 
> 
> -We'd consolidate all the checks related to delaying install into
> ShouldDelayExtensionUpdate, including checking with all the registered
> InstallDelayer's. Also the SharedModule checks above could probably be moved
> into the SharedModuleService, and make the SharedModuleService implement the
> InstallDelayer interface. 
> 
> 
> Basically I think our goal should be to make ExtensionService a general
purpose
> object that doesn't have any #ifdef OS_CHROMEOS blocks of code in it, but
rather
> has config values and delegate objects to abstract different behaviors. 
> 

Done.

[jennyz, 2016-04-19 21:54:28]

lgtm with nits. BTW, where are the source files for the testing crx?

https://codereview.chromium.org/1869483002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/1869483002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:585: // Empty required
version are compliant with any platform version.
nit: are->is?

https://codereview.chromium.org/1869483002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:591: if
(!required_version.IsValid() || required_version.components().size() > 3u)
Will it be helpful to add some log for such case?

https://codereview.chromium.org/1869483002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_update_install_gate.cc (right):

https://codereview.chromium.org/1869483002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_update_install_gate.cc:25: if
(!registry_->GetInstalledExtension(extension->id()) ||
For first install case, if the required platform version happens to be not
compliant, will it be helpful if we add a log message for diagnostic purpose?

[xiyuan, 2016-04-19 22:28:45]

I did not add new testing crx, just moved it to webstore dir to make it work
with FakeCWS and KioskUpdateTest.

The source is added when we add the test case
KioskAppManagerTest.UpdateAppDataFromCrx, located under:

chrome/test/data/chromeos/app_mode/offline_enabled_kiosk_app/v2_required_platform_version_added/

https://codereview.chromium.org/1869483002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/1869483002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:585: // Empty required
version are compliant with any platform version.
On 2016/04/19 21:54:28, jennyz wrote:
> nit: are->is?

Done.

https://codereview.chromium.org/1869483002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:591: if
(!required_version.IsValid() || required_version.components().size() > 3u)
On 2016/04/19 21:54:28, jennyz wrote:
> Will it be helpful to add some log for such case? 

Done.

https://codereview.chromium.org/1869483002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_update_install_gate.cc (right):

https://codereview.chromium.org/1869483002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_update_install_gate.cc:25: if
(!registry_->GetInstalledExtension(extension->id()) ||
On 2016/04/19 21:54:28, jennyz wrote:
> For first install case, if the required platform version happens to be not
> compliant, will it be helpful if we add a log message for diagnostic purpose?

Done.

[asargent_no_longer_on_chrome, 2016-04-19 22:59:43]

lgtm

[xiyuan, 2016-04-19 23:04:02]

Thanks guys for the quick review.

[xiyuan, 2016-04-19 23:06:41]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2016-04-19 23:06:42]

The patchset sent to the CQ was uploaded after l-g-t-m from jennyz@chromium.org
Link to the patchset: https://codereview.chromium.org/1869483002/#ps60001
(title: "for comments from jenny")

[commit-bot: I haz the power, 2016-04-19 23:07:17]

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

[commit-bot: I haz the power, 2016-04-19 23:34:25]

Description was changed from

==========
kiosk: Defer app update until platform matches

- Add KioskAppManager::IsPlatformCompliant(WithApp) helpers;
- Add a KioskAppUpdateInstallDelayer to defer app update
  if the platform version is not compliant;
- Update required platform version meta data when an app update
  is deferred so that update engine can move forward with the new
  platform version;
- Update app meta data only when the app does not have delayed
  installs during launch;

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

to

==========
kiosk: Defer app update until platform matches

- Add KioskAppManager::IsPlatformCompliant(WithApp) helpers;
- Add a KioskAppUpdateInstallDelayer to defer app update
  if the platform version is not compliant;
- Update required platform version meta data when an app update
  is deferred so that update engine can move forward with the new
  platform version;
- Update app meta data only when the app does not have delayed
  installs during launch;

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

[commit-bot: I haz the power, 2016-04-19 23:34:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-22 19:18:06]

Description was changed from

==========
kiosk: Defer app update until platform matches

- Add KioskAppManager::IsPlatformCompliant(WithApp) helpers;
- Add a KioskAppUpdateInstallDelayer to defer app update
  if the platform version is not compliant;
- Update required platform version meta data when an app update
  is deferred so that update engine can move forward with the new
  platform version;
- Update app meta data only when the app does not have delayed
  installs during launch;

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

to

==========
kiosk: Defer app update until platform matches

- Add KioskAppManager::IsPlatformCompliant(WithApp) helpers;
- Add a KioskAppUpdateInstallDelayer to defer app update
  if the platform version is not compliant;
- Update required platform version meta data when an app update
  is deferred so that update engine can move forward with the new
  platform version;
- Update app meta data only when the app does not have delayed
  installs during launch;

BUG=585143

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

[commit-bot: I haz the power, 2016-04-22 19:18:07]

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

[Denis Kuznetsov (DE-MUC), 2016-07-22 14:36:45]

antrim@chromium.org changed reviewers:
+ antrim@chromium.org