Enable resource prefetch from the browser process.

Enable resource prefetch from the browser process.

R=jam@chromium.org, jochen@chromium.org, jschuh@chromium.org

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

[tburkard, 2014-06-12 18:53:41]

I discussed this change with all of you.

It'd be great if you could review this.

Thanks.

[jam, 2014-06-12 19:39:55]

Is this part of the web platform? Or an experimental chrome-specific feature?

Because right now it seems this could be implemented entirely in src\chrome.

[tburkard, 2014-06-12 20:02:20]

Good point. I now realize how I could do this entirely in chrome.

I will redo this in chrome.

Thanks.


On Thu, Jun 12, 2014 at 9:39 PM, <jam@chromium.org> wrote:

> Is this part of the web platform? Or an experimental chrome-specific
> feature?
>
> Because right now it seems this could be implemented entirely in
> src\chrome.
>
> https://codereview.chromium.org/334483004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tburkard, 2014-06-13 13:16:14]

Moved this completely to chrome.

jam, could you review the ipc part.
jochen, could you review the chrome part.

Nate, I Cc'ed you in case you have some input since this based on a discussion
with you.

Ditto for jkarlin as owner of prefetch.

[jochen (gone - plz use gerrit), 2014-06-13 16:42:56]

needs moar tests

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
File chrome/renderer/prefetch_helper.cc (right):

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
chrome/renderer/prefetch_helper.cc:18: PrefetchManager() { }
nit. no space between { } (did you clang-format?)

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
chrome/renderer/prefetch_helper.cc:21: STLDeleteElements(&loader_set_);
what happens if the manager is deleted while loads are still ongoing? Can we
have a test for this?

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
chrome/renderer/prefetch_helper.cc:49: };
nit. disallow copy & assign

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
chrome/renderer/prefetch_helper.cc:70:
prefetch_manager_->AddPrefetch(render_frame()->GetWebFrame(), url);
what's the reason you use a separate class here?

[jam, 2014-06-13 16:44:38]

lgtm with comments. i'm not an ipc owner so you will need someone from security
to review

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
File chrome/renderer/prefetch_helper.cc (right):

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
chrome/renderer/prefetch_helper.cc:65: // message.
i dont think you need this behavior for this ipc, at least not yet. so do this
block with a standard handled return value that is set inside the macros

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
File chrome/renderer/prefetch_helper.h (right):

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
chrome/renderer/prefetch_helper.h:13: // Helper class iniitiate prefetches on
behalf of a RenderFrame.
nit: spelling

[jkarlin, 2014-06-13 16:53:46]

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
File chrome/renderer/prefetch_helper.cc (right):

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
chrome/renderer/prefetch_helper.cc:70:
prefetch_manager_->AddPrefetch(render_frame()->GetWebFrame(), url);
On 2014/06/13 16:42:56, jochen traveling until Jun 23 wrote:
> what's the reason you use a separate class here?

Also, I'm adding a chrome/browser/prefetch/prefetch_manager.h+cc so would prefer
not to have another class with the same name.

[tburkard, 2014-06-13 18:35:46]

Addressed all comments except for the test Jochen asked for.

Jochen, I will send another update once I have finished the test.

In the meantime, let me know if there is anything else.

Also, adding jschuh. Justin, are these ipc changes ok?

Thanks.

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
File chrome/renderer/prefetch_helper.cc (right):

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
chrome/renderer/prefetch_helper.cc:18: PrefetchManager() { }
On 2014/06/13 16:42:55, jochen traveling until Jun 23 wrote:
> nit. no space between { } (did you clang-format?)

obsolete comment

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
chrome/renderer/prefetch_helper.cc:21: STLDeleteElements(&loader_set_);
On 2014/06/13 16:42:56, jochen traveling until Jun 23 wrote:
> what happens if the manager is deleted while loads are still ongoing? Can we
> have a test for this?

will add a test for this later (will send out a new update, before committing),
just wanted to get out this revised version first.

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
chrome/renderer/prefetch_helper.cc:49: };
On 2014/06/13 16:42:56, jochen traveling until Jun 23 wrote:
> nit. disallow copy & assign

obsolete comment

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
chrome/renderer/prefetch_helper.cc:65: // message.
On 2014/06/13 16:44:37, jam wrote:
> i dont think you need this behavior for this ipc, at least not yet. so do this
> block with a standard handled return value that is set inside the macros

Done. Does this look ok?

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
chrome/renderer/prefetch_helper.cc:70:
prefetch_manager_->AddPrefetch(render_frame()->GetWebFrame(), url);
On 2014/06/13 16:42:56, jochen traveling until Jun 23 wrote:
> what's the reason you use a separate class here?

Done.

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
chrome/renderer/prefetch_helper.cc:70:
prefetch_manager_->AddPrefetch(render_frame()->GetWebFrame(), url);
On 2014/06/13 16:53:46, jkarlin wrote:
> On 2014/06/13 16:42:56, jochen traveling until Jun 23 wrote:
> > what's the reason you use a separate class here?
> 
> Also, I'm adding a chrome/browser/prefetch/prefetch_manager.h+cc so would
prefer
> not to have another class with the same name.

Done.

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
File chrome/renderer/prefetch_helper.h (right):

https://codereview.chromium.org/334483004/diff/40001/chrome/renderer/prefetch...
chrome/renderer/prefetch_helper.h:13: // Helper class iniitiate prefetches on
behalf of a RenderFrame.
On 2014/06/13 16:44:37, jam wrote:
> nit: spelling

Done.

[jschuh, 2014-06-13 19:21:55]

This message is browser -> renderer?

[tburkard, 2014-06-15 05:53:52]

On 2014/06/13 19:21:55, Justin Schuh wrote:
> This message is browser -> renderer?

Yes.

[tburkard, 2014-06-16 14:20:58]

Added the test Jochen requested.

Jochen, Josh, and Justin: can you take another pass and let me know if you have
any further requests?

I'd like to land this in the next day or so.

Thanks.

[jschuh, 2014-06-16 14:38:44]

ipc security lgtm (notes: no security impact)

[jkarlin, 2014-06-16 14:43:27]

https://codereview.chromium.org/334483004/diff/80001/chrome/renderer/prefetch...
File chrome/renderer/prefetch_helper.cc (right):

https://codereview.chromium.org/334483004/diff/80001/chrome/renderer/prefetch...
chrome/renderer/prefetch_helper.cc:35: blink::WebURLRequest request(url);
Where are you setting the priority of this request?

[jkarlin, 2014-06-16 14:48:29]

https://codereview.chromium.org/334483004/diff/80001/chrome/browser/prefetch/...
File chrome/browser/prefetch/prefetch_browsertest.cc (right):

https://codereview.chromium.org/334483004/diff/80001/chrome/browser/prefetch/...
chrome/browser/prefetch/prefetch_browsertest.cc:186: loop_.Run();
What is this test testing exactly?

[tburkard, 2014-06-16 15:02:58]

https://codereview.chromium.org/334483004/diff/80001/chrome/browser/prefetch/...
File chrome/browser/prefetch/prefetch_browsertest.cc (right):

https://codereview.chromium.org/334483004/diff/80001/chrome/browser/prefetch/...
chrome/browser/prefetch/prefetch_browsertest.cc:186: loop_.Run();
On 2014/06/16 14:48:29, jkarlin wrote:
> What is this test testing exactly?

Done.

added a comment explaining

https://codereview.chromium.org/334483004/diff/80001/chrome/renderer/prefetch...
File chrome/renderer/prefetch_helper.cc (right):

https://codereview.chromium.org/334483004/diff/80001/chrome/renderer/prefetch...
chrome/renderer/prefetch_helper.cc:35: blink::WebURLRequest request(url);
On 2014/06/16 14:43:27, jkarlin wrote:
> Where are you setting the priority of this request?

I thought setting the type as Prefetch would be sufficient? I modeled this after
LinkLoader, and I thought there wasn't anything else re: priority there?

Please let me know!

Thanks.

[jkarlin, 2014-06-16 15:31:02]

https://codereview.chromium.org/334483004/diff/80001/chrome/renderer/prefetch...
File chrome/renderer/prefetch_helper.cc (right):

https://codereview.chromium.org/334483004/diff/80001/chrome/renderer/prefetch...
chrome/renderer/prefetch_helper.cc:35: blink::WebURLRequest request(url);
On 2014/06/16 15:02:57, tburkard wrote:
> On 2014/06/16 14:43:27, jkarlin wrote:
> > Where are you setting the priority of this request?
> 
> I thought setting the type as Prefetch would be sufficient? I modeled this
after
> LinkLoader, and I thought there wasn't anything else re: priority there?
> 
> Please let me know!
> 
> Thanks.

It's set by ResourceFetcher in Blink
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....
 This CL bypasses the ResourceFetcher.

[tburkard, 2014-06-17 04:02:15]

Once blink has rolled, I will set the priority appropriately as described below.

Meanwhile, Jochen/Josh, if you have any other requests, please let me know so
that I can commit this soon.

Thanks.

https://codereview.chromium.org/334483004/diff/80001/chrome/renderer/prefetch...
File chrome/renderer/prefetch_helper.cc (right):

https://codereview.chromium.org/334483004/diff/80001/chrome/renderer/prefetch...
chrome/renderer/prefetch_helper.cc:35: blink::WebURLRequest request(url);
On 2014/06/16 15:31:02, jkarlin wrote:
> On 2014/06/16 15:02:57, tburkard wrote:
> > On 2014/06/16 14:43:27, jkarlin wrote:
> > > Where are you setting the priority of this request?
> > 
> > I thought setting the type as Prefetch would be sufficient? I modeled this
> after
> > LinkLoader, and I thought there wasn't anything else re: priority there?
> > 
> > Please let me know!
> > 
> > Thanks.
> 
> It's set by ResourceFetcher in Blink
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....
>  This CL bypasses the ResourceFetcher.

Just waiting for a blink roll to include
https://codereview.chromium.org/338093006/
Then, I will add setPriority to VeryLow.

[jochen (gone - plz use gerrit), 2014-06-17 04:25:18]

lgtm with nits

https://codereview.chromium.org/334483004/diff/100001/chrome/browser/prefetch...
File chrome/browser/prefetch/prefetch_browsertest.cc (right):

https://codereview.chromium.org/334483004/diff/100001/chrome/browser/prefetch...
chrome/browser/prefetch/prefetch_browsertest.cc:99: net::NetworkDelegate*
network_delegate)
nit, please clang-format

https://codereview.chromium.org/334483004/diff/100001/chrome/browser/prefetch...
chrome/browser/prefetch/prefetch_browsertest.cc:103: virtual void Start()
OVERRIDE {}
while i'm adding nits anyway: it's good practice to add a comment from which
interface the methods are you're overriding.

https://codereview.chromium.org/334483004/diff/100001/chrome/browser/prefetch...
chrome/browser/prefetch/prefetch_browsertest.cc:107: };
disallow copy and assign

https://codereview.chromium.org/334483004/diff/100001/chrome/browser/prefetch...
chrome/browser/prefetch/prefetch_browsertest.cc:111: explicit
HangingRequestInterceptor(base::Closure callback)
pass callbacks as const ref

https://codereview.chromium.org/334483004/diff/100001/chrome/browser/prefetch...
chrome/browser/prefetch/prefetch_browsertest.cc:120: if (!callback_.is_null()) {
no { }

[tburkard, 2014-06-17 09:13:03]

https://codereview.chromium.org/334483004/diff/100001/chrome/browser/prefetch...
File chrome/browser/prefetch/prefetch_browsertest.cc (right):

https://codereview.chromium.org/334483004/diff/100001/chrome/browser/prefetch...
chrome/browser/prefetch/prefetch_browsertest.cc:99: net::NetworkDelegate*
network_delegate)
On 2014/06/17 04:25:17, jochen traveling until Jun 23 wrote:
> nit, please clang-format

Done.

https://codereview.chromium.org/334483004/diff/100001/chrome/browser/prefetch...
chrome/browser/prefetch/prefetch_browsertest.cc:103: virtual void Start()
OVERRIDE {}
On 2014/06/17 04:25:17, jochen traveling until Jun 23 wrote:
> while i'm adding nits anyway: it's good practice to add a comment from which
> interface the methods are you're overriding.

Done.

https://codereview.chromium.org/334483004/diff/100001/chrome/browser/prefetch...
chrome/browser/prefetch/prefetch_browsertest.cc:107: };
On 2014/06/17 04:25:17, jochen traveling until Jun 23 wrote:
> disallow copy and assign

Done.

https://codereview.chromium.org/334483004/diff/100001/chrome/browser/prefetch...
chrome/browser/prefetch/prefetch_browsertest.cc:111: explicit
HangingRequestInterceptor(base::Closure callback)
On 2014/06/17 04:25:17, jochen traveling until Jun 23 wrote:
> pass callbacks as const ref

Done.

https://codereview.chromium.org/334483004/diff/100001/chrome/browser/prefetch...
chrome/browser/prefetch/prefetch_browsertest.cc:120: if (!callback_.is_null()) {
On 2014/06/17 04:25:17, jochen traveling until Jun 23 wrote:
> no { }

Done.

[jkarlin, 2014-06-17 10:15:33]

https://codereview.chromium.org/334483004/diff/120001/chrome/common/prefetch_...
File chrome/common/prefetch_messages.h (right):

https://codereview.chromium.org/334483004/diff/120001/chrome/common/prefetch_...
chrome/common/prefetch_messages.h:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
2014

https://codereview.chromium.org/334483004/diff/120001/chrome/renderer/prefetc...
File chrome/renderer/prefetch_helper.h (right):

https://codereview.chromium.org/334483004/diff/120001/chrome/renderer/prefetc...
chrome/renderer/prefetch_helper.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
2014

[tburkard, 2014-06-17 10:46:52]

Everything addressed, including priority.

Josh, can you take another look?

Thanks.

https://codereview.chromium.org/334483004/diff/80001/chrome/renderer/prefetch...
File chrome/renderer/prefetch_helper.cc (right):

https://codereview.chromium.org/334483004/diff/80001/chrome/renderer/prefetch...
chrome/renderer/prefetch_helper.cc:35: blink::WebURLRequest request(url);
On 2014/06/16 14:43:27, jkarlin wrote:
> Where are you setting the priority of this request?

Done.

https://codereview.chromium.org/334483004/diff/120001/chrome/common/prefetch_...
File chrome/common/prefetch_messages.h (right):

https://codereview.chromium.org/334483004/diff/120001/chrome/common/prefetch_...
chrome/common/prefetch_messages.h:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
On 2014/06/17 10:15:33, jkarlin wrote:
> 2014

Done.

https://codereview.chromium.org/334483004/diff/120001/chrome/renderer/prefetc...
File chrome/renderer/prefetch_helper.h (right):

https://codereview.chromium.org/334483004/diff/120001/chrome/renderer/prefetc...
chrome/renderer/prefetch_helper.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
On 2014/06/17 10:15:33, jkarlin wrote:
> 2014

Done.

[jkarlin, 2014-06-17 10:54:28]

On 2014/06/17 10:46:52, tburkard wrote:
> Everything addressed, including priority.
> 
> Josh, can you take another look?
> 
> Thanks.
> 
>
https://codereview.chromium.org/334483004/diff/80001/chrome/renderer/prefetch...
> File chrome/renderer/prefetch_helper.cc (right):
> 
>
https://codereview.chromium.org/334483004/diff/80001/chrome/renderer/prefetch...
> chrome/renderer/prefetch_helper.cc:35: blink::WebURLRequest request(url);
> On 2014/06/16 14:43:27, jkarlin wrote:
> > Where are you setting the priority of this request?
> 
> Done.
> 
>
https://codereview.chromium.org/334483004/diff/120001/chrome/common/prefetch_...
> File chrome/common/prefetch_messages.h (right):
> 
>
https://codereview.chromium.org/334483004/diff/120001/chrome/common/prefetch_...
> chrome/common/prefetch_messages.h:1: // Copyright (c) 2012 The Chromium
Authors.
> All rights reserved.
> On 2014/06/17 10:15:33, jkarlin wrote:
> > 2014
> 
> Done.
> 
>
https://codereview.chromium.org/334483004/diff/120001/chrome/renderer/prefetc...
> File chrome/renderer/prefetch_helper.h (right):
> 
>
https://codereview.chromium.org/334483004/diff/120001/chrome/renderer/prefetc...
> chrome/renderer/prefetch_helper.h:1: // Copyright (c) 2011 The Chromium
Authors.
> All rights reserved.
> On 2014/06/17 10:15:33, jkarlin wrote:
> > 2014
> 
> Done.

looks good to me (note I'm not an owner of anything so am not supposed to type
the magic 4 letters)

[tburkard, 2014-06-17 10:56:37]

The CQ bit was checked by tburkard@chromium.org

[commit-bot: I haz the power, 2014-06-17 10:58:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tburkard@chromium.org/334483004/140001

[tburkard, 2014-06-17 12:17:46]

The CQ bit was checked by tburkard@chromium.org

[commit-bot: I haz the power, 2014-06-17 12:19:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tburkard@chromium.org/334483004/160001

[tburkard, 2014-06-17 15:02:44]

Committed patchset #9 manually as r277761 (presubmit successful).