Pass WebKit priority changes and parsing messages up to the browser process.

Pass WebKit priority changes and parsing messages up to browser process.

These signals will be consumed by the upcoming ResourceScheduler. The first use
case will be preloaded images. WebKit will initially preload images at IDLE
priority. Once the image is needed by the parser, it will be promoted to normal
image priority (LOW). This nets us a 2-3% PLT improvement.


BUG=163963

[James Simonsen, 2013-02-06 04:32:26]

You might want to skim ResourceScheduler.h in my other patch as a reminder of
how this all will work. ResourceScheduler::ScheduledRequest will consume this
signal.

Will, are there any negative consequences to calling URLRequest::set_priority()
after its started? I know we don't do anything useful yet, but will it actually
cause harm?

[willchan no longer on Chromium, 2013-02-06 04:46:14]

I do not believe it will cause harm. Can you set BUG to
https://code.google.com/p/chromium/issues/detail?id=163963?

[James Simonsen, 2013-02-06 04:48:17]

Done.

[darin (slow to review), 2013-02-06 05:04:13]

https://codereview.chromium.org/12243002/diff/1/content/browser/loader/resour...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/12243002/diff/1/content/browser/loader/resour...
content/browser/loader/resource_dispatcher_host_impl.cc:1143:
loader->ChangePriority(ConvertWebKitPriorityToNetPriority(priority));
Maybe instead of routing this notification through the ResourceHandler and
ResourceThrottle interfaces, you could just pass the notification directly to
the ResourceScheduler.  It could maintain a mapping from (child_id, request_id)
to scheduled requests.

https://codereview.chromium.org/12243002/diff/1/content/public/browser/resour...
File content/public/browser/resource_throttle.h (right):

https://codereview.chromium.org/12243002/diff/1/content/public/browser/resour...
content/public/browser/resource_throttle.h:28: virtual void
OnRequestPriorityChanged() {}
Hmm, this doesn't quite fit with the scope of ResourceThrottle.  All of the
other methods are about providing opportunities to defer resource loading at
particular stages of the process.

[James Simonsen, 2013-02-06 22:41:29]

Fair enough. Passing through to the ResourceThrottle was the ugliest part and
the additional map works fine as a substitute.

I've restructured things a little. Now we pass up net::RequestPriority instead
of WebKit::WebURLRequest::Priority. It makes more sense to make that transition
in webkit/glue.

Also, I added the DidParseBody message to this CL. It could be its own CL, but
it's tiny and both messages are needed by the Resource Scheduler.

Once this is in, I'll update the Resource Scheduler CL and we should be good to
go.

[darin (slow to review), 2013-02-06 22:44:43]

There are actually a number of cases where ResourceHandler subclasses need
to subscribe to IPC messages that are keyed to a resource request.  It
seems like it might make sense to add a map on ResourceDispatcherHostImpl
that enables one to register a handler for IPC messages that may be
resource request specific.

Take a look at async_resource_handler.cc for an example of a special case
that could be generalized with a mechanism as proposed above.

-Darin


On Wed, Feb 6, 2013 at 2:41 PM, <simonjam@chromium.org> wrote:

> Fair enough. Passing through to the ResourceThrottle was the ugliest part
> and
> the additional map works fine as a substitute.
>
> I've restructured things a little. Now we pass up net::RequestPriority
> instead
> of WebKit::WebURLRequest::**Priority. It makes more sense to make that
> transition
> in webkit/glue.
>
> Also, I added the DidParseBody message to this CL. It could be its own CL,
> but
> it's tiny and both messages are needed by the Resource Scheduler.
>
> Once this is in, I'll update the Resource Scheduler CL and we should be
> good to
> go.
>
>
https://codereview.chromium.**org/12243002/<https://codereview.chromium.org/1...
>

[James Simonsen, 2013-02-07 03:39:31]

BTW, this CL stands on its own. Please review it when you get a chance.

In the meantime, I also sent out a review for the refactoring, which doesn't
interact with this CL at all.

[darin (slow to review), 2013-02-07 07:40:41]

LGTM

https://codereview.chromium.org/12243002/diff/11001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12243002/diff/11001/content/renderer/render_v...
content/renderer/render_view_impl.cc:4479: if (frame == webview()->mainFrame())
{
nit: !frame->parent() is a slightly more compact way of checking for topmost

https://codereview.chromium.org/12243002/diff/11001/content/renderer/render_v...
content/renderer/render_view_impl.cc:4480: Send(new
ViewHostMsg_DidParseBody(routing_id()));
Maybe the IPC message should also use the term "Insert" instead of "Parse"?  Or,
is willInsertBody misnamed?

[James Simonsen, 2013-02-11 22:53:11]

Adding security reviewer for new IPC messages.

https://codereview.chromium.org/12243002/diff/11001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12243002/diff/11001/content/renderer/render_v...
content/renderer/render_view_impl.cc:4479: if (frame == webview()->mainFrame())
{
On 2013/02/07 07:40:41, darin wrote:
> nit: !frame->parent() is a slightly more compact way of checking for topmost

Done.

https://codereview.chromium.org/12243002/diff/11001/content/renderer/render_v...
content/renderer/render_view_impl.cc:4480: Send(new
ViewHostMsg_DidParseBody(routing_id()));
On 2013/02/07 07:40:41, darin wrote:
> Maybe the IPC message should also use the term "Insert" instead of "Parse"? 
Or,
> is willInsertBody misnamed?

Done.

Either name is accurate. There's a delay between when the body is parsed and
when it's inserted, because it's a micro-task that does it. I've made it
consistently WillInsertBody throughout.

[Cris Neckar, 2013-02-11 23:14:12]

lgtm for IPC message security audit

https://codereview.chromium.org/12243002/diff/12013/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/12243002/diff/12013/content/common/view_messa...
content/common/view_messages.h:2401:
IPC_MESSAGE_ROUTED0(ViewHostMsg_WillInsertBody)
Please also get a security review when you land the handler for this.

[willchan no longer on Chromium, 2013-02-11 23:20:49]

https://codereview.chromium.org/12243002/diff/12013/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/12243002/diff/12013/content/common/view_messa...
content/common/view_messages.h:2401:
IPC_MESSAGE_ROUTED0(ViewHostMsg_WillInsertBody)
On 2013/02/11 23:14:12, Cris Neckar wrote:
> Please also get a security review when you land the handler for this.

I haven't read this CL, but if there's no handler for this yet, I recommend
removing the IPC and the code that sends it. I recommend the security team
insist on this in the future, because I am paranoid :)

[Cris Neckar, 2013-02-11 23:22:24]

On 2013/02/11 23:20:49, willchan wrote:
>
https://codereview.chromium.org/12243002/diff/12013/content/common/view_messa...
> File content/common/view_messages.h (right):
> 
>
https://codereview.chromium.org/12243002/diff/12013/content/common/view_messa...
> content/common/view_messages.h:2401:
> IPC_MESSAGE_ROUTED0(ViewHostMsg_WillInsertBody)
> On 2013/02/11 23:14:12, Cris Neckar wrote:
> > Please also get a security review when you land the handler for this.
> 
> I haven't read this CL, but if there's no handler for this yet, I recommend
> removing the IPC and the code that sends it. I recommend the security team
> insist on this in the future, because I am paranoid :)

Should fall through to a crash but on principle you seem right :)

[willchan no longer on Chromium, 2013-02-11 23:36:25]

Oh, I was thinking more in terms of forcing people to get reviews for the
handlers. When people add new base/ APIs, I often insist on seeing the
patch that will use it, so I can evaluate its use as well, before I let the
base/ change go in. If the IPC is powerful enough to require the handler to
get a security review, then I'd block landing the IPC code on showing the
handler code as well, because sometimes people forget to cc the API owner
afterward.

Just my two cents :)


On Mon, Feb 11, 2013 at 3:22 PM, <cdn@chromium.org> wrote:

> On 2013/02/11 23:20:49, willchan wrote:
>
> https://codereview.chromium.**org/12243002/diff/12013/**
>
content/common/view_messages.h<https://codereview.chromium.org/12243002/diff/12013/content/common/view_messages.h>
>
>> File content/common/view_messages.h (right):
>>
>
>
> https://codereview.chromium.**org/12243002/diff/12013/**
>
content/common/view_messages.**h#newcode2401<https://codereview.chromium.org/12243002/diff/12013/content/common/view_messages.h#newcode2401>
>
>> content/common/view_messages.**h:2401:
>> IPC_MESSAGE_ROUTED0(**ViewHostMsg_WillInsertBody)
>> On 2013/02/11 23:14:12, Cris Neckar wrote:
>> > Please also get a security review when you land the handler for this.
>>
>
>  I haven't read this CL, but if there's no handler for this yet, I
>> recommend
>> removing the IPC and the code that sends it. I recommend the security team
>> insist on this in the future, because I am paranoid :)
>>
>
> Should fall through to a crash but on principle you seem right :)
>
>
https://codereview.chromium.**org/12243002/<https://codereview.chromium.org/1...
>

[James Simonsen, 2013-02-12 00:09:00]

I was just trying to split up the ResourceScheduler CL into smaller pieces.
Plumbing up to the browser seemed like a good chunk (plus this CL used to do a
bit more, but we trimmed that out).

I thought the security review was just to make sure I'm not passing something
stupid around, like a FD.

If we need to review the handler now, I'll just have to merge this in to the
ResourceScheduler CL and have you guys review that.

FWIW, an unhandled IPC doesn't cause a crash. I can browse around just fine with
this build.

[Cris Neckar, 2013-02-12 20:41:33]

> I thought the security review was just to make sure I'm not passing something
> stupid around, like a FD.
> 
> If we need to review the handler now, I'll just have to merge this in to the
> ResourceScheduler CL and have you guys review that.

We ALWAYS need to review handlers, especially for messages to the browser.
Otherwise we end up with benign sounding messages which do insane things.
 
> FWIW, an unhandled IPC doesn't cause a crash. I can browse around just fine
with
> this build.

Weird I thought we forced the renderer to crash when we got a browser message
with no handler.