Refactor code that defers extension background page loading

Refactor code that defers extension background page loading

src/extensions depends on chrome::NOTIFICATION_PROFILE_CREATED to support deferred loading of extension background pages when the profile isn't ready yet. This is a layering violation.

* Remove Chrome concepts like "browser window ready" and "profile created" from ProcessManager. Introduce ProcessManagerDelegate with a general concept of deferring background page loading.
* Consolidate all the tricky Chrome-specific background page loading rules into ChromeProcessManagerDelegate. This keeps all the rules in one place. Annotate each block of special case code with the bug that inspired it.
* Extend unit test coverage for ProcessManager.

This will make it easier to eliminate chrome::NOTIFICATION_PROFILE_DESTROYED in ProcessManager in a later CL.

BUG=392658
TEST=unit_tests ProcessManagerTest, browser_tests ProcessManagerBrowserTest, manual

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

[James Cook, 2014-07-11 00:04:24]

yoz, could I get some early feedback on the test pieces of this?

I've added tests and done some refactoring without getting rid of the
notification.  I'm curious if you think MockExtensionSystem(Provider) should be
in its own file, and if so if it should become the base class for
TestExtensionSystem.  I didn't do that because so far this is the only test
needing this sort of ExtensionSystem.  (And what would I call it? 
TestExtensionSystemBase?  I don't want to rename TestExtensionSystem -- it's
used in >100 places.)

Getting rid of the notification will be trivial, now that I know what's going
on.  See crbug.com/222473 and crrev.com/12437016 for some of the history of all
this deferred loading.

Please ignore the debug spam. :-)

[Yoyo Zhou, 2014-07-11 01:51:53]

https://codereview.chromium.org/381283002/diff/40001/extensions/DEPS
File extensions/DEPS (right):

https://codereview.chromium.org/381283002/diff/40001/extensions/DEPS#newcode3
extensions/DEPS:3: "-chrome",
I think this is superfluous but the comment is good. =)

https://codereview.chromium.org/381283002/diff/40001/extensions/browser/proce...
File extensions/browser/process_manager_unittest.cc (right):

https://codereview.chromium.org/381283002/diff/40001/extensions/browser/proce...
extensions/browser/process_manager_unittest.cc:84: class
MockExtensionSystemFactory : public ExtensionSystemProvider {
Is it possible to use SetTestingFactoryAndUse? You *might* be able to avoid some
of this boilerplate.

https://codereview.chromium.org/381283002/diff/40001/extensions/browser/proce...
extensions/browser/process_manager_unittest.cc:250:
chrome::NOTIFICATION_PROFILE_CREATED,
We're trying to avoid this in src/extensions, right?

https://codereview.chromium.org/381283002/diff/40001/extensions/browser/proce...
extensions/browser/process_manager_unittest.cc:267:
extensions_browser_client()->set_defer_loading_background_hosts(true);
I think this is an issue. If we set_defer_loading_background_hosts(true), that
alone prevents background hosts from being created. So it's hard to tell whether
the other stuff you're doing (sending EXTENSIONS_READY in this test) has any
effect on the expectation in 279. Does that make sense?

https://codereview.chromium.org/381283002/diff/40001/extensions/browser/proce...
extensions/browser/process_manager_unittest.cc:293: TEST_F(ProcessManagerTest,
DeferUntilBrowserWindowReady) {
Do you also want a test in the opposite order (browser window ready, then
extension system ready?)

[James Cook, 2014-07-11 21:19:29]

yoz, PTAL. I updated the CL description too.  It's a moderate amount of code
change, but consolidates all the deferred loading craziness in one place.

https://codereview.chromium.org/381283002/diff/40001/extensions/DEPS
File extensions/DEPS (right):

https://codereview.chromium.org/381283002/diff/40001/extensions/DEPS#newcode3
extensions/DEPS:3: "-chrome",
On 2014/07/11 01:51:53, Yoyo Zhou wrote:
> I think this is superfluous but the comment is good. =)

Done.

https://codereview.chromium.org/381283002/diff/40001/extensions/browser/proce...
File extensions/browser/process_manager_unittest.cc (right):

https://codereview.chromium.org/381283002/diff/40001/extensions/browser/proce...
extensions/browser/process_manager_unittest.cc:84: class
MockExtensionSystemFactory : public ExtensionSystemProvider {
On 2014/07/11 01:51:53, Yoyo Zhou wrote:
> Is it possible to use SetTestingFactoryAndUse? You *might* be able to avoid
some
> of this boilerplate.

It turns out I don't need any of this.

https://codereview.chromium.org/381283002/diff/40001/extensions/browser/proce...
extensions/browser/process_manager_unittest.cc:250:
chrome::NOTIFICATION_PROFILE_CREATED,
On 2014/07/11 01:51:53, Yoyo Zhou wrote:
> We're trying to avoid this in src/extensions, right?

Yes, this was the initial "test driven development" patch, trying to capture the
behavior of the system before I modified it. Now that notification is expunged
from src/extensions.

https://codereview.chromium.org/381283002/diff/40001/extensions/browser/proce...
extensions/browser/process_manager_unittest.cc:267:
extensions_browser_client()->set_defer_loading_background_hosts(true);
On 2014/07/11 01:51:53, Yoyo Zhou wrote:
> I think this is an issue. If we set_defer_loading_background_hosts(true), that
> alone prevents background hosts from being created. So it's hard to tell
whether
> the other stuff you're doing (sending EXTENSIONS_READY in this test) has any
> effect on the expectation in 279. Does that make sense?

This should be clearer now. From the extensions module's perspective, the
embedder can defer background host creation for any reason and it can request
that background hosts be created whenever something interesting happens.

https://codereview.chromium.org/381283002/diff/40001/extensions/browser/proce...
extensions/browser/process_manager_unittest.cc:293: TEST_F(ProcessManagerTest,
DeferUntilBrowserWindowReady) {
On 2014/07/11 01:51:53, Yoyo Zhou wrote:
> Do you also want a test in the opposite order (browser window ready, then
> extension system ready?)

More tests added.

[James Cook, 2014-07-14 22:35:33]

Friendly ping?

[Yoyo Zhou, 2014-07-15 02:13:22]

LGTM

https://codereview.chromium.org/381283002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/chrome_process_manager_delegate.cc (right):

https://codereview.chromium.org/381283002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/chrome_process_manager_delegate.cc:26:
ChromeProcessManagerDelegate::ChromeProcessManagerDelegate() {
I like this much better than the terribly unspecific ChromeNotificationObserver.

https://codereview.chromium.org/381283002/diff/80001/extensions/browser/proce...
File extensions/browser/process_manager.h (right):

https://codereview.chromium.org/381283002/diff/80001/extensions/browser/proce...
extensions/browser/process_manager.h:39: class ProcessManagerDelegate {
Probably it's best to put this in a separate header file.

https://codereview.chromium.org/381283002/diff/80001/extensions/browser/proce...
extensions/browser/process_manager.h:186: void CreateStartupBackgroundHosts();
nit: does this need to be protected? can it be in private?

https://codereview.chromium.org/381283002/diff/80001/extensions/browser/proce...
File extensions/browser/process_manager_unittest.cc (right):

https://codereview.chromium.org/381283002/diff/80001/extensions/browser/proce...
extensions/browser/process_manager_unittest.cc:206:
EXPECT_TRUE(manager->startup_background_hosts_created_for_test());
"Nothing changes" - hosts don't get uncreated, right? What does this test that
199 doesn't?

[James Cook, 2014-07-15 17:02:53]

Thanks!

https://codereview.chromium.org/381283002/diff/80001/extensions/browser/proce...
File extensions/browser/process_manager.h (right):

https://codereview.chromium.org/381283002/diff/80001/extensions/browser/proce...
extensions/browser/process_manager.h:39: class ProcessManagerDelegate {
On 2014/07/15 02:13:22, Yoyo Zhou wrote:
> Probably it's best to put this in a separate header file.

Done.

https://codereview.chromium.org/381283002/diff/80001/extensions/browser/proce...
extensions/browser/process_manager.h:186: void CreateStartupBackgroundHosts();
On 2014/07/15 02:13:22, Yoyo Zhou wrote:
> nit: does this need to be protected? can it be in private?

Good catch! Moved to private.

https://codereview.chromium.org/381283002/diff/80001/extensions/browser/proce...
File extensions/browser/process_manager_unittest.cc (right):

https://codereview.chromium.org/381283002/diff/80001/extensions/browser/proce...
extensions/browser/process_manager_unittest.cc:206:
EXPECT_TRUE(manager->startup_background_hosts_created_for_test());
On 2014/07/15 02:13:22, Yoyo Zhou wrote:
> "Nothing changes" - hosts don't get uncreated, right? What does this test that
> 199 doesn't?

Nothing really, I guess. I was just emulating what Chrome does. Removed.

[James Cook, 2014-07-15 17:15:17]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-07-15 17:16:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamescook@chromium.org/381283002/120001

[commit-bot: I haz the power, 2014-07-16 05:09:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-16 05:14:09]

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

[commit-bot: I haz the power, 2014-07-16 05:14:11]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[James Cook, 2014-07-16 17:13:24]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2014-07-16 17:13:58]

The CQ bit was unchecked by jamescook@chromium.org

[James Cook, 2014-07-16 17:18:57]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-07-16 17:22:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamescook@chromium.org/381283002/160001

[commit-bot: I haz the power, 2014-07-16 19:18:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-16 19:48:23]

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

[commit-bot: I haz the power, 2014-07-16 19:48:24]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[James Cook, 2014-07-16 20:40:36]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-07-16 20:42:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamescook@chromium.org/381283002/160001

[commit-bot: I haz the power, 2014-07-16 23:09:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-17 07:05:52]

Change committed as 283678

[James Cook, 2014-07-17 17:23:31]

On 2014/07/17 07:05:52, I haz the power (commit-bot) wrote:
> Change committed as 283678

Reverted in 283801.

This broke Chrome OS valgrind bots, for example:
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28v...