Lower the priority of shared workers

Lower the priority of shared workers that aren't associated with the foreground tab.

This should help out less powerful devices in the case where there's a shared worker in another tab and a cpu intensive page in the foreground.

BUG=
TEST=Open a doc, see webworker running, switch tabs and check webworker's priority by outputting contents of /sys/fs/cgroup/cpu/chrome_renderers/background/cgroup.proc on ChromeOS. With an open doc in a background tab, run 720p video (ie. youtube) at fullscreen, should stay fairly smooth.

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

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

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

[darin (slow to review), 2013-04-12 21:36:03]

Can you say more about the design goals?  I know the problem that spawned this
work, but I would think that we'd need to do more than just adjust thread
priorities.  Even low priority threads can do too much work, trigger too much
I/O, burn too much battery, etc.  It seems like we need to throttle the process'
MessageLoops (or something along those lines) to ensure that the rate of work is
kept in check.

[tonyg, 2013-04-16 15:45:13]

On 2013/04/12 21:36:03, darin wrote:
> Can you say more about the design goals?  I know the problem that spawned this
> work, but I would think that we'd need to do more than just adjust thread
> priorities.  Even low priority threads can do too much work, trigger too much
> I/O, burn too much battery, etc.  It seems like we need to throttle the
process'
> MessageLoops (or something along those lines) to ensure that the rate of work
is
> kept in check.

Hey Darin,

Simon and I were chatting about the design goal here. It seems to me like the
intended behavior is for a worker to run as fast as possible as long as it
doesn't interfere with the foreground tab. I don't think we'd want to always put
workers into a slow mode. This is distinct from background tabs which aren't
related to the foreground tab so they should be throttled way back in order to
not do useless work. I just don't think that the worker thread's work is useless
to the foreground tab. As far as I know, we haven't seen a case yet of a worker
thread that is continually doing work that we'd want to throttle even when there
is no foreground work.

What do you think about that story for worker priority? If you agree, then
thread priorities seem like the most elegant answer to the goal.

-Tony

[shatch, 2013-04-18 19:13:59]

On 2013/04/16 15:45:13, tonyg wrote:
> On 2013/04/12 21:36:03, darin wrote:
> > Can you say more about the design goals?  I know the problem that spawned
this
> > work, but I would think that we'd need to do more than just adjust thread
> > priorities.  Even low priority threads can do too much work, trigger too
much
> > I/O, burn too much battery, etc.  It seems like we need to throttle the
> process'
> > MessageLoops (or something along those lines) to ensure that the rate of
work
> is
> > kept in check.
> 
> Hey Darin,
> 
> Simon and I were chatting about the design goal here. It seems to me like the
> intended behavior is for a worker to run as fast as possible as long as it
> doesn't interfere with the foreground tab. I don't think we'd want to always
put
> workers into a slow mode. This is distinct from background tabs which aren't
> related to the foreground tab so they should be throttled way back in order to
> not do useless work. I just don't think that the worker thread's work is
useless
> to the foreground tab. As far as I know, we haven't seen a case yet of a
worker
> thread that is continually doing work that we'd want to throttle even when
there
> is no foreground work.
> 
> What do you think about that story for worker priority? If you agree, then
> thread priorities seem like the most elegant answer to the goal.
> 
> -Tony

Hi Darin,

I've gone ahead and tried throttling the worker's run loop, which works somewhat
but presents some problems. The main issue is choosing a value that works for a
given situation. For example, a value of 10 ms for the docs case still results
in a lot of video stutter, whereas 50 ms works (I didn't keep looking for values
between). Even in the case of a single platform, the value needs to be
dynamically chosen per website as well as taking into account how many other
shared webworkers are running (and what their dynamically chosen throttle values
are).

Perhaps the solution is to do both? Putting the shared workers into the
background will alleviate the problem of workers interfering with the foreground
tab. Having some hooks in with the ability to force a worker to sleep after
tasks would allow the browser a level of control over their rate of work and
allow it to scale back to preserve battery life.

What do you guys think?

[darin (slow to review), 2013-04-18 23:14:29]

I was thinking the throttle could be dynamic.  It could start out with a
delay of 0, and then increase based on the frequency of posted requests.
 It could similarly drop back down to 0 when there is inactivity.  This way
quick bursts of activity would be permitted and would run efficiently, but
longer strides of work would be throttled.  This way you could potentially
choose a larger delay.

I agree that the ideal solution probably involves a global view of all
background workers.  We could explore some solution like that, but I would
start with just the simpler (per worker) solution for now.

<over-engineering-alert>

Maybe a formula like this could be used to compute the task delay?

   t_delta = t_now - t_last_task
   if (t_delta >= K) {
     delay = 0
   } else {
     delay = D * (K - t_delta)
   }

K is the time period after which you'd stop delaying.  D is some constant
you can tune.

It's possible you might want to run the delay through a low pass filter to
ease into the change.

For some value of C < 1:

  delay_filtered = (1 - C) * delay_last + C * delay

If C is closer to 1, then delay_filtered will more quickly match the value
of delay.

</over-engineering-alert>

Perhaps a simpler step-function would work too :-)

-Darin


On Thu, Apr 18, 2013 at 12:13 PM, <simonhatch@chromium.org> wrote:

> On 2013/04/16 15:45:13, tonyg wrote:
>
>> On 2013/04/12 21:36:03, darin wrote:
>> > Can you say more about the design goals?  I know the problem that
>> spawned
>>
> this
>
>> > work, but I would think that we'd need to do more than just adjust
>> thread
>> > priorities.  Even low priority threads can do too much work, trigger too
>>
> much
>
>> > I/O, burn too much battery, etc.  It seems like we need to throttle the
>> process'
>> > MessageLoops (or something along those lines) to ensure that the rate of
>>
> work
>
>> is
>> > kept in check.
>>
>
>  Hey Darin,
>>
>
>  Simon and I were chatting about the design goal here. It seems to me like
>> the
>> intended behavior is for a worker to run as fast as possible as long as it
>> doesn't interfere with the foreground tab. I don't think we'd want to
>> always
>>
> put
>
>> workers into a slow mode. This is distinct from background tabs which
>> aren't
>> related to the foreground tab so they should be throttled way back in
>> order to
>> not do useless work. I just don't think that the worker thread's work is
>>
> useless
>
>> to the foreground tab. As far as I know, we haven't seen a case yet of a
>>
> worker
>
>> thread that is continually doing work that we'd want to throttle even when
>>
> there
>
>> is no foreground work.
>>
>
>  What do you think about that story for worker priority? If you agree, then
>> thread priorities seem like the most elegant answer to the goal.
>>
>
>  -Tony
>>
>
> Hi Darin,
>
> I've gone ahead and tried throttling the worker's run loop, which works
> somewhat
> but presents some problems. The main issue is choosing a value that works
> for a
> given situation. For example, a value of 10 ms for the docs case still
> results
> in a lot of video stutter, whereas 50 ms works (I didn't keep looking for
> values
> between). Even in the case of a single platform, the value needs to be
> dynamically chosen per website as well as taking into account how many
> other
> shared webworkers are running (and what their dynamically chosen throttle
> values
> are).
>
> Perhaps the solution is to do both? Putting the shared workers into the
> background will alleviate the problem of workers interfering with the
> foreground
> tab. Having some hooks in with the ability to force a worker to sleep after
> tasks would allow the browser a level of control over their rate of work
> and
> allow it to scale back to preserve battery life.
>
> What do you guys think?
>
>
>
https://codereview.chromium.**org/14137016/<https://codereview.chromium.org/1...
>

[shatch, 2013-04-19 21:54:15]

Hi Darin,

I gave this implementation a trial run as well, and it suffers from similar
issues, or at least my implementation did.

It still comes back to how does one choose K/D? I chose conservatively (100 ms)
at first, which resulted greatly reduced video stutter (as in the case above)
but excessively long sync times. I chose a bit more aggressively for another
run, which resulted in severe video stutter (though not at severe as no sleeps
at all). One might work around this by perhaps removing the sleeps when the
worker is associated with the foreground tab, although the implications of this
would be that it would take excessively long to sync unless you had the docs tab
open. You could also attempt to manage the sleep value using information about
the foreground tab's processor usage, but that might be very tricky to get
right.

This is only in the single worker case. Even if you were to come up with some
way of dynamically choosing K/D such that most applications worked ok, if
another tab has another shared worker running, you'd need a global view and some
sort of scheduler that could ensure that while one worker sleeps, the other
runs. You also have no idea how long each task will take.

Underlying all those issues though is the issue of sharing priority with the
foreground tab. The issues above aren't a matter of how quickly you scale
in/out, or how long you sleep. The fundamental issue is that, at some point, the
worker has to wake up and perform it's task. At this point the 2 processes will
share resources since they're of equal priority. In the netflix/youtube/hangouts
case, you'll get jank until the worker finishes.


What do you think of doing both, ie. backgrounding the workers as well as adding
these hooks to allow the worker to sleep after tasks.

Putting the workers in the background would make sure they're not sucking up
resources needed by the foreground tab (if it needs them, ie. video).

Adding hooks would give the browser the ability to request the worker sleep
after tasks. We could potentially use the battery state to request the worker
sleep periodically or just run whenever there are free resources.


Or am I missing something here?


On 2013/04/18 23:14:29, darin wrote:
> I was thinking the throttle could be dynamic.  It could start out with a
> delay of 0, and then increase based on the frequency of posted requests.
>  It could similarly drop back down to 0 when there is inactivity.  This way
> quick bursts of activity would be permitted and would run efficiently, but
> longer strides of work would be throttled.  This way you could potentially
> choose a larger delay.
> 
> I agree that the ideal solution probably involves a global view of all
> background workers.  We could explore some solution like that, but I would
> start with just the simpler (per worker) solution for now.
> 
> <over-engineering-alert>
> 
> Maybe a formula like this could be used to compute the task delay?
> 
>    t_delta = t_now - t_last_task
>    if (t_delta >= K) {
>      delay = 0
>    } else {
>      delay = D * (K - t_delta)
>    }
> 
> K is the time period after which you'd stop delaying.  D is some constant
> you can tune.
> 
> It's possible you might want to run the delay through a low pass filter to
> ease into the change.
> 
> For some value of C < 1:
> 
>   delay_filtered = (1 - C) * delay_last + C * delay
> 
> If C is closer to 1, then delay_filtered will more quickly match the value
> of delay.
> 
> </over-engineering-alert>
> 
> Perhaps a simpler step-function would work too :-)
> 
> -Darin
> 
> 
> On Thu, Apr 18, 2013 at 12:13 PM, <mailto:simonhatch@chromium.org> wrote:
> 
> > On 2013/04/16 15:45:13, tonyg wrote:
> >
> >> On 2013/04/12 21:36:03, darin wrote:
> >> > Can you say more about the design goals?  I know the problem that
> >> spawned
> >>
> > this
> >
> >> > work, but I would think that we'd need to do more than just adjust
> >> thread
> >> > priorities.  Even low priority threads can do too much work, trigger too
> >>
> > much
> >
> >> > I/O, burn too much battery, etc.  It seems like we need to throttle the
> >> process'
> >> > MessageLoops (or something along those lines) to ensure that the rate of
> >>
> > work
> >
> >> is
> >> > kept in check.
> >>
> >
> >  Hey Darin,
> >>
> >
> >  Simon and I were chatting about the design goal here. It seems to me like
> >> the
> >> intended behavior is for a worker to run as fast as possible as long as it
> >> doesn't interfere with the foreground tab. I don't think we'd want to
> >> always
> >>
> > put
> >
> >> workers into a slow mode. This is distinct from background tabs which
> >> aren't
> >> related to the foreground tab so they should be throttled way back in
> >> order to
> >> not do useless work. I just don't think that the worker thread's work is
> >>
> > useless
> >
> >> to the foreground tab. As far as I know, we haven't seen a case yet of a
> >>
> > worker
> >
> >> thread that is continually doing work that we'd want to throttle even when
> >>
> > there
> >
> >> is no foreground work.
> >>
> >
> >  What do you think about that story for worker priority? If you agree, then
> >> thread priorities seem like the most elegant answer to the goal.
> >>
> >
> >  -Tony
> >>
> >
> > Hi Darin,
> >
> > I've gone ahead and tried throttling the worker's run loop, which works
> > somewhat
> > but presents some problems. The main issue is choosing a value that works
> > for a
> > given situation. For example, a value of 10 ms for the docs case still
> > results
> > in a lot of video stutter, whereas 50 ms works (I didn't keep looking for
> > values
> > between). Even in the case of a single platform, the value needs to be
> > dynamically chosen per website as well as taking into account how many
> > other
> > shared webworkers are running (and what their dynamically chosen throttle
> > values
> > are).
> >
> > Perhaps the solution is to do both? Putting the shared workers into the
> > background will alleviate the problem of workers interfering with the
> > foreground
> > tab. Having some hooks in with the ability to force a worker to sleep after
> > tasks would allow the browser a level of control over their rate of work
> > and
> > allow it to scale back to preserve battery life.
> >
> > What do you guys think?
> >
> >
> >
>
https://codereview.chromium.**org/14137016/%3Chttps://codereview.chromium.org...>
> >

[shatch, 2013-04-26 01:47:09]

New snapshot uploaded with Darin's request for restoring priority on workers if
they're associated with foreground tab.

[DaveMoore, 2013-04-26 17:00:33]

I think this approach will work on chromeos, but a few things should be noted:
1) We try to have stronger split between foreground and background processes
than traditional linux by using cgroups. This is more like desktop Windows.
Linux typically attempts to be more "fair" to lower priority processes.
2) We currently do this by putting processes in cgroup directories named
"chrome_renderers/foreground" and "chrome_renderers/background" as only
renderers have their priorities set. With this change that naming will no longer
make sense. Changing it requires changes both to chromeos where we create the
cgroups and chrome where we move them. As at the lower level where the priority
is set we don't know the process type these names didn't really make sense
anyway.

[jam, 2013-04-29 06:04:54]

drive by: why do you need to modify chrome and add public content APIs? it
appears you're doing this to ask the embedder about which WebContents' are
visible and to remove the background pages.

isn't what you really want to know is if a webcontents is visible? content knows
this information already. i.e. RenderWidgetHostView::IsShowing(),
RenderWidgetHostImpl::is_hidden()

[darin (slow to review), 2013-04-29 15:49:13]

Yeah, I agree with jam@.  We should be able to implement this entirely within
content/.

[darin (slow to review), 2013-04-29 15:57:03]

On 2013/04/29 15:49:13, darin wrote:
> Yeah, I agree with jam@.  We should be able to implement this entirely within
> content/.

WorkerProcessHost has a WorkerDocumentSet, which tracks all of the associated
RenderViewHosts.  From that, you should be able to determine the
background-state of a shared worker.

I think jam@ also mentioned that there are plans to move NotificationService out
of content/.  In the case of this CL, that means developing a more direct way
for RenderViewHost visibility changes to be communicated to associated shared
workers.

[shatch, 2013-04-29 16:19:38]

On 2013/04/29 15:57:03, darin wrote:
> On 2013/04/29 15:49:13, darin wrote:
> > Yeah, I agree with jam@.  We should be able to implement this entirely
within
> > content/.
> 
> WorkerProcessHost has a WorkerDocumentSet, which tracks all of the associated
> RenderViewHosts.  From that, you should be able to determine the
> background-state of a shared worker.
> 
> I think jam@ also mentioned that there are plans to move NotificationService
out
> of content/.  In the case of this CL, that means developing a more direct way
> for RenderViewHost visibility changes to be communicated to associated shared
> workers.

jam: Believe I tried those first and they seemed to return visible for
background pages (or at least the case I'm looking at). Just tried it again
locally and the background page was getting placed into the visible list. Tried
RenderWidgetHostView::IsShowing() and RenderWidgetHostImpl::is_hidden()
individually, and together, without luck. Here's the quick change I made in case
you spot anything:

content::RenderWidgetHost* widget =
host->GetRenderWidgetHostByID(rit.GetCurrentKey());
content::RenderWidgetHostImpl* impl =
content::RenderWidgetHostImpl::From(widget);

// Tried this way
if (widget->GetView()->IsShowing()) {
  ...
}

// Then I switched it to this
if (!impl->is_hidden()) {
  ....
}

The code in the chrome_worker_service_delegate is based on how the TaskManager
seems to count background pages.


darin: Yeah I use the worker's WorkerDocumentSet and match that to the visible
list I generate.

[jam, 2013-04-29 19:55:17]

On 2013/04/29 16:19:38, shatch wrote:
> On 2013/04/29 15:57:03, darin wrote:
> > On 2013/04/29 15:49:13, darin wrote:
> > > Yeah, I agree with jam@.  We should be able to implement this entirely
> within
> > > content/.
> > 
> > WorkerProcessHost has a WorkerDocumentSet, which tracks all of the
associated
> > RenderViewHosts.  From that, you should be able to determine the
> > background-state of a shared worker.
> > 
> > I think jam@ also mentioned that there are plans to move NotificationService
> out
> > of content/.  In the case of this CL, that means developing a more direct
way
> > for RenderViewHost visibility changes to be communicated to associated
shared
> > workers.
> 
> jam: Believe I tried those first and they seemed to return visible for
> background pages (or at least the case I'm looking at).

When you say background pages, do you mean background tabs, or background pages
in extensions? if the latter, can you point me to an example extension that has
a background page?

> Just tried it again
> locally and the background page was getting placed into the visible list.
Tried
> RenderWidgetHostView::IsShowing() and RenderWidgetHostImpl::is_hidden()
> individually, and together, without luck. Here's the quick change I made in
case
> you spot anything:
> 
> content::RenderWidgetHost* widget =
> host->GetRenderWidgetHostByID(rit.GetCurrentKey());
> content::RenderWidgetHostImpl* impl =
> content::RenderWidgetHostImpl::From(widget);
> 
> // Tried this way
> if (widget->GetView()->IsShowing()) {
>   ...
> }
> 
> // Then I switched it to this
> if (!impl->is_hidden()) {
>   ....
> }
> 
> The code in the chrome_worker_service_delegate is based on how the TaskManager
> seems to count background pages.
> 
> 
> darin: Yeah I use the worker's WorkerDocumentSet and match that to the visible
> list I generate.

[shatch, 2013-04-29 20:03:53]

On 2013/04/29 19:55:17, jam wrote:
> On 2013/04/29 16:19:38, shatch wrote:
> > On 2013/04/29 15:57:03, darin wrote:
> > > On 2013/04/29 15:49:13, darin wrote:
> > > > Yeah, I agree with jam@.  We should be able to implement this entirely
> > within
> > > > content/.
> > > 
> > > WorkerProcessHost has a WorkerDocumentSet, which tracks all of the
> associated
> > > RenderViewHosts.  From that, you should be able to determine the
> > > background-state of a shared worker.
> > > 
> > > I think jam@ also mentioned that there are plans to move
NotificationService
> > out
> > > of content/.  In the case of this CL, that means developing a more direct
> way
> > > for RenderViewHost visibility changes to be communicated to associated
> shared
> > > workers.
> > 
> > jam: Believe I tried those first and they seemed to return visible for
> > background pages (or at least the case I'm looking at).
> 
> When you say background pages, do you mean background tabs, or background
pages
> in extensions? if the latter, can you point me to an example extension that
has
> a background page?
> 

I'm looking at the docs web worker, so go to docs and enable offline mode.
According to task manager, there's "Background Page: Google Drive".


> > Just tried it again
> > locally and the background page was getting placed into the visible list.
> Tried
> > RenderWidgetHostView::IsShowing() and RenderWidgetHostImpl::is_hidden()
> > individually, and together, without luck. Here's the quick change I made in
> case
> > you spot anything:
> > 
> > content::RenderWidgetHost* widget =
> > host->GetRenderWidgetHostByID(rit.GetCurrentKey());
> > content::RenderWidgetHostImpl* impl =
> > content::RenderWidgetHostImpl::From(widget);
> > 
> > // Tried this way
> > if (widget->GetView()->IsShowing()) {
> >   ...
> > }
> > 
> > // Then I switched it to this
> > if (!impl->is_hidden()) {
> >   ....
> > }
> > 
> > The code in the chrome_worker_service_delegate is based on how the
TaskManager
> > seems to count background pages.
> > 
> > 
> > darin: Yeah I use the worker's WorkerDocumentSet and match that to the
visible
> > list I generate.

[jam, 2013-04-29 20:40:53]

On 2013/04/29 20:03:53, shatch wrote:
> On 2013/04/29 19:55:17, jam wrote:
> > On 2013/04/29 16:19:38, shatch wrote:
> > > On 2013/04/29 15:57:03, darin wrote:
> > > > On 2013/04/29 15:49:13, darin wrote:
> > > > > Yeah, I agree with jam@.  We should be able to implement this entirely
> > > within
> > > > > content/.
> > > > 
> > > > WorkerProcessHost has a WorkerDocumentSet, which tracks all of the
> > associated
> > > > RenderViewHosts.  From that, you should be able to determine the
> > > > background-state of a shared worker.
> > > > 
> > > > I think jam@ also mentioned that there are plans to move
> NotificationService
> > > out
> > > > of content/.  In the case of this CL, that means developing a more
direct
> > way
> > > > for RenderViewHost visibility changes to be communicated to associated
> > shared
> > > > workers.
> > > 
> > > jam: Believe I tried those first and they seemed to return visible for
> > > background pages (or at least the case I'm looking at).
> > 
> > When you say background pages, do you mean background tabs, or background
> pages
> > in extensions? if the latter, can you point me to an example extension that
> has
> > a background page?
> > 
> 
> I'm looking at the docs web worker, so go to docs and enable offline mode.
> According to task manager, there's "Background Page: Google Drive".

how do I do this? I don't see the option to enable this in docs (signed in with
my chromium account), or when I'm signed in to my personal gmail. I tried to
follow http://support.google.com/drive/bin/answer.py?hl=en&answer=2375012
> 
> 
> > > Just tried it again
> > > locally and the background page was getting placed into the visible list.
> > Tried
> > > RenderWidgetHostView::IsShowing() and RenderWidgetHostImpl::is_hidden()
> > > individually, and together, without luck. Here's the quick change I made
in
> > case
> > > you spot anything:
> > > 
> > > content::RenderWidgetHost* widget =
> > > host->GetRenderWidgetHostByID(rit.GetCurrentKey());
> > > content::RenderWidgetHostImpl* impl =
> > > content::RenderWidgetHostImpl::From(widget);
> > > 
> > > // Tried this way
> > > if (widget->GetView()->IsShowing()) {
> > >   ...
> > > }
> > > 
> > > // Then I switched it to this
> > > if (!impl->is_hidden()) {
> > >   ....
> > > }
> > > 
> > > The code in the chrome_worker_service_delegate is based on how the
> TaskManager
> > > seems to count background pages.
> > > 
> > > 
> > > darin: Yeah I use the worker's WorkerDocumentSet and match that to the
> visible
> > > list I generate.

[shatch, 2013-04-30 17:34:01]

On 2013/04/29 20:40:53, jam wrote:
> On 2013/04/29 20:03:53, shatch wrote:
> > On 2013/04/29 19:55:17, jam wrote:
> > > On 2013/04/29 16:19:38, shatch wrote:
> > > > On 2013/04/29 15:57:03, darin wrote:
> > > > > On 2013/04/29 15:49:13, darin wrote:
> > > > > > Yeah, I agree with jam@.  We should be able to implement this
entirely
> > > > within
> > > > > > content/.
> > > > > 
> > > > > WorkerProcessHost has a WorkerDocumentSet, which tracks all of the
> > > associated
> > > > > RenderViewHosts.  From that, you should be able to determine the
> > > > > background-state of a shared worker.
> > > > > 
> > > > > I think jam@ also mentioned that there are plans to move
> > NotificationService
> > > > out
> > > > > of content/.  In the case of this CL, that means developing a more
> direct
> > > way
> > > > > for RenderViewHost visibility changes to be communicated to associated
> > > shared
> > > > > workers.
> > > > 
> > > > jam: Believe I tried those first and they seemed to return visible for
> > > > background pages (or at least the case I'm looking at).
> > > 
> > > When you say background pages, do you mean background tabs, or background
> > pages
> > > in extensions? if the latter, can you point me to an example extension
that
> > has
> > > a background page?
> > > 
> > 
> > I'm looking at the docs web worker, so go to docs and enable offline mode.
> > According to task manager, there's "Background Page: Google Drive".
> 
> how do I do this? I don't see the option to enable this in docs (signed in
with
> my chromium account), or when I'm signed in to my personal gmail. I tried to
> follow http://support.google.com/drive/bin/answer.py?hl=en&answer=2375012
> > 

Hmm I'm not sure how to help, I used my private gmail and followed the
instructions without any trouble.

> > 
> > > > Just tried it again
> > > > locally and the background page was getting placed into the visible
list.
> > > Tried
> > > > RenderWidgetHostView::IsShowing() and RenderWidgetHostImpl::is_hidden()
> > > > individually, and together, without luck. Here's the quick change I made
> in
> > > case
> > > > you spot anything:
> > > > 
> > > > content::RenderWidgetHost* widget =
> > > > host->GetRenderWidgetHostByID(rit.GetCurrentKey());
> > > > content::RenderWidgetHostImpl* impl =
> > > > content::RenderWidgetHostImpl::From(widget);
> > > > 
> > > > // Tried this way
> > > > if (widget->GetView()->IsShowing()) {
> > > >   ...
> > > > }
> > > > 
> > > > // Then I switched it to this
> > > > if (!impl->is_hidden()) {
> > > >   ....
> > > > }
> > > > 
> > > > The code in the chrome_worker_service_delegate is based on how the
> > TaskManager
> > > > seems to count background pages.
> > > > 
> > > > 
> > > > darin: Yeah I use the worker's WorkerDocumentSet and match that to the
> > visible
> > > > list I generate.

[darin (slow to review), 2013-05-01 05:37:09]

I think you need to install the Google Docs App:
https://chrome.google.com/webstore/detail/google-docs/aohghmighlieiainnegkcij...


On Mon, Apr 29, 2013 at 1:40 PM, <jam@chromium.org> wrote:

> On 2013/04/29 20:03:53, shatch wrote:
>
>> On 2013/04/29 19:55:17, jam wrote:
>> > On 2013/04/29 16:19:38, shatch wrote:
>> > > On 2013/04/29 15:57:03, darin wrote:
>> > > > On 2013/04/29 15:49:13, darin wrote:
>> > > > > Yeah, I agree with jam@.  We should be able to implement this
>> entirely
>> > > within
>> > > > > content/.
>> > > >
>> > > > WorkerProcessHost has a WorkerDocumentSet, which tracks all of the
>> > associated
>> > > > RenderViewHosts.  From that, you should be able to determine the
>> > > > background-state of a shared worker.
>> > > >
>> > > > I think jam@ also mentioned that there are plans to move
>> NotificationService
>> > > out
>> > > > of content/.  In the case of this CL, that means developing a more
>>
> direct
>
>> > way
>> > > > for RenderViewHost visibility changes to be communicated to
>> associated
>> > shared
>> > > > workers.
>> > >
>> > > jam: Believe I tried those first and they seemed to return visible for
>> > > background pages (or at least the case I'm looking at).
>> >
>> > When you say background pages, do you mean background tabs, or
>> background
>> pages
>> > in extensions? if the latter, can you point me to an example extension
>> that
>> has
>> > a background page?
>> >
>>
>
>  I'm looking at the docs web worker, so go to docs and enable offline mode.
>> According to task manager, there's "Background Page: Google Drive".
>>
>
> how do I do this? I don't see the option to enable this in docs (signed in
> with
> my chromium account), or when I'm signed in to my personal gmail. I tried
> to
> follow http://support.google.com/**drive/bin/answer.py?hl=en&**
>
answer=2375012<http://support.google.com/drive/bin/answer.py?hl=en&answer=2375012>
>
>
>
>  > > Just tried it again
>> > > locally and the background page was getting placed into the visible
>> list.
>> > Tried
>> > > RenderWidgetHostView::**IsShowing() and RenderWidgetHostImpl::is_**
>> hidden()
>> > > individually, and together, without luck. Here's the quick change I
>> made
>>
> in
>
>> > case
>> > > you spot anything:
>> > >
>> > > content::RenderWidgetHost* widget =
>> > > host->GetRenderWidgetHostByID(**rit.GetCurrentKey());
>> > > content::RenderWidgetHostImpl* impl =
>> > > content::RenderWidgetHostImpl:**:From(widget);
>> > >
>> > > // Tried this way
>> > > if (widget->GetView()->IsShowing(**)) {
>> > >   ...
>> > > }
>> > >
>> > > // Then I switched it to this
>> > > if (!impl->is_hidden()) {
>> > >   ....
>> > > }
>> > >
>> > > The code in the chrome_worker_service_delegate is based on how the
>> TaskManager
>> > > seems to count background pages.
>> > >
>> > >
>> > > darin: Yeah I use the worker's WorkerDocumentSet and match that to the
>> visible
>> > > list I generate.
>>
>
>
>
https://codereview.chromium.**org/14137016/<https://codereview.chromium.org/1...
>

[jam, 2013-05-01 19:33:10]

ah, it worked with the google drive app and the google docs app (it might have
only required the first, not sure since i installed the second first) AND only
on my gmail account and not on my chromium.

after looking at the code, it seems that RenderWidgetHostImpl::is_hidden() won't
do what we want since it gets set based on navigation and not actual window
visibility. But at least on Windows, RenderWidgetHostView::IsShowing() is
correct in that background pages return false. Which platform were you testing
on? It seems that perhaps the bug is there. If you're on ChromeOS, that would be
RenderWidgetHostViewAura so that could just be a bug in aura.

On 2013/05/01 05:37:09, darin wrote:
> I think you need to install the Google Docs App:
>
https://chrome.google.com/webstore/detail/google-docs/aohghmighlieiainnegkcij...

[shatch, 2013-05-02 16:44:14]

On 2013/05/01 19:33:10, jam wrote:
> ah, it worked with the google drive app and the google docs app (it might have
> only required the first, not sure since i installed the second first) AND only
> on my gmail account and not on my chromium.
> 
> after looking at the code, it seems that RenderWidgetHostImpl::is_hidden()
won't
> do what we want since it gets set based on navigation and not actual window
> visibility. But at least on Windows, RenderWidgetHostView::IsShowing() is
> correct in that background pages return false. Which platform were you testing
> on? It seems that perhaps the bug is there. If you're on ChromeOS, that would
be
> RenderWidgetHostViewAura so that could just be a bug in aura.
> 
> On 2013/05/01 05:37:09, darin wrote:
> > I think you need to install the Google Docs App:
> >
>
https://chrome.google.com/webstore/detail/google-docs/aohghmighlieiainnegkcij...

I was testing your suggestions on my linux desktop.

[shatch, 2013-05-03 21:18:08]

RenderWidgetHostView::IsShowing() seems to work fine on ChromeOS, so I moved the
logic back into worker_service_impl.cc and removed all changes to /chrome. Maybe
I should log a bug for linux?

[shatch, 2013-05-06 19:56:49]

ping

So is there anything else needed for this? Jam gave a pass on using the
notification service.

[sky, 2013-05-06 21:10:28]

When you ping on a review please indicate who you are waiting on.

On Mon, May 6, 2013 at 12:56 PM,  <simonhatch@chromium.org> wrote:
> ping
>
> So is there anything else needed for this? Jam gave a pass on using the
> notification service.
>
> https://codereview.chromium.org/14137016/

[shatch, 2013-05-07 22:22:56]

Sorry, Darin could you please take a look. Thanks.

On 2013/05/06 21:10:28, sky wrote:
> When you ping on a review please indicate who you are waiting on.
> 
> On Mon, May 6, 2013 at 12:56 PM,  <mailto:simonhatch@chromium.org> wrote:
> > ping
> >
> > So is there anything else needed for this? Jam gave a pass on using the
> > notification service.
> >
> > https://codereview.chromium.org/14137016/

[shatch, 2013-05-09 15:49:52]

ping:darin

On 2013/05/07 22:22:56, shatch wrote:
> Sorry, Darin could you please take a look. Thanks.
> 
> On 2013/05/06 21:10:28, sky wrote:
> > When you ping on a review please indicate who you are waiting on.
> > 
> > On Mon, May 6, 2013 at 12:56 PM,  <mailto:simonhatch@chromium.org> wrote:
> > > ping
> > >
> > > So is there anything else needed for this? Jam gave a pass on using the
> > > notification service.
> > >
> > > https://codereview.chromium.org/14137016/

[jam, 2013-05-09 15:53:54]

On 2013/05/03 21:18:08, shatch wrote:
> RenderWidgetHostView::IsShowing() seems to work fine on ChromeOS, so I moved
the
> logic back into worker_service_impl.cc and removed all changes to /chrome.
Maybe
> I should log a bug for linux?

you can. i assume this is the GTK implementation, which will go away in the near
term when we switch to aura on desktop linux.

[jam, 2013-05-09 18:02:57]

(darin is busy, so I can review on his behalf)

https://codereview.chromium.org/14137016/diff/43001/content/browser/browser_c...
File content/browser/browser_child_process_host_impl.h (right):

https://codereview.chromium.org/14137016/diff/43001/content/browser/browser_c...
content/browser/browser_child_process_host_impl.h:64: void SetBackgrounded(bool
backgrounded);
nit: please document

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
File content/browser/worker_host/worker_process_host.h (right):

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
content/browser/worker_host/worker_process_host.h:173: bool IsProcessLaunched()
const;
nit: this is really just a getter, so give it a unix_hacker name and inline it
(like the above getters)

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
File content/browser/worker_host/worker_service_impl.cc (right):

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.cc:546: void
WorkerServiceImpl::RegisterObserver() {
this class is currently completely on the IO thread. adding members and methods
that are used on the UI thread will make it harder to read and possibly
introduce bugs. two things that jump at me off the top of my head:
-now registrar_'s destructor would run on the IO thread, which would
assert/crash since it registered on a different thread
-the base::bind that uses Unretained in the constructor might crash in unittests
which create this object and delete it before the task run. with
https://codereview.chromium.org/8947021/ which will land soon, singletons will
be deleted between each unit test 

how about having an inner ref counted class that does the work on the UI thread,
and notifies the WorkerServiceImpl object on the UI thread?

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.cc:547: registrar_.Add(this,
content::NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED,
nit: likewise, here and below remove the "content::"

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.cc:651: int id =
content::Source<content::RenderWidgetHost>(source).ptr()->
nit: naming wise, convention is
render_view_id
render_process_id
please use this convention because it makes it clear what the IDs are referring
to
(also in other places in this file)

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
File content/browser/worker_host/worker_service_impl.h (right):

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.h:28: public
content::NotificationObserver {
no "content::" since this is in the content namespace

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.h:33: 
nit: remove

[shatch, 2013-05-09 22:42:49]

New snapshot uploaded.

https://codereview.chromium.org/14137016/diff/43001/content/browser/browser_c...
File content/browser/browser_child_process_host_impl.h (right):

https://codereview.chromium.org/14137016/diff/43001/content/browser/browser_c...
content/browser/browser_child_process_host_impl.h:64: void SetBackgrounded(bool
backgrounded);
On 2013/05/09 18:02:57, jam wrote:
> nit: please document

Done.

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
File content/browser/worker_host/worker_process_host.h (right):

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
content/browser/worker_host/worker_process_host.h:173: bool IsProcessLaunched()
const;
On 2013/05/09 18:02:57, jam wrote:
> nit: this is really just a getter, so give it a unix_hacker name and inline it
> (like the above getters)

Done.

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
File content/browser/worker_host/worker_service_impl.cc (right):

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.cc:546: void
WorkerServiceImpl::RegisterObserver() {
On 2013/05/09 18:02:57, jam wrote:
> this class is currently completely on the IO thread. adding members and
methods
> that are used on the UI thread will make it harder to read and possibly
> introduce bugs. two things that jump at me off the top of my head:
> -now registrar_'s destructor would run on the IO thread, which would
> assert/crash since it registered on a different thread
> -the base::bind that uses Unretained in the constructor might crash in
unittests
> which create this object and delete it before the task run. with
> https://codereview.chromium.org/8947021/ which will land soon, singletons will
> be deleted between each unit test 
> 
> how about having an inner ref counted class that does the work on the UI
thread,
> and notifies the WorkerServiceImpl object on the UI thread?

Done.

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.cc:547: registrar_.Add(this,
content::NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED,
On 2013/05/09 18:02:57, jam wrote:
> nit: likewise, here and below remove the "content::"

Done.

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.cc:651: int id =
content::Source<content::RenderWidgetHost>(source).ptr()->
On 2013/05/09 18:02:57, jam wrote:
> nit: naming wise, convention is
> render_view_id
> render_process_id
> please use this convention because it makes it clear what the IDs are
referring
> to
> (also in other places in this file)

Done.

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
File content/browser/worker_host/worker_service_impl.h (right):

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.h:28: public
content::NotificationObserver {
On 2013/05/09 18:02:57, jam wrote:
> no "content::" since this is in the content namespace

Done.

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.h:33: 
On 2013/05/09 18:02:57, jam wrote:
> nit: remove

Done.

[shatch, 2013-05-10 23:08:26]

ping:jam

On 2013/05/09 22:42:49, shatch wrote:
> New snapshot uploaded.
> 
>
https://codereview.chromium.org/14137016/diff/43001/content/browser/browser_c...
> File content/browser/browser_child_process_host_impl.h (right):
> 
>
https://codereview.chromium.org/14137016/diff/43001/content/browser/browser_c...
> content/browser/browser_child_process_host_impl.h:64: void
SetBackgrounded(bool
> backgrounded);
> On 2013/05/09 18:02:57, jam wrote:
> > nit: please document
> 
> Done.
> 
>
https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
> File content/browser/worker_host/worker_process_host.h (right):
> 
>
https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
> content/browser/worker_host/worker_process_host.h:173: bool
IsProcessLaunched()
> const;
> On 2013/05/09 18:02:57, jam wrote:
> > nit: this is really just a getter, so give it a unix_hacker name and inline
it
> > (like the above getters)
> 
> Done.
> 
>
https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
> File content/browser/worker_host/worker_service_impl.cc (right):
> 
>
https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
> content/browser/worker_host/worker_service_impl.cc:546: void
> WorkerServiceImpl::RegisterObserver() {
> On 2013/05/09 18:02:57, jam wrote:
> > this class is currently completely on the IO thread. adding members and
> methods
> > that are used on the UI thread will make it harder to read and possibly
> > introduce bugs. two things that jump at me off the top of my head:
> > -now registrar_'s destructor would run on the IO thread, which would
> > assert/crash since it registered on a different thread
> > -the base::bind that uses Unretained in the constructor might crash in
> unittests
> > which create this object and delete it before the task run. with
> > https://codereview.chromium.org/8947021/ which will land soon, singletons
will
> > be deleted between each unit test 
> > 
> > how about having an inner ref counted class that does the work on the UI
> thread,
> > and notifies the WorkerServiceImpl object on the UI thread?
> 
> Done.
> 
>
https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
> content/browser/worker_host/worker_service_impl.cc:547: registrar_.Add(this,
> content::NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED,
> On 2013/05/09 18:02:57, jam wrote:
> > nit: likewise, here and below remove the "content::"
> 
> Done.
> 
>
https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
> content/browser/worker_host/worker_service_impl.cc:651: int id =
> content::Source<content::RenderWidgetHost>(source).ptr()->
> On 2013/05/09 18:02:57, jam wrote:
> > nit: naming wise, convention is
> > render_view_id
> > render_process_id
> > please use this convention because it makes it clear what the IDs are
> referring
> > to
> > (also in other places in this file)
> 
> Done.
> 
>
https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
> File content/browser/worker_host/worker_service_impl.h (right):
> 
>
https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
> content/browser/worker_host/worker_service_impl.h:28: public
> content::NotificationObserver {
> On 2013/05/09 18:02:57, jam wrote:
> > no "content::" since this is in the content namespace
> 
> Done.
> 
>
https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
> content/browser/worker_host/worker_service_impl.h:33: 
> On 2013/05/09 18:02:57, jam wrote:
> > nit: remove
> 
> Done.

[darin (slow to review), 2013-05-10 23:42:13]

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
File content/browser/worker_host/worker_service_impl.cc (right):

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.cc:609:
content::RenderProcessHost::AllHostsIterator();
nit: like function calls, we usually wrap the line starting the column after the
"("... which is a 5 space indent in the case of for loops.

[darin (slow to review), 2013-05-10 23:43:07]

I really like the WorkerPrioritySetter refactoring.  LGTM.

[darin (slow to review), 2013-05-11 06:02:18]

https://codereview.chromium.org/14137016/diff/51003/content/browser/worker_ho...
File content/browser/worker_host/worker_service_impl.cc (right):

https://codereview.chromium.org/14137016/diff/51003/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.cc:38: public
base::RefCountedThreadSafe<WorkerPrioritySetter> {
should you indicate a thread on which this object should be deleted?  is it safe
to delete a NotificationRegistrar on any thread?  i thought that had to die on
the UI thread.

https://codereview.chromium.org/14137016/diff/51003/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.cc:132:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
nit: How about adding more DCHECKs like these to other methods?

[shatch, 2013-05-13 17:47:47]

New snapshot uploaded.

darin: ptal

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
File content/browser/worker_host/worker_service_impl.cc (right):

https://codereview.chromium.org/14137016/diff/43001/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.cc:609:
content::RenderProcessHost::AllHostsIterator();
On 2013/05/10 23:42:13, darin wrote:
> nit: like function calls, we usually wrap the line starting the column after
the
> "("... which is a 5 space indent in the case of for loops.

Done.

https://codereview.chromium.org/14137016/diff/51003/content/browser/worker_ho...
File content/browser/worker_host/worker_service_impl.cc (right):

https://codereview.chromium.org/14137016/diff/51003/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.cc:38: public
base::RefCountedThreadSafe<WorkerPrioritySetter> {
On 2013/05/11 06:02:19, darin wrote:
> should you indicate a thread on which this object should be deleted?  is it
safe
> to delete a NotificationRegistrar on any thread?  i thought that had to die on
> the UI thread.

Done.

https://codereview.chromium.org/14137016/diff/51003/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.cc:132:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2013/05/11 06:02:19, darin wrote:
> nit: How about adding more DCHECKs like these to other methods?

Done.

[darin (slow to review), 2013-05-13 18:36:16]

LGTM

[commit-bot: I haz the power, 2013-05-13 19:19:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonhatch@chromium.org/14137016/88001

[commit-bot: I haz the power, 2013-05-13 22:46:26]

Change committed as 199840

[shatch, 2013-05-14 16:01:25]

New snapshot uploaded. PTAL


This failed a DCHECK(!in_dtor_) on some dbg bots. I think what happened is the
call to assign to the scoped_refptr in the constructor of WorkerServiceImpl
fails if PostTask completes too quickly in the constructor of
WorkerPrioritySetter. Managed to get the same failed DCHECK by inserting a small
sleep into the constructor after the PostTask call. Moved the call to register
the observer into a separate function to ensure the scoped_refptr can acquire.

[shatch, 2013-05-15 17:55:41]

ping:darin

On 2013/05/14 16:01:25, shatch wrote:
> New snapshot uploaded. PTAL
> 
> 
> This failed a DCHECK(!in_dtor_) on some dbg bots. I think what happened is the
> call to assign to the scoped_refptr in the constructor of WorkerServiceImpl
> fails if PostTask completes too quickly in the constructor of
> WorkerPrioritySetter. Managed to get the same failed DCHECK by inserting a
small
> sleep into the constructor after the PostTask call. Moved the call to register
> the observer into a separate function to ensure the scoped_refptr can acquire.

[shatch, 2013-05-16 21:35:36]

ping:darin

On 2013/05/15 17:55:41, shatch wrote:
> ping:darin
> 
> On 2013/05/14 16:01:25, shatch wrote:
> > New snapshot uploaded. PTAL
> > 
> > 
> > This failed a DCHECK(!in_dtor_) on some dbg bots. I think what happened is
the
> > call to assign to the scoped_refptr in the constructor of WorkerServiceImpl
> > fails if PostTask completes too quickly in the constructor of
> > WorkerPrioritySetter. Managed to get the same failed DCHECK by inserting a
> small
> > sleep into the constructor after the PostTask call. Moved the call to
register
> > the observer into a separate function to ensure the scoped_refptr can
acquire.

[darin (slow to review), 2013-05-17 00:33:26]

LGTM

This is probably why the style guide recommends keeping constructors simple:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Doing_...

https://codereview.chromium.org/14137016/diff/127002/content/browser/worker_h...
File content/browser/worker_host/worker_service_impl.cc (right):

https://codereview.chromium.org/14137016/diff/127002/content/browser/worker_h...
content/browser/worker_host/worker_service_impl.cc:234:
priority_setter_->PostRegisterObserverTask();
nit: You might consider a more generic name for this method like "Initialize". 
from the point of view of the WorkerServiceImpl, it doesn't really care what
this method does other than that it is the second part of setting up the
WorkerPrioritySetter.

[shatch, 2013-05-17 21:00:46]

https://codereview.chromium.org/14137016/diff/127002/content/browser/worker_h...
File content/browser/worker_host/worker_service_impl.cc (right):

https://codereview.chromium.org/14137016/diff/127002/content/browser/worker_h...
content/browser/worker_host/worker_service_impl.cc:234:
priority_setter_->PostRegisterObserverTask();
On 2013/05/17 00:33:26, darin wrote:
> nit: You might consider a more generic name for this method like "Initialize".

> from the point of view of the WorkerServiceImpl, it doesn't really care what
> this method does other than that it is the second part of setting up the
> WorkerPrioritySetter.

Done.

[commit-bot: I haz the power, 2013-05-17 21:01:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonhatch@chromium.org/14137016/155001

[commit-bot: I haz the power, 2013-05-18 00:14:47]

Change committed as 200932

[shatch, 2013-05-20 21:22:41]

New snapshot uploaded.

Failed valgrind, priority_setter_ is destructed on UI thread, and needs to be
cleared out before that's shut down in unit test.


ptal:darin

[darin (slow to review), 2013-05-24 20:48:40]

https://codereview.chromium.org/14137016/diff/189002/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/14137016/diff/189002/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_unittest.cc:596:
WorkerServiceImpl::GetInstance()->PerformUnitTestTeardown();
another approach might have been to register a MessageLoop DestructionObserver
that will run when message_loop_ is being destroyed.

https://codereview.chromium.org/14137016/diff/189002/content/browser/worker_h...
File content/browser/worker_host/worker_service_impl.h (right):

https://codereview.chromium.org/14137016/diff/189002/content/browser/worker_h...
content/browser/worker_host/worker_service_impl.h:34: void
PerformUnitTestTeardown();
nitty nit: A common convention is to slap a ForTesting suffix on methods like
this.

[jam, 2013-05-28 14:28:29]

https://codereview.chromium.org/14137016/diff/51003/content/browser/worker_ho...
File content/browser/worker_host/worker_process_host.h (right):

https://codereview.chromium.org/14137016/diff/51003/content/browser/worker_ho...
content/browser/worker_host/worker_process_host.h:173: bool process_launched()
const;
to be clear: this should be inlined like the above getters, i.e.

bool process_launched() const { return process_launched_; }

https://codereview.chromium.org/14137016/diff/51003/content/browser/worker_ho...
File content/browser/worker_host/worker_service_impl.cc (right):

https://codereview.chromium.org/14137016/diff/51003/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.cc:40: WorkerPrioritySetter();
nit: since this class is all in the cc file, why split the declaration from the
definition? i find it more readable if they're together, but since this is just
personal style, i leave it ultimately to you

[shatch, 2013-05-28 17:52:12]

New snapshot uploaded.

https://codereview.chromium.org/14137016/diff/51003/content/browser/worker_ho...
File content/browser/worker_host/worker_process_host.h (right):

https://codereview.chromium.org/14137016/diff/51003/content/browser/worker_ho...
content/browser/worker_host/worker_process_host.h:173: bool process_launched()
const;
On 2013/05/28 14:28:30, jam wrote:
> to be clear: this should be inlined like the above getters, i.e.
> 
> bool process_launched() const { return process_launched_; }

Done.

https://codereview.chromium.org/14137016/diff/51003/content/browser/worker_ho...
File content/browser/worker_host/worker_service_impl.cc (right):

https://codereview.chromium.org/14137016/diff/51003/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.cc:40: WorkerPrioritySetter();
On 2013/05/28 14:28:30, jam wrote:
> nit: since this class is all in the cc file, why split the declaration from
the
> definition? i find it more readable if they're together, but since this is
just
> personal style, i leave it ultimately to you

I guess it was the opposite for me, find it easier to follow if split

https://codereview.chromium.org/14137016/diff/189002/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/14137016/diff/189002/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_unittest.cc:596:
WorkerServiceImpl::GetInstance()->PerformUnitTestTeardown();
On 2013/05/24 20:48:40, darin wrote:
> another approach might have been to register a MessageLoop DestructionObserver
> that will run when message_loop_ is being destroyed.

Didn't know about those, do you mind if I try that in a subsequent patch?

https://codereview.chromium.org/14137016/diff/189002/content/browser/worker_h...
File content/browser/worker_host/worker_service_impl.h (right):

https://codereview.chromium.org/14137016/diff/189002/content/browser/worker_h...
content/browser/worker_host/worker_service_impl.h:34: void
PerformUnitTestTeardown();
On 2013/05/24 20:48:40, darin wrote:
> nitty nit: A common convention is to slap a ForTesting suffix on methods like
> this.

Done.

[jam, 2013-05-28 17:56:17]

https://codereview.chromium.org/14137016/diff/51003/content/browser/worker_ho...
File content/browser/worker_host/worker_service_impl.cc (right):

https://codereview.chromium.org/14137016/diff/51003/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.cc:40: WorkerPrioritySetter();
On 2013/05/28 17:52:13, shatch wrote:
> On 2013/05/28 14:28:30, jam wrote:
> > nit: since this class is all in the cc file, why split the declaration from
> the
> > definition? i find it more readable if they're together, but since this is
> just
> > personal style, i leave it ultimately to you
> 
> I guess it was the opposite for me, find it easier to follow if split

ok, up to you since this is personal style and not in the style guide :)

[darin (slow to review), 2013-05-29 21:35:53]

LGTM

[commit-bot: I haz the power, 2013-05-30 15:37:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonhatch@chromium.org/14137016/220001

[commit-bot: I haz the power, 2013-05-30 17:36:08]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-05-30 18:33:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonhatch@chromium.org/14137016/220001

[commit-bot: I haz the power, 2013-05-31 00:04:22]

Change committed as 203272