Simplify ExtensionProcessManager to only load extension background pages when a browser window is o…

Simplify ExtensionProcessManager to only load extension background pages when a browser window is opened, and skip the other case of also watching for when extension system is initialized. This gets rid of a dependency on browser finder.

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

[jam, 2013-04-09 19:48:42]

Matt: I can't find a reason why we'd need to watch both
NOTIFICATION_EXTENSIONS_READY and NOTIFICATION_BROWSER_WINDOW_READY. The tests
seem to pass, but I'm not sure if there's a case I need to test that doesn't
have automated tests.

[Matt Perry, 2013-04-09 21:22:25]

I don't know about this. This will delay background pages until the browser
window first appears, which could be a significant time after the extension
service is ready.

If you're only trying to get rid of the call to
chrome::GetTotalBrowserCountForProfile(GetProfile()), it seems we could replace
it with a boolean which we set to true in response to
NOTIFICATION_BROWSER_WINDOW_READY.

[jam, 2013-04-09 22:54:42]

On 2013/04/09 21:22:25, Matt Perry wrote:
> I don't know about this. This will delay background pages until the browser
> window first appears, which could be a significant time after the extension
> service is ready.

in a debug build on Windows, this takes 1100 ms. I would imagine Release is
quite a bit faster.

Are you certain this causes any appreciable user-visible delay?

> 
> If you're only trying to get rid of the call to
> chrome::GetTotalBrowserCountForProfile(GetProfile()), it seems we could
replace
> it with a boolean which we set to true in response to
> NOTIFICATION_BROWSER_WINDOW_READY.

I'm not sure what you mean? NOTIFICATION_EXTENSIONS_READY is fired before
NOTIFICATION_BROWSER_WINDOW_READY, so if we were to wait for
NOTIFICATION_BROWSER_WINDOW_READY then that would be equivalent ot my patch.

[Matt Perry, 2013-04-09 23:22:46]

On 2013/04/09 22:54:42, jam wrote:
> On 2013/04/09 21:22:25, Matt Perry wrote:
> > I don't know about this. This will delay background pages until the browser
> > window first appears, which could be a significant time after the extension
> > service is ready.
> 
> in a debug build on Windows, this takes 1100 ms. I would imagine Release is
> quite a bit faster.
> 
> Are you certain this causes any appreciable user-visible delay?

I'd expect it to vary from setup to setup.

> > > 
> > If you're only trying to get rid of the call to
> > chrome::GetTotalBrowserCountForProfile(GetProfile()), it seems we could
> replace
> > it with a boolean which we set to true in response to
> > NOTIFICATION_BROWSER_WINDOW_READY.
> 
> I'm not sure what you mean? NOTIFICATION_EXTENSIONS_READY is fired before
> NOTIFICATION_BROWSER_WINDOW_READY, so if we were to wait for
> NOTIFICATION_BROWSER_WINDOW_READY then that would be equivalent ot my patch.

We have this check in DeDeferLoadingBackgroundHosts:
chrome::GetTotalBrowserCountForProfile(GetProfile()) == 0

We can replace that with a boolean browser_window_ready_ which is set to true in
NOTIFICATION_BROWSER_WINDOW_READY. Everything else stays the same as it was
before your patch.

[jam, 2013-04-09 23:30:31]

On 2013/04/09 23:22:46, Matt Perry wrote:
> On 2013/04/09 22:54:42, jam wrote:
> > On 2013/04/09 21:22:25, Matt Perry wrote:
> > > I don't know about this. This will delay background pages until the
browser
> > > window first appears, which could be a significant time after the
extension
> > > service is ready.
> > 
> > in a debug build on Windows, this takes 1100 ms. I would imagine Release is
> > quite a bit faster.
> > 
> > Are you certain this causes any appreciable user-visible delay?
> 
> I'd expect it to vary from setup to setup.

sure. But what I'm trying to get at is do you think the 1s delay in starting
background hosts would be visible? If the delay is slower for some people on
slower machines, this is the delay in showing the chrome browser window. it
seems that's much more series than background extension hosts starting. does it
really matter that background extension hosts start before the browser window
comes up?

> 
> > > > 
> > > If you're only trying to get rid of the call to
> > > chrome::GetTotalBrowserCountForProfile(GetProfile()), it seems we could
> > replace
> > > it with a boolean which we set to true in response to
> > > NOTIFICATION_BROWSER_WINDOW_READY.
> > 
> > I'm not sure what you mean? NOTIFICATION_EXTENSIONS_READY is fired before
> > NOTIFICATION_BROWSER_WINDOW_READY, so if we were to wait for
> > NOTIFICATION_BROWSER_WINDOW_READY then that would be equivalent ot my patch.
> 
> We have this check in DeDeferLoadingBackgroundHosts:
> chrome::GetTotalBrowserCountForProfile(GetProfile()) == 0
> 
> We can replace that with a boolean browser_window_ready_ which is set to true
in
> NOTIFICATION_BROWSER_WINDOW_READY. Everything else stays the same as it was
> before your patch.


Maybe I'm not following what you mean. If what you propose is done, then that is
effectively what this patch does. i.e. basically NOTIFICATION_EXTENSIONS_READY
does nothing, since we wait for NOTIFICATION_BROWSER_WINDOW_READY.
NOTIFICATION_EXTENSIONS_READY is fired before NOTIFICATION_BROWSER_WINDOW_READY,
so browser_window_ready_  would always be false.

[Matt Perry, 2013-04-09 23:36:14]

On 2013/04/09 23:30:31, jam wrote:
> On 2013/04/09 23:22:46, Matt Perry wrote:
> > On 2013/04/09 22:54:42, jam wrote:
> > > On 2013/04/09 21:22:25, Matt Perry wrote:
> > > > I don't know about this. This will delay background pages until the
> browser
> > > > window first appears, which could be a significant time after the
> extension
> > > > service is ready.
> > > 
> > > in a debug build on Windows, this takes 1100 ms. I would imagine Release
is
> > > quite a bit faster.
> > > 
> > > Are you certain this causes any appreciable user-visible delay?
> > 
> > I'd expect it to vary from setup to setup.
> 
> sure. But what I'm trying to get at is do you think the 1s delay in starting
> background hosts would be visible? If the delay is slower for some people on
> slower machines, this is the delay in showing the chrome browser window. it
> seems that's much more series than background extension hosts starting. does
it
> really matter that background extension hosts start before the browser window
> comes up?

I'd rather not delay it further. Some extensions want to do things early on
browser start. For the case of extensions that use webRequest, we also delay
network requests until the background page is loaded.

> 
> > 
> > > > > 
> > > > If you're only trying to get rid of the call to
> > > > chrome::GetTotalBrowserCountForProfile(GetProfile()), it seems we could
> > > replace
> > > > it with a boolean which we set to true in response to
> > > > NOTIFICATION_BROWSER_WINDOW_READY.
> > > 
> > > I'm not sure what you mean? NOTIFICATION_EXTENSIONS_READY is fired before
> > > NOTIFICATION_BROWSER_WINDOW_READY, so if we were to wait for
> > > NOTIFICATION_BROWSER_WINDOW_READY then that would be equivalent ot my
patch.
> > 
> > We have this check in DeDeferLoadingBackgroundHosts:
> > chrome::GetTotalBrowserCountForProfile(GetProfile()) == 0
> > 
> > We can replace that with a boolean browser_window_ready_ which is set to
true
> in
> > NOTIFICATION_BROWSER_WINDOW_READY. Everything else stays the same as it was
> > before your patch.
> 
> 
> Maybe I'm not following what you mean. If what you propose is done, then that
is
> effectively what this patch does. i.e. basically NOTIFICATION_EXTENSIONS_READY
> does nothing, since we wait for NOTIFICATION_BROWSER_WINDOW_READY.
> NOTIFICATION_EXTENSIONS_READY is fired before
NOTIFICATION_BROWSER_WINDOW_READY,
> so browser_window_ready_  would always be false.

NOTIFICATION_EXTENSIONS_READY does nothing only if DeferLoadingBackgroundHosts()
return true, which is conditional on 2 things, one of which is "are there any
browser windows open". NOTIFICATION_EXTENSIONS_READY is not guaranteed to always
fire before a browser window opens.

[jam, 2013-04-10 00:55:44]

On 2013/04/09 23:36:14, Matt Perry wrote:
> On 2013/04/09 23:30:31, jam wrote:
> > On 2013/04/09 23:22:46, Matt Perry wrote:
> > > On 2013/04/09 22:54:42, jam wrote:
> > > > On 2013/04/09 21:22:25, Matt Perry wrote:
> > > > > I don't know about this. This will delay background pages until the
> > browser
> > > > > window first appears, which could be a significant time after the
> > extension
> > > > > service is ready.
> > > > 
> > > > in a debug build on Windows, this takes 1100 ms. I would imagine Release
> is
> > > > quite a bit faster.
> > > > 
> > > > Are you certain this causes any appreciable user-visible delay?
> > > 
> > > I'd expect it to vary from setup to setup.
> > 
> > sure. But what I'm trying to get at is do you think the 1s delay in starting
> > background hosts would be visible? If the delay is slower for some people on
> > slower machines, this is the delay in showing the chrome browser window. it
> > seems that's much more series than background extension hosts starting. does
> it
> > really matter that background extension hosts start before the browser
window
> > comes up?
> 
> I'd rather not delay it further.

I understand. I'm not trying to slow down extensions code. I'm just wondering if
this change would affect it. In general I prefer to remove code unless I see
evidence that it's necessary.

It's hard for me to reason if it would slow things down or not if there are no
perf tests, as it seems there's no evidence that waiting till the browser window
opens would slow things down. No evidence to the contrary either, but that's
what I was hoping to find some way to figure out.

> Some extensions want to do things early on
> browser start. For the case of extensions that use webRequest, we also delay
> network requests until the background page is loaded.

Won't the requests not start until the browser window is opened anyways?


> 
> > 
> > > 
> > > > > > 
> > > > > If you're only trying to get rid of the call to
> > > > > chrome::GetTotalBrowserCountForProfile(GetProfile()), it seems we
could
> > > > replace
> > > > > it with a boolean which we set to true in response to
> > > > > NOTIFICATION_BROWSER_WINDOW_READY.
> > > > 
> > > > I'm not sure what you mean? NOTIFICATION_EXTENSIONS_READY is fired
before
> > > > NOTIFICATION_BROWSER_WINDOW_READY, so if we were to wait for
> > > > NOTIFICATION_BROWSER_WINDOW_READY then that would be equivalent ot my
> patch.
> > > 
> > > We have this check in DeDeferLoadingBackgroundHosts:
> > > chrome::GetTotalBrowserCountForProfile(GetProfile()) == 0
> > > 
> > > We can replace that with a boolean browser_window_ready_ which is set to
> true
> > in
> > > NOTIFICATION_BROWSER_WINDOW_READY. Everything else stays the same as it
was
> > > before your patch.
> > 
> > 
> > Maybe I'm not following what you mean. If what you propose is done, then
that
> is
> > effectively what this patch does. i.e. basically
NOTIFICATION_EXTENSIONS_READY
> > does nothing, since we wait for NOTIFICATION_BROWSER_WINDOW_READY.
> > NOTIFICATION_EXTENSIONS_READY is fired before
> NOTIFICATION_BROWSER_WINDOW_READY,
> > so browser_window_ready_  would always be false.
> 
> NOTIFICATION_EXTENSIONS_READY does nothing only if
DeferLoadingBackgroundHosts()
> return true, which is conditional on 2 things, one of which is "are there any
> browser windows open".

Ah, I understand now.

> NOTIFICATION_EXTENSIONS_READY is not guaranteed to always
> fire before a browser window opens.

really? the callstack I see for NOTIFICATION_EXTENSIONS_READY is on profile
creation. that must happen before a window is created. is there another path?

[jam, 2013-04-10 01:03:54]

btw I tested this on a release build, and it took 200ms between these two
notifications

[Matt Perry, 2013-04-10 01:09:56]

On 2013/04/10 00:55:44, jam wrote:
> On 2013/04/09 23:36:14, Matt Perry wrote:
> > On 2013/04/09 23:30:31, jam wrote:
> > > On 2013/04/09 23:22:46, Matt Perry wrote:
> > > > On 2013/04/09 22:54:42, jam wrote:
> > > > > On 2013/04/09 21:22:25, Matt Perry wrote:
> > > > > > I don't know about this. This will delay background pages until the
> > > browser
> > > > > > window first appears, which could be a significant time after the
> > > extension
> > > > > > service is ready.
> > > > > 
> > > > > in a debug build on Windows, this takes 1100 ms. I would imagine
Release
> > is
> > > > > quite a bit faster.
> > > > > 
> > > > > Are you certain this causes any appreciable user-visible delay?
> > > > 
> > > > I'd expect it to vary from setup to setup.
> > > 
> > > sure. But what I'm trying to get at is do you think the 1s delay in
starting
> > > background hosts would be visible? If the delay is slower for some people
on
> > > slower machines, this is the delay in showing the chrome browser window.
it
> > > seems that's much more series than background extension hosts starting.
does
> > it
> > > really matter that background extension hosts start before the browser
> window
> > > comes up?
> > 
> > I'd rather not delay it further.
> 
> I understand. I'm not trying to slow down extensions code. I'm just wondering
if
> this change would affect it. In general I prefer to remove code unless I see
> evidence that it's necessary.
> 
> It's hard for me to reason if it would slow things down or not if there are no
> perf tests, as it seems there's no evidence that waiting till the browser
window
> opens would slow things down. No evidence to the contrary either, but that's
> what I was hoping to find some way to figure out.
> 
> > Some extensions want to do things early on
> > browser start. For the case of extensions that use webRequest, we also delay
> > network requests until the background page is loaded.
> 
> Won't the requests not start until the browser window is opened anyways?

Yeah, but there's no guarantee the background pages will finish loading before
the browser window opens. (The requests are delayed until the bg page finishes
loading.)

> 
> 
> > 
> > > 
> > > > 
> > > > > > > 
> > > > > > If you're only trying to get rid of the call to
> > > > > > chrome::GetTotalBrowserCountForProfile(GetProfile()), it seems we
> could
> > > > > replace
> > > > > > it with a boolean which we set to true in response to
> > > > > > NOTIFICATION_BROWSER_WINDOW_READY.
> > > > > 
> > > > > I'm not sure what you mean? NOTIFICATION_EXTENSIONS_READY is fired
> before
> > > > > NOTIFICATION_BROWSER_WINDOW_READY, so if we were to wait for
> > > > > NOTIFICATION_BROWSER_WINDOW_READY then that would be equivalent ot my
> > patch.
> > > > 
> > > > We have this check in DeDeferLoadingBackgroundHosts:
> > > > chrome::GetTotalBrowserCountForProfile(GetProfile()) == 0
> > > > 
> > > > We can replace that with a boolean browser_window_ready_ which is set to
> > true
> > > in
> > > > NOTIFICATION_BROWSER_WINDOW_READY. Everything else stays the same as it
> was
> > > > before your patch.
> > > 
> > > 
> > > Maybe I'm not following what you mean. If what you propose is done, then
> that
> > is
> > > effectively what this patch does. i.e. basically
> NOTIFICATION_EXTENSIONS_READY
> > > does nothing, since we wait for NOTIFICATION_BROWSER_WINDOW_READY.
> > > NOTIFICATION_EXTENSIONS_READY is fired before
> > NOTIFICATION_BROWSER_WINDOW_READY,
> > > so browser_window_ready_  would always be false.
> > 
> > NOTIFICATION_EXTENSIONS_READY does nothing only if
> DeferLoadingBackgroundHosts()
> > return true, which is conditional on 2 things, one of which is "are there
any
> > browser windows open".
> 
> Ah, I understand now.
> 
> > NOTIFICATION_EXTENSIONS_READY is not guaranteed to always
> > fire before a browser window opens.
> 
> really? the callstack I see for NOTIFICATION_EXTENSIONS_READY is on profile
> creation. that must happen before a window is created. is there another path?

Oh OK, after rechecking that code, it looks like you're right. So maybe we can
just remove the check for 0 browser windows entirely.

[jam, 2013-04-10 01:28:11]

On 2013/04/10 01:09:56, Matt Perry wrote:
> > I understand. I'm not trying to slow down extensions code. I'm just
wondering
> if
> > this change would affect it. In general I prefer to remove code unless I see
> > evidence that it's necessary.
> > 
> > It's hard for me to reason if it would slow things down or not if there are
no
> > perf tests, as it seems there's no evidence that waiting till the browser
> window
> > opens would slow things down. No evidence to the contrary either, but that's
> > what I was hoping to find some way to figure out.
> > 
> > > Some extensions want to do things early on
> > > browser start. For the case of extensions that use webRequest, we also
delay
> > > network requests until the background page is loaded.
> > 
> > Won't the requests not start until the browser window is opened anyways?
> 
> Yeah, but there's no guarantee the background pages will finish loading before
> the browser window opens. (The requests are delayed until the bg page finishes
> loading.)

yes, but if the system is overloaded/slow that loading the extensions will be
slow enough to be user-perceptible, then wouldn't loading the first network
request be slow as well?

> > 
> > > NOTIFICATION_EXTENSIONS_READY is not guaranteed to always
> > > fire before a browser window opens.
> > 
> > really? the callstack I see for NOTIFICATION_EXTENSIONS_READY is on profile
> > creation. that must happen before a window is created. is there another
path?
> 
> Oh OK, after rechecking that code, it looks like you're right. So maybe we can
> just remove the check for 0 browser windows entirely.

[Matt Perry, 2013-04-10 19:31:45]

https://codereview.chromium.org/13811030/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://codereview.chromium.org/13811030/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_process_manager.cc:555: if (!service ||
!service->is_ready())
Can you move the service->is_ready() part to a CHECK?

[jam, 2013-04-10 19:33:38]

https://codereview.chromium.org/13811030/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://codereview.chromium.org/13811030/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_process_manager.cc:555: if (!service ||
!service->is_ready())
On 2013/04/10 19:31:45, Matt Perry wrote:
> Can you move the service->is_ready() part to a CHECK?

Done.

[Matt Perry, 2013-04-10 19:36:26]

lgtm