Perform install tasks for newly installed or upgraded component apps/extensions.

Perform install tasks for newly installed or upgraded component apps/extensions.
This ensures, e.g., that background pages for component extensions are run and
that their event listeners are registered appropriately. The change also saves
manifest information into profile preferences for component applications so that
the version can be tracked, which was not previously done.

BUG=157717
TEST=browser_test PlatformAppBrowserTest.ComponentAppBackgroundPages

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170137

[tapted, 2012-11-20 11:26:28]

@aa I hope I can impose on you for a your take on this, and a review. Lots more
info/rationale at http://crbug.com/157717

https://codereview.chromium.org/11275069/diff/17005/chrome/browser/performanc...
File chrome/browser/performance_monitor/performance_monitor_browsertest.cc
(right):

https://codereview.chromium.org/11275069/diff/17005/chrome/browser/performanc...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:677: //
Load the second profile, which has also exited uncleanly. Note that since
see http://crbug.com/157717#c7 for more info about why this test is changing

[tapted, 2012-11-20 23:31:34]

Forgot a note I meant to add in extension_service.cc (below).

Also, there are some tryjobs in patchset 3 - mac_rel is green. linux_rel hit
some unrelated issues. But some of the things on win_rel suggest to me that we
will introduce flakes if we allow component extension background pages to run in
browser tests. There was a workaround for this already in
ExtensionPrefs::CheckRegisteredEventsUpToDate():

http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...

i.e. 

bool ExtensionPrefs::CheckRegisteredEventsUpToDate() {
  // If we're running inside a test, then assume prefs are all up-to-date.
  if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType))
    return true;
/* snip */

I'll start some jobs with a similar check, to avoid adding component extensions
known to have background pages during tests.

https://codereview.chromium.org/11275069/diff/17005/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11275069/diff/17005/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2103: void
ExtensionService::AddComponentExtension(const Extension* extension) {
If this feels heavyweight, an option I considered here was to check
extension->has_background_page(), and just use the old behavior if false.

[Aaron Boodman, 2012-11-26 22:14:25]

.redirect("yoz")

[Yoyo Zhou, 2012-11-27 23:54:18]

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
chrome/browser/extensions/component_loader.cc:394: if
(!enable_background_extenstions_during_testing &&
typo: extensions

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2110: << (old_version.IsValid() ?
old_version.GetString() : "<none>")
GetVersionString gives you a string; you might as well use that here.

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
chrome/browser/extensions/platform_app_browsertest.cc:828: // extension
utilising a background page (i.e. a v2 platform app) has its
I think you meant e.g. not i.e.

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
chrome/browser/extensions/platform_app_browsertest.cc:833:
ExtensionTestMessageListener launched_listener("Launched", false);
Declare this closer to the point where it might start being used (say, at 843).

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
chrome/browser/extensions/platform_app_browsertest.cc:834:
content::WindowedNotificationObserver app_loaded_observer(
What is the purpose of this observer? If according to the comment at 858 it's
for the app being installed, why not observe NOTIFICATION_EXTENSION_INSTALLED?
It seems like the background page ought to be loaded regardless of whether the
app is installed.

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
chrome/browser/extensions/platform_app_browsertest.cc:859: // re-adding it in
the same profile should not cause it to be re-installed.
Is this tested?

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
chrome/browser/extensions/platform_app_browsertest.cc:873:
(void)extension_prefs;
What is this for?

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
chrome/browser/extensions/platform_app_browsertest.cc:875: // Clear the
registered events to ensure they are updated.
This comment should be at 870.

[tapted, 2012-11-28 06:21:13]

Thank you for the review! PTAL

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
chrome/browser/extensions/component_loader.cc:394: if
(!enable_background_extenstions_during_testing &&
On 2012/11/27 23:54:18, Yoyo Zhou wrote:
> typo: extensions

Drat. Done.

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2110: << (old_version.IsValid() ?
old_version.GetString() : "<none>")
On 2012/11/27 23:54:18, Yoyo Zhou wrote:
> GetVersionString gives you a string; you might as well use that here.

Done. (I think that's what you had in mind).

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
chrome/browser/extensions/platform_app_browsertest.cc:828: // extension
utilising a background page (i.e. a v2 platform app) has its
On 2012/11/27 23:54:18, Yoyo Zhou wrote:
> I think you meant e.g. not i.e.

Done.

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
chrome/browser/extensions/platform_app_browsertest.cc:833:
ExtensionTestMessageListener launched_listener("Launched", false);
On 2012/11/27 23:54:18, Yoyo Zhou wrote:
> Declare this closer to the point where it might start being used (say, at
843).

Done.

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
chrome/browser/extensions/platform_app_browsertest.cc:834:
content::WindowedNotificationObserver app_loaded_observer(
On 2012/11/27 23:54:18, Yoyo Zhou wrote:
> What is the purpose of this observer? If according to the comment at 858 it's
> for the app being installed, why not observe NOTIFICATION_EXTENSION_INSTALLED?
> It seems like the background page ought to be loaded regardless of whether the
> app is installed.

I think NOTIFICATION_EXTENSION_INSTALLED comes out too early for this case.
NOTIFICATION_EXTENSION_INSTALLED happens when the extension is added to
ExtensionService, just before a call to AddExtension, which then calls
NotifyExtensionLoaded, which emits NOTIFICATION_EXTENSION_LOADED to be picked up
by the EventRouter, and only at that point would any pre-registered lazy
listeners get registered, thus making it safe to call OpenApplication (since the
App's OnLaunched event would now be registered).

In the 'old' scenario, component extensions are never installed, so that code
path (via NOTIFICATION_EXTENSION_LOADED when ComponentLoader called AddExtension
directly) meant that there were no lazy listeners, OnLaunched never registered,
and the app could not start.

Since this CL would add installation, we need to wait for the background page to
be run (now, in response to a PostTask to call DispatchOnInstalledEvent() in
event_router.cc [triggered by NOTIFICATION_EXTENSION_INSTALLED] winding up in
EventRouter::MaybeLoadLazyBackgroundPageToDispatchEvent(..) ). Once the
background page has run (and its OnLaunched listener registered)
NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME is sent out. So, waiting on that allows
the test to ensure the OnLaunched event is registered before trying to launch
the app.

A lot of the platform apps use this technique (encapsulated in
PlatformAppBrowserTest::LoadAndLaunchPlatformApp(..) 
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
). However, that utility function loads the extension as a "regular" extension,
from a directory. However, we need to test loading the extension as a component
extension, so I decomposed it.

I'll add a comment describing this better.

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
chrome/browser/extensions/platform_app_browsertest.cc:859: // re-adding it in
the same profile should not cause it to be re-installed.
On 2012/11/27 23:54:18, Yoyo Zhou wrote:
> Is this tested?

Initially I had thought checking for an event to _not_ happen would be too
flaky, but I think I have a good solution in the next patchset - let me know
what you think.

Link:
https://codereview.chromium.org/11275069/diff/13038/chrome/browser/extensions...

Essentially, an observer for chrome::NOTIFICATION_EXTENSION_INSTALLED that just
sets a flag when observed. Test parts 1 and 3 assert the flag gets set before
launch. Test part 2 asserts the flag is not set.

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
chrome/browser/extensions/platform_app_browsertest.cc:873:
(void)extension_prefs;
On 2012/11/27 23:54:18, Yoyo Zhou wrote:
> What is this for?
Ah, sorry - that was a stray from experimenting with other ways to simulate the
"downgrade". Removed.

https://codereview.chromium.org/11275069/diff/10051/chrome/browser/extensions...
chrome/browser/extensions/platform_app_browsertest.cc:875: // Clear the
registered events to ensure they are updated.
On 2012/11/27 23:54:18, Yoyo Zhou wrote:
> This comment should be at 870.

Done.

https://codereview.chromium.org/11275069/diff/13038/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

https://codereview.chromium.org/11275069/diff/13038/chrome/browser/extensions...
chrome/browser/extensions/platform_app_browsertest.cc:829: // Simple observer to
check for NOTIFICATION_EXTENSION_INSTALLED events to
note: link to new code

https://codereview.chromium.org/11275069/diff/13038/chrome/browser/extensions...
chrome/browser/extensions/platform_app_browsertest.cc:910:
ASSERT_FALSE(should_not_install.seen());
note: new check to verify re-installation does not occur

[tapted, 2012-11-28 07:30:35]

Just a heads-up/note: there was a slightly non-trivial rebase (isolated in
patchset 9) required due to http://crrev.com/11415020 from earlier this week
[refactoring around IdleInstall].

I think this CL is still OK in light of the new functional decomposition.

patchset 8 has just the changes to address the earlier review.

https://codereview.chromium.org/11275069/diff/10074/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11275069/diff/10074/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2412:
FinishInstallation(extension);
notable change from rebase

[Yoyo Zhou, 2012-11-28 22:02:30]

LGTM, makes sense. Thanks for doing the rebase in a separate patchset; it makes
it much easier to see your changes.

[tapted, 2012-11-28 23:46:24]

+sky for an OWNERS review in c/b/ui/views (or any additional warmly-welcomed
feedback ;). link to file:
https://codereview.chromium.org/11275069/diff/13041/chrome/browser/ui/views/s...

Context:
That file tests the ChromeOS file manager extension ('file_manager_extension!=0'
in chrome/chrome_tests.gypi), which uses a background page.
Background pages launch renderers which can do funny things with other browser
tests (e.g. win_rel failures in patchset 4).
They were previously disabled as a side-effect of
ExtensionPrefs::CheckRegisteredEventsUpToDate() always returning 'true' in
tests, but performing the install tasks for component extensions makes updating
events pages explicit.


Also cc grv@ wrt r169911 r170087 - you will like this change ;) (it will save
you from http://crbug.com/157717), but something to keep in mind if writing
browser tests for component extensions with background pages

(note: also rebasing to incorporate r169911, r170087 so that it the
apps_debugger component app is moved into new background pages section in
component_loader.cc

 note re:trybots lkgr hasn't caught up yet with the thing I need to rebase
against -- I've started some jobs with a specific, recent revision and will keep
an eye on lkgr before committing)

[sky, 2012-11-29 01:50:06]

LGTM

[commit-bot: I haz the power, 2012-11-29 04:38:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/11275069/13041

[commit-bot: I haz the power, 2012-11-29 06:05:41]

Change committed as 170137