Make BackgroundContentsService start up BackgroundContents with a delay, as for ExtensionHosts.

Make BackgroundContentsService start up BackgroundContents with a delay, as for ExtensionHosts.

extensions::ProcessManager starts ExtensionHosts asynchronously with a queue to avoid using too much resources at startup; make BackgroundContentsService use the same queue implementation. (ExtensionHostQueue now accepts an interface that both BackgroundContentsService and ExtensionHost share.)

This significantly reduces the time BackgroundContentsService contributes to ExtensionService startup (75th percentile was ~300ms, according to Extensions.BackgroundContentsServiceStartupTime).

BUG=47236
Committed: https://crrev.com/d61dfe19ab3188101e511f633eeaaef3dc689e41
Cr-Commit-Position: refs/heads/master@{#317480}

[Yoyo Zhou, 2015-02-18 22:53:17]

yoz@chromium.org changed reviewers:
+ atwilson@chromium.org, rdevlin.cronin@chromium.org

[Yoyo Zhou, 2015-02-18 22:53:19]

https://codereview.chromium.org/933423003/diff/1/extensions/browser/delayed_s...
File extensions/browser/delayed_start_render_host.h (right):

https://codereview.chromium.org/933423003/diff/1/extensions/browser/delayed_s...
extensions/browser/delayed_start_render_host.h:16: class DelayedStartRenderHost
{
This is not my favoritest name - suggestions welcome.

[Devlin, 2015-02-18 23:22:12]

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
File chrome/browser/background/background_contents.cc (right):

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
chrome/browser/background/background_contents.cc:31: struct QueueWrapper {
Should we have two separate queues for extension hosts and background contents? 
Possibly more importantly, should they be of (potentially) different
implementations?  Or should we put this somewhere more central where it can be
shared between extension hosts and background contents?

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
File chrome/browser/background/background_contents.h (right):

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
chrome/browser/background/background_contents.h:66: void CreateRenderViewNow()
override;
We can actually make this private now (same for extension_host.h) and move the
"DO NOT USE" comment just to DelayedStartRenderHost.

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
File chrome/browser/background/background_contents_service.cc (right):

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
chrome/browser/background/background_contents_service.cc:658:
BackgroundContentsOpened(&details, profile);
Andrew will probably know better, but do we need to adjust these
(BackgroundContentsOpened(), sending the notifications) at all?  (They still
happened before the render was created, but there was a bit less of a gap -
particularly if we experiment with more than just a "post-task-at-end-of-queue
ExtensionHostQueue implementation.)

https://codereview.chromium.org/933423003/diff/1/extensions/browser/delayed_s...
File extensions/browser/delayed_start_render_host.h (right):

https://codereview.chromium.org/933423003/diff/1/extensions/browser/delayed_s...
extensions/browser/delayed_start_render_host.h:16: class DelayedStartRenderHost
{
On 2015/02/18 22:53:18, Yoyo Zhou (OOO...2015-02-16) wrote:
> This is not my favoritest name - suggestions welcome.

Even though the DelayedStart part of this is a pretty central detail of the
implementation, I think it actually is still an implementation detail here, and
is more suited to a comment on the ExtensionHostQueue (imagine, for instance, if
we had a queue that just immediately created everything).  So maybe just
ExtensionRenderViewCreator?  Not sure that's much better...

https://codereview.chromium.org/933423003/diff/1/extensions/browser/delayed_s...
extensions/browser/delayed_start_render_host.h:19: virtual void
CreateRenderViewNow() = 0;
virtual destructor?

[Yoyo Zhou, 2015-02-19 02:52:59]

yoz@chromium.org changed reviewers:
+ tengs@chromium.org

[Yoyo Zhou, 2015-02-19 02:55:38]

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
File chrome/browser/background/background_contents.cc (right):

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
chrome/browser/background/background_contents.cc:31: struct QueueWrapper {
On 2015/02/18 23:22:12, Devlin wrote:
> Should we have two separate queues for extension hosts and background
contents? 
> Possibly more importantly, should they be of (potentially) different
> implementations?  Or should we put this somewhere more central where it can be
> shared between extension hosts and background contents?

I don't have a strong preference either way. I think we could get a singleton
(lazy instance) SerialExtensionHostQueue, so we replace references to
g_queue_wrapper with SerialExtensionHostQueue::GetInstance. (An alternative
might be to make ExtensionHostQueue::GetInstance do the right thing, which puts
a dependency from the new extension_host_queue.cc to
serial_extension_host_queue.h.) What do you think?

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
File chrome/browser/background/background_contents.h (right):

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
chrome/browser/background/background_contents.h:66: void CreateRenderViewNow()
override;
On 2015/02/18 23:22:12, Devlin wrote:
> We can actually make this private now (same for extension_host.h) and move the
> "DO NOT USE" comment just to DelayedStartRenderHost.

Cool, I didn't know you could do that.

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
File chrome/browser/background/background_contents_service.cc (right):

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
chrome/browser/background/background_contents_service.cc:658:
BackgroundContentsOpened(&details, profile);
On 2015/02/18 23:22:12, Devlin wrote:
> Andrew will probably know better, but do we need to adjust these
> (BackgroundContentsOpened(), sending the notifications) at all?  (They still
> happened before the render was created, but there was a bit less of a gap -
> particularly if we experiment with more than just a "post-task-at-end-of-queue
> ExtensionHostQueue implementation.)

Adding tengs to comment, since DriveWebContentsManager is the only observer of
this notification.

https://codereview.chromium.org/933423003/diff/1/extensions/browser/delayed_s...
File extensions/browser/delayed_start_render_host.h (right):

https://codereview.chromium.org/933423003/diff/1/extensions/browser/delayed_s...
extensions/browser/delayed_start_render_host.h:16: class DelayedStartRenderHost
{
On 2015/02/18 23:22:12, Devlin wrote:
> On 2015/02/18 22:53:18, Yoyo Zhou (OOO...2015-02-16) wrote:
> > This is not my favoritest name - suggestions welcome.
> 
> Even though the DelayedStart part of this is a pretty central detail of the
> implementation, I think it actually is still an implementation detail here,
and
> is more suited to a comment on the ExtensionHostQueue (imagine, for instance,
if
> we had a queue that just immediately created everything).  So maybe just
> ExtensionRenderViewCreator?  Not sure that's much better...

I came up with the slightly tautological ExtensionQueueHost. Sadly, the queue is
still an implementation detail... I'll keep thinking about it.

https://codereview.chromium.org/933423003/diff/1/extensions/browser/delayed_s...
extensions/browser/delayed_start_render_host.h:19: virtual void
CreateRenderViewNow() = 0;
On 2015/02/18 23:22:12, Devlin wrote:
> virtual destructor?

Done.

[Devlin, 2015-02-19 16:41:43]

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
File chrome/browser/background/background_contents.cc (right):

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
chrome/browser/background/background_contents.cc:31: struct QueueWrapper {
On 2015/02/19 02:55:37, Yoyo Zhou (OOO...2015-02-16) wrote:
> On 2015/02/18 23:22:12, Devlin wrote:
> > Should we have two separate queues for extension hosts and background
> contents? 
> > Possibly more importantly, should they be of (potentially) different
> > implementations?  Or should we put this somewhere more central where it can
be
> > shared between extension hosts and background contents?
> 
> I don't have a strong preference either way. I think we could get a singleton
> (lazy instance) SerialExtensionHostQueue, so we replace references to
> g_queue_wrapper with SerialExtensionHostQueue::GetInstance. (An alternative
> might be to make ExtensionHostQueue::GetInstance do the right thing, which
puts
> a dependency from the new extension_host_queue.cc to
> serial_extension_host_queue.h.) What do you think?

I think Ben's trying to make it easy to switch between different implementations
of the queue, so I don't think making a lazy instance of
SerialExtensionHostQueue is the right thing to do, as it binds all the code that
uses the queue to a particular implementation.  I think that, for now at least,
I'd prefer ExtensionHostQueue::GetInstance to just Do The Right Thing -
depending on how big all the implementations turn out, we could even put them
all into extension_host_queue.cc in an anonymous namespace.  Even if we don't,
the dependency on each implementation is unfortunate, but probably better than
each client having a dependency on each implementation. ;)

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
File chrome/browser/background/background_contents.h (right):

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
chrome/browser/background/background_contents.h:66: void CreateRenderViewNow()
override;
On 2015/02/19 02:55:37, Yoyo Zhou (OOO...2015-02-16) wrote:
> On 2015/02/18 23:22:12, Devlin wrote:
> > We can actually make this private now (same for extension_host.h) and move
the
> > "DO NOT USE" comment just to DelayedStartRenderHost.
> 
> Cool, I didn't know you could do that.

Handy for these implementation-detail-type interfaces. :)

https://codereview.chromium.org/933423003/diff/1/extensions/browser/delayed_s...
File extensions/browser/delayed_start_render_host.h (right):

https://codereview.chromium.org/933423003/diff/1/extensions/browser/delayed_s...
extensions/browser/delayed_start_render_host.h:16: class DelayedStartRenderHost
{
On 2015/02/19 02:55:37, Yoyo Zhou (OOO...2015-02-16) wrote:
> On 2015/02/18 23:22:12, Devlin wrote:
> > On 2015/02/18 22:53:18, Yoyo Zhou (OOO...2015-02-16) wrote:
> > > This is not my favoritest name - suggestions welcome.
> > 
> > Even though the DelayedStart part of this is a pretty central detail of the
> > implementation, I think it actually is still an implementation detail here,
> and
> > is more suited to a comment on the ExtensionHostQueue (imagine, for
instance,
> if
> > we had a queue that just immediately created everything).  So maybe just
> > ExtensionRenderViewCreator?  Not sure that's much better...
> 
> I came up with the slightly tautological ExtensionQueueHost. Sadly, the queue
is
> still an implementation detail... I'll keep thinking about it.

Also, I think that having ExtensionQueueHost and ExtensionHostQueue is a little,
uh, much. I'll keep thinking, too.

[Devlin, 2015-02-19 16:42:29]

+Ben as FYI (since he's working on adding the different implementations of the
host queue)

[not at google - send to devlin, 2015-02-19 18:07:41]

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

[not at google - send to devlin, 2015-02-19 18:07:41]

This is awesome.

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
File chrome/browser/background/background_contents.cc (right):

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
chrome/browser/background/background_contents.cc:31: struct QueueWrapper {
On 2015/02/19 16:41:43, Devlin wrote:
> On 2015/02/19 02:55:37, Yoyo Zhou (OOO...2015-02-16) wrote:
> > On 2015/02/18 23:22:12, Devlin wrote:
> > > Should we have two separate queues for extension hosts and background
> > contents? 
> > > Possibly more importantly, should they be of (potentially) different
> > > implementations?  Or should we put this somewhere more central where it
can
> be
> > > shared between extension hosts and background contents?
> > 
> > I don't have a strong preference either way. I think we could get a
singleton
> > (lazy instance) SerialExtensionHostQueue, so we replace references to
> > g_queue_wrapper with SerialExtensionHostQueue::GetInstance. (An alternative
> > might be to make ExtensionHostQueue::GetInstance do the right thing, which
> puts
> > a dependency from the new extension_host_queue.cc to
> > serial_extension_host_queue.h.) What do you think?
> 
> I think Ben's trying to make it easy to switch between different
implementations
> of the queue, so I don't think making a lazy instance of
> SerialExtensionHostQueue is the right thing to do, as it binds all the code
that
> uses the queue to a particular implementation.  I think that, for now at
least,
> I'd prefer ExtensionHostQueue::GetInstance to just Do The Right Thing -
> depending on how big all the implementations turn out, we could even put them
> all into extension_host_queue.cc in an anonymous namespace.  Even if we don't,
> the dependency on each implementation is unfortunate, but probably better than
> each client having a dependency on each implementation. ;)

Note that I will probably need to modify the way the Singleton-isms work here
and instead delegate ExtensionHostQueue creation to chrome. I will want to
experiment with an ExtensionHostQueue implementation that delays until the first
tab has loaded, which means it needs to go in the //chrome layer.

As for the general question: ExtensionHostQueue::GetInstance() sounds
reasonable, if it's delegatable.

[not at google - send to devlin, 2015-02-19 19:51:07]

Btw in this case I believe they should share a single queue, but I don't think
that necessarily generalises.

[Yoyo Zhou, 2015-02-19 23:57:58]

I renamed to DeferredStartRenderHost. I think it's slightly better.

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
File chrome/browser/background/background_contents.cc (right):

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
chrome/browser/background/background_contents.cc:31: struct QueueWrapper {
On 2015/02/19 18:07:41, kalman wrote:
> On 2015/02/19 16:41:43, Devlin wrote:
> > On 2015/02/19 02:55:37, Yoyo Zhou (OOO...2015-02-16) wrote:
> > > On 2015/02/18 23:22:12, Devlin wrote:
> > > > Should we have two separate queues for extension hosts and background
> > > contents? 
> > > > Possibly more importantly, should they be of (potentially) different
> > > > implementations?  Or should we put this somewhere more central where it
> can
> > be
> > > > shared between extension hosts and background contents?
> > > 
> > > I don't have a strong preference either way. I think we could get a
> singleton
> > > (lazy instance) SerialExtensionHostQueue, so we replace references to
> > > g_queue_wrapper with SerialExtensionHostQueue::GetInstance. (An
alternative
> > > might be to make ExtensionHostQueue::GetInstance do the right thing, which
> > puts
> > > a dependency from the new extension_host_queue.cc to
> > > serial_extension_host_queue.h.) What do you think?
> > 
> > I think Ben's trying to make it easy to switch between different
> implementations
> > of the queue, so I don't think making a lazy instance of
> > SerialExtensionHostQueue is the right thing to do, as it binds all the code
> that
> > uses the queue to a particular implementation.  I think that, for now at
> least,
> > I'd prefer ExtensionHostQueue::GetInstance to just Do The Right Thing -
> > depending on how big all the implementations turn out, we could even put
them
> > all into extension_host_queue.cc in an anonymous namespace.  Even if we
don't,
> > the dependency on each implementation is unfortunate, but probably better
than
> > each client having a dependency on each implementation. ;)
> 
> Note that I will probably need to modify the way the Singleton-isms work here
> and instead delegate ExtensionHostQueue creation to chrome. I will want to
> experiment with an ExtensionHostQueue implementation that delays until the
first
> tab has loaded, which means it needs to go in the //chrome layer.
> 
> As for the general question: ExtensionHostQueue::GetInstance() sounds
> reasonable, if it's delegatable.

Turns out we have an ExtensionHostDelegate which works quite well for this
delegation. This makes it easy to share a queue for BackgroundContents and
ExtensionHosts, and if someone else wants their own queue, they can just make
one.

[Devlin, 2015-02-20 00:07:57]

lgtm, though still waiting to hear about notification orderings.

https://codereview.chromium.org/933423003/diff/40001/chrome/browser/backgroun...
File chrome/browser/background/background_contents.cc (right):

https://codereview.chromium.org/933423003/diff/40001/chrome/browser/backgroun...
chrome/browser/background/background_contents.cc:7: #include
"base/lazy_instance.h"
don't need

https://codereview.chromium.org/933423003/diff/40001/chrome/browser/backgroun...
chrome/browser/background/background_contents.cc:23: #include
"extensions/browser/serial_extension_host_queue.h"
don't need

[not at google - send to devlin, 2015-02-20 00:28:33]

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
File chrome/browser/background/background_contents.cc (right):

https://codereview.chromium.org/933423003/diff/1/chrome/browser/background/ba...
chrome/browser/background/background_contents.cc:31: struct QueueWrapper {
On 2015/02/19 23:57:58, Yoyo Zhou (OOO...2015-02-16) wrote:
> On 2015/02/19 18:07:41, kalman wrote:
> > On 2015/02/19 16:41:43, Devlin wrote:
> > > On 2015/02/19 02:55:37, Yoyo Zhou (OOO...2015-02-16) wrote:
> > > > On 2015/02/18 23:22:12, Devlin wrote:
> > > > > Should we have two separate queues for extension hosts and background
> > > > contents? 
> > > > > Possibly more importantly, should they be of (potentially) different
> > > > > implementations?  Or should we put this somewhere more central where
it
> > can
> > > be
> > > > > shared between extension hosts and background contents?
> > > > 
> > > > I don't have a strong preference either way. I think we could get a
> > singleton
> > > > (lazy instance) SerialExtensionHostQueue, so we replace references to
> > > > g_queue_wrapper with SerialExtensionHostQueue::GetInstance. (An
> alternative
> > > > might be to make ExtensionHostQueue::GetInstance do the right thing,
which
> > > puts
> > > > a dependency from the new extension_host_queue.cc to
> > > > serial_extension_host_queue.h.) What do you think?
> > > 
> > > I think Ben's trying to make it easy to switch between different
> > implementations
> > > of the queue, so I don't think making a lazy instance of
> > > SerialExtensionHostQueue is the right thing to do, as it binds all the
code
> > that
> > > uses the queue to a particular implementation.  I think that, for now at
> > least,
> > > I'd prefer ExtensionHostQueue::GetInstance to just Do The Right Thing -
> > > depending on how big all the implementations turn out, we could even put
> them
> > > all into extension_host_queue.cc in an anonymous namespace.  Even if we
> don't,
> > > the dependency on each implementation is unfortunate, but probably better
> than
> > > each client having a dependency on each implementation. ;)
> > 
> > Note that I will probably need to modify the way the Singleton-isms work
here
> > and instead delegate ExtensionHostQueue creation to chrome. I will want to
> > experiment with an ExtensionHostQueue implementation that delays until the
> first
> > tab has loaded, which means it needs to go in the //chrome layer.
> > 
> > As for the general question: ExtensionHostQueue::GetInstance() sounds
> > reasonable, if it's delegatable.
> 
> Turns out we have an ExtensionHostDelegate which works quite well for this
> delegation. This makes it easy to share a queue for BackgroundContents and
> ExtensionHosts, and if someone else wants their own queue, they can just make
> one.

Sweet, that's where I put it as well (in my unfinished, probably never to be
finished by the sounds of it, patch).

[Andrew T Wilson (Slow), 2015-02-20 11:08:57]

background/ LGTM - thanks so much for tackling this!

[Yoyo Zhou, 2015-02-20 19:30:47]

Tim: any thoughts on the changes to the NOTIFICATION_BACKGROUND_CONTENTS_OPENED
notification? With this change, the notification is sent some time (potentially,
numerous seconds) before the app's background page is actually opened.

[Yoyo Zhou, 2015-02-20 19:59:40]

New patchsets have been uploaded after l-g-t-m from
rdevlin.cronin@chromium.org,atwilson@chromium.org

[Yoyo Zhou, 2015-02-20 20:00:01]

https://codereview.chromium.org/933423003/diff/40001/chrome/browser/backgroun...
File chrome/browser/background/background_contents.cc (right):

https://codereview.chromium.org/933423003/diff/40001/chrome/browser/backgroun...
chrome/browser/background/background_contents.cc:7: #include
"base/lazy_instance.h"
On 2015/02/20 00:07:57, Devlin wrote:
> don't need

Done - also removed from extension_host.cc.

[Tim Song, 2015-02-21 00:59:07]

I don't perform any operations on the background page, and only use the
background page opening as a signal, so delaying the launch doesn't really
matter.

[Tim Song, 2015-02-21 00:59:09]

lgtm

I don't perform any operations on the background page, and only use the
background page opening as a signal, so delaying the launch doesn't really
matter.

[Yoyo Zhou, 2015-02-21 01:00:20]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2015-02-21 01:00:57]

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

[commit-bot: I haz the power, 2015-02-21 01:30:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-21 01:31:40]

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