Check import module update before launching the primary kiosk app.

Check import module update before launching the primary kiosk app.

Add a test case for verifying launching app with updating a shared module to the new version.

Re-packaged several testing apps downloaded from webstore to remove the update url from their manifest files.

BUG=555083
Committed: https://crrev.com/bb7ef8a93d64d6d3c23edb5095db8a441f1b5b7c
Cr-Commit-Position: refs/heads/master@{#363038}

[jennyz, 2015-12-02 01:28:22]

Description was changed from

==========
Check import module update before launching the primary kiosk app.

BUG=555083
==========

to

==========
Check import module update before launching the primary kiosk app.

Add a test case for verifying launching app with updating a shared module to the
new version. 

Re-packaged several testing apps downloaded from webstore to remove the update
url from their manifest files.

BUG=555083
==========

[jennyz, 2015-12-02 01:28:22]

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

[xiyuan, 2015-12-02 21:04:21]

https://codereview.chromium.org/1488393002/diff/1/chrome/browser/chromeos/app...
File chrome/browser/chromeos/app_mode/startup_app_launcher.cc (right):

https://codereview.chromium.org/1488393002/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:288:
content::NotificationService::AllSources());
Move line 286-287 before updater->CheckNow() call, just in case the update check
callback is invoked before we run here.

https://codereview.chromium.org/1488393002/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:301:
->EnableExtension(app_id_);
Can we use ExtensionService::ReloadExtension?

https://codereview.chromium.org/1488393002/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:303:
content::NotificationService::AllSources());
Should we move this out of the "if"?

https://codereview.chromium.org/1488393002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1488393002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/kiosk_browsertest.cc:2044: }
Should we do a

    EXPECT_TRUE(IsAppInstalled(kTestSharedModuleId, "2.0.0"));

here and a test of 1.0.0 at the beginning of the test case (or at the end of the
PRE test) to ensure the update actually happened?

[jennyz, 2015-12-03 00:27:55]

https://codereview.chromium.org/1488393002/diff/1/chrome/browser/chromeos/app...
File chrome/browser/chromeos/app_mode/startup_app_launcher.cc (right):

https://codereview.chromium.org/1488393002/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:288:
content::NotificationService::AllSources());
On 2015/12/02 21:04:20, xiyuan wrote:
> Move line 286-287 before updater->CheckNow() call, just in case the update
check
> callback is invoked before we run here.

Done.

https://codereview.chromium.org/1488393002/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:301:
->EnableExtension(app_id_);
On 2015/12/02 21:04:20, xiyuan wrote:
> Can we use ExtensionService::ReloadExtension?

Done.

https://codereview.chromium.org/1488393002/diff/1/chrome/browser/chromeos/app...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:303:
content::NotificationService::AllSources());
On 2015/12/02 21:04:20, xiyuan wrote:
> Should we move this out of the "if"?

Done.

https://codereview.chromium.org/1488393002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1488393002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/kiosk_browsertest.cc:2044: }
On 2015/12/02 21:04:20, xiyuan wrote:
> Should we do a
> 
>     EXPECT_TRUE(IsAppInstalled(kTestSharedModuleId, "2.0.0"));
> 
> here and a test of 1.0.0 at the beginning of the test case (or at the end of
the
> PRE test) to ensure the update actually happened?

Done.

[xiyuan, 2015-12-03 00:46:53]

Mostly good.

https://codereview.chromium.org/1488393002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/fake_cws.cc (right):

https://codereview.chromium.org/1488393002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/fake_cws.cc:100: update_url.spec());
OverrideGalleryCommandlineSwitches() does this too. Can we use it here?

https://codereview.chromium.org/1488393002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/startup_app_launcher.cc (right):

https://codereview.chromium.org/1488393002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:287:
content::NotificationService::AllSources());
Sorry for the confusion. I meant to moving this before the whole CheckNow block.
That is, before line 277

https://codereview.chromium.org/1488393002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1488393002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/kiosk_browsertest.cc:542: &extension_cache_dir));
Oops. We really should not be doing this. We should create a temp dir using
ScopedTempDir and override it with ScopedPathOverride.

Otherwise, tests may fail when the default dir is not present or writable on the
bots, and we leave some state which might affect other tests.

[jennyz, 2015-12-03 01:23:47]

https://codereview.chromium.org/1488393002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/fake_cws.cc (right):

https://codereview.chromium.org/1488393002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/fake_cws.cc:100: update_url.spec());
On 2015/12/03 00:46:53, xiyuan wrote:
> OverrideGalleryCommandlineSwitches() does this too. Can we use it here?

Done.

https://codereview.chromium.org/1488393002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/startup_app_launcher.cc (right):

https://codereview.chromium.org/1488393002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:287:
content::NotificationService::AllSources());
On 2015/12/03 00:46:53, xiyuan wrote:
> Sorry for the confusion. I meant to moving this before the whole CheckNow
block.
> That is, before line 277

Done.

https://codereview.chromium.org/1488393002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1488393002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/kiosk_browsertest.cc:542: &extension_cache_dir));
On 2015/12/03 00:46:53, xiyuan wrote:
> Oops. We really should not be doing this. We should create a temp dir using
> ScopedTempDir and override it with ScopedPathOverride.
> 
> Otherwise, tests may fail when the default dir is not present or writable on
the
> bots, and we leave some state which might affect other tests.

It looks like the path is already overrided at RegisterStubPathOverrides() and
created under a temporary directory:
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/chromeos_...

[xiyuan, 2015-12-03 03:05:38]

lgtm

https://codereview.chromium.org/1488393002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1488393002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/kiosk_browsertest.cc:542: &extension_cache_dir));
On 2015/12/03 01:23:47, jennyz wrote:
> On 2015/12/03 00:46:53, xiyuan wrote:
> > Oops. We really should not be doing this. We should create a temp dir using
> > ScopedTempDir and override it with ScopedPathOverride.
> > 
> > Otherwise, tests may fail when the default dir is not present or writable on
> the
> > bots, and we leave some state which might affect other tests.
> 
> It looks like the path is already overrided at RegisterStubPathOverrides() and
> created under a temporary directory:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/chromeos_...

I see. Never mind then.

[jennyz, 2015-12-03 04:25:51]

The CQ bit was checked by jennyz@chromium.org

[commit-bot: I haz the power, 2015-12-03 04:26:48]

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

[commit-bot: I haz the power, 2015-12-03 06:41:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-03 06:41:36]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[jennyz, 2015-12-03 08:09:44]

The CQ bit was checked by jennyz@chromium.org

[commit-bot: I haz the power, 2015-12-03 08:10:58]

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

[commit-bot: I haz the power, 2015-12-03 10:15:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-03 10:15:05]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[jennyz, 2015-12-03 18:22:16]

The CQ bit was checked by jennyz@chromium.org

[jennyz, 2015-12-03 18:22:16]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/1488393002/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-12-03 18:28:49]

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

[commit-bot: I haz the power, 2015-12-03 20:07:20]

Description was changed from

==========
Check import module update before launching the primary kiosk app.

Add a test case for verifying launching app with updating a shared module to the
new version. 

Re-packaged several testing apps downloaded from webstore to remove the update
url from their manifest files.

BUG=555083
==========

to

==========
Check import module update before launching the primary kiosk app.

Add a test case for verifying launching app with updating a shared module to the
new version. 

Re-packaged several testing apps downloaded from webstore to remove the update
url from their manifest files.

BUG=555083
==========

[commit-bot: I haz the power, 2015-12-03 20:07:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-03 20:08:09]

Description was changed from

==========
Check import module update before launching the primary kiosk app.

Add a test case for verifying launching app with updating a shared module to the
new version. 

Re-packaged several testing apps downloaded from webstore to remove the update
url from their manifest files.

BUG=555083
==========

to

==========
Check import module update before launching the primary kiosk app.

Add a test case for verifying launching app with updating a shared module to the
new version.

Re-packaged several testing apps downloaded from webstore to remove the update
url from their manifest files.

BUG=555083

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

[commit-bot: I haz the power, 2015-12-03 20:08:10]

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