Hook up loading signal to ResourceScheduler.

Hook up RenderWidgetHost's
loading signal to ResourceScheduler

BUG=128035
Committed: https://crrev.com/fcdaa48695d24f5ced472aa727898c0328ede80e
Cr-Commit-Position: refs/heads/master@{#292695}

[aiolos (Not reviewing), 2014-08-23 02:15:57]

Matt: are the soon-to-come browser tests enough for adding this signal? I poked
at adding a unittest for it like Zhen did, but don't think it would be
worthwhile. I verified that the signal works via dtrace in the mean time. I
doubt it will change in the next week or so.

Scott: I'm adding is_loading as a signal to the ResourceScheduler. Could you
please verify that I'm piping the signal correctly in the three
render_*_host_impl.* files?

[sky, 2014-08-25 15:24:31]

https://codereview.chromium.org/501703002/diff/20001/content/browser/loader/r...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/501703002/diff/20001/content/browser/loader/r...
content/browser/loader/resource_scheduler.h:119: // Called when a renderer stops
or restarts loading.
Why are you moving these? Style guide says definition and declaration position
should match. So if you move this you have to move the other.

[mmenke, 2014-08-25 15:50:49]

On 2014/08/23 02:15:57, aiolos wrote:
> Matt: are the soon-to-come browser tests enough for adding this signal? I
poked
> at adding a unittest for it like Zhen did, but don't think it would be
> worthwhile. I verified that the signal works via dtrace in the mean time. I
> doubt it will change in the next week or so.
> 
> Scott: I'm adding is_loading as a signal to the ResourceScheduler. Could you
> please verify that I'm piping the signal correctly in the three
> render_*_host_impl.* files?

Yes, I'm fine with just browser tests.  That having been said, it may be
possible to write unit tests as well.  There's a test fixture that allows fake
RenderViewHosts in unit tests, but not sure if it allows for testing of this
kind of thing or not.

[aiolos (Not reviewing), 2014-08-26 14:12:19]

On 2014/08/25 15:24:31, sky wrote:
>
https://codereview.chromium.org/501703002/diff/20001/content/browser/loader/r...
> File content/browser/loader/resource_scheduler.h (right):
> 
>
https://codereview.chromium.org/501703002/diff/20001/content/browser/loader/r...
> content/browser/loader/resource_scheduler.h:119: // Called when a renderer
stops
> or restarts loading.
> Why are you moving these? Style guide says definition and declaration position
> should match. So if you move this you have to move the other.

I was moving them out of the Client Functions section of the header file and
into the UI Signals section since these functions aren't going through the
Client. I can move the declarations as well.

[aiolos (Not reviewing), 2014-08-26 14:29:08]

On 2014/08/25 15:50:49, mmenke wrote:
> On 2014/08/23 02:15:57, aiolos wrote:
> > Matt: are the soon-to-come browser tests enough for adding this signal? I
> poked
> > at adding a unittest for it like Zhen did, but don't think it would be
> > worthwhile. I verified that the signal works via dtrace in the mean time. I
> > doubt it will change in the next week or so.
> > 
> > Scott: I'm adding is_loading as a signal to the ResourceScheduler. Could you
> > please verify that I'm piping the signal correctly in the three
> > render_*_host_impl.* files?
> 
> Yes, I'm fine with just browser tests.  That having been said, it may be
> possible to write unit tests as well.  There's a test fixture that allows fake
> RenderViewHosts in unit tests, but not sure if it allows for testing of this
> kind of thing or not.

When I looked at it earlier, I remember seeing a comment about avoiding sending
load events for other tests. I didn't see that again when I just looked though.
But the test harness does override SetIsLoading to do nothing. I could change
that, but I assume it will break current tests.

[aiolos (Not reviewing), 2014-08-26 15:57:06]

On 2014/08/26 14:12:19, aiolos wrote:
> On 2014/08/25 15:24:31, sky wrote:
> >
>
https://codereview.chromium.org/501703002/diff/20001/content/browser/loader/r...
> > File content/browser/loader/resource_scheduler.h (right):
> > 
> >
>
https://codereview.chromium.org/501703002/diff/20001/content/browser/loader/r...
> > content/browser/loader/resource_scheduler.h:119: // Called when a renderer
> stops
> > or restarts loading.
> > Why are you moving these? Style guide says definition and declaration
position
> > should match. So if you move this you have to move the other.
> 
> I was moving them out of the Client Functions section of the header file and
> into the UI Signals section since these functions aren't going through the
> Client. I can move the declarations as well.

ptal

[mmenke, 2014-08-26 16:04:11]

On 2014/08/26 15:57:06, aiolos wrote:
> On 2014/08/26 14:12:19, aiolos wrote:
> > On 2014/08/25 15:24:31, sky wrote:
> > >
> >
>
https://codereview.chromium.org/501703002/diff/20001/content/browser/loader/r...
> > > File content/browser/loader/resource_scheduler.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/501703002/diff/20001/content/browser/loader/r...
> > > content/browser/loader/resource_scheduler.h:119: // Called when a renderer
> > stops
> > > or restarts loading.
> > > Why are you moving these? Style guide says definition and declaration
> position
> > > should match. So if you move this you have to move the other.
> > 
> > I was moving them out of the Client Functions section of the header file and
> > into the UI Signals section since these functions aren't going through the
> > Client. I can move the declarations as well.
> 
> ptal

Will do.  Should get to this after lunch sometime.

[mmenke, 2014-08-26 18:16:06]

Do you want to land this as-is, or hold off until you have browser tests?  If
the former, should add a bug ID.

https://codereview.chromium.org/501703002/diff/40001/content/browser/loader/r...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/501703002/diff/40001/content/browser/loader/r...
content/browser/loader/resource_scheduler.h:150: bool
IsClientLoadedForTesting(int child_id, int route_id);
This isn't currently used - I'm fine with having this if we use it in tests, but
if we're not going to use it, I don't think we should have it.

[aiolos (Not reviewing), 2014-08-26 18:28:28]

Bug ID added.

https://codereview.chromium.org/501703002/diff/40001/content/browser/loader/r...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/501703002/diff/40001/content/browser/loader/r...
content/browser/loader/resource_scheduler.h:150: bool
IsClientLoadedForTesting(int child_id, int route_id);
On 2014/08/26 18:16:06, mmenke wrote:
> This isn't currently used - I'm fine with having this if we use it in tests,
but
> if we're not going to use it, I don't think we should have it.

It's easy enough to add back in if it ends up in tests. Removed for now.

[mmenke, 2014-08-26 18:30:29]

On 2014/08/26 18:28:28, aiolos wrote:
> Bug ID added.
> 
>
https://codereview.chromium.org/501703002/diff/40001/content/browser/loader/r...
> File content/browser/loader/resource_scheduler.h (right):
> 
>
https://codereview.chromium.org/501703002/diff/40001/content/browser/loader/r...
> content/browser/loader/resource_scheduler.h:150: bool
> IsClientLoadedForTesting(int child_id, int route_id);
> On 2014/08/26 18:16:06, mmenke wrote:
> > This isn't currently used - I'm fine with having this if we use it in tests,
> but
> > if we're not going to use it, I don't think we should have it.
> 
> It's easy enough to add back in if it ends up in tests. Removed for now.

LGTM.

Maybe update your description to something like "Hook up RenderWidgetHost's
loading signal to ResourceScheduler"?  ResourceScheduler already handles loading
signals, it's just not getting them.

[sky, 2014-08-28 20:27:50]

sky@chromium.org changed reviewers:
+ jam@chromium.org

[sky, 2014-08-28 20:27:50]

This LGTM, but I'm not an expert on the routing.
+jam

[jam, 2014-08-28 23:43:57]

lgtm

https://codereview.chromium.org/501703002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/501703002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.h:230: virtual void
SetIsLoading(bool is_loading);
nit: move this to line 223 with the other virtual methods

[aiolos (Not reviewing), 2014-08-29 03:58:17]

The CQ bit was checked by aiolos@chromium.org

[aiolos (Not reviewing), 2014-08-29 03:58:30]

The CQ bit was unchecked by aiolos@chromium.org

[aiolos (Not reviewing), 2014-08-29 16:43:52]

The CQ bit was checked by aiolos@chromium.org

[commit-bot: I haz the power, 2014-08-29 16:44:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aiolos@chromium.org/501703002/80001

[commit-bot: I haz the power, 2014-08-29 18:27:33]

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

[commit-bot: I haz the power, 2014-08-29 22:39:44]

Committed patchset #5 (id:80001) as 619d1efb6397f8889d943626b57a37298d64614f

[commit-bot: I haz the power, 2014-09-10 03:10:56]

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