Split requestResource into request preparation and loading

Split requestResource into request preparation and loading

This CL is refactoring of requestResource in order to split the
request preparation phase from the request loading phase. 
This would enable to separate these phases for link prefetch, in order to
delay its actual loading.

BUG=651160
Review-Url: https://codereview.chromium.org/2660213003
Cr-Commit-Position: refs/heads/master@{#447454}
Committed: https://chromium.googlesource.com/chromium/src/+/ab7722c427586b8b0b7e0f82e2f3e7d5d67ec46a

[Yoav Weiss, 2017-01-30 20:31:12]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 20:32:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yoav Weiss, 2017-01-30 20:42:29]

Description was changed from

==========
requestResource refactoring

This is simple refactoring of requestResource in order to split the
request preparation phase from the request loading phase. This would
enable e.g. to separate these phases for link prefetch, in order to
delay the latter.

BUG=
==========

to

==========
Split requestResource into request preparation and loading

This CL is refactoring of requestResource in order to split the
request preparation phase from the request loading phase. 
This would enable to separate these phases for link prefetch, in order to
delay its actual loading.

BUG=651160
==========

[Yoav Weiss, 2017-01-30 20:42:29]

yoav@yoav.ws changed reviewers:
+ csharrison@chromium.org

[Yoav Weiss, 2017-01-30 20:43:58]

Hey Charlie,

Mind taking a look? This CL doesn't change functionality, but logically splits
requestResource into two functional parts.

[commit-bot: I haz the power, 2017-01-30 21:24:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-30 21:24:06]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Charlie Harrison, 2017-01-30 22:02:58]

looks good, just a few comments.

https://codereview.chromium.org/2660213003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2660213003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:473:
TRACE_EVENT1("blink", "ResourceFetcher::requestResource", "url",
This changes behavior, as trace events are scoped to the function they are
defined in. Could you just move this to the top of requestResource, so it is
aligned with the SCOPED_BLINK_UMA histogram?

https://codereview.chromium.org/2660213003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h (right):

https://codereview.chromium.org/2660213003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h:174: struct
PrepareRequestReturnValues {
delete unused struct.

https://codereview.chromium.org/2660213003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h:179: Resource*
prepareRequest(FetchRequest&,
Please document this method. It is a bit strange for a method to be called
"prepareRequest" but return a Resource*, so I think a little explanation is
necessary.

[Yoav Weiss, 2017-01-31 18:38:01]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 18:38:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yoav Weiss, 2017-01-31 18:39:59]

On 2017/01/30 22:02:58, Charlie Harrison-slow til Feb3 wrote:
> looks good, just a few comments.
> 
>
https://codereview.chromium.org/2660213003/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
> (right):
> 
>
https://codereview.chromium.org/2660213003/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:473:
> TRACE_EVENT1("blink", "ResourceFetcher::requestResource", "url",
> This changes behavior, as trace events are scoped to the function they are
> defined in. Could you just move this to the top of requestResource, so it is
> aligned with the SCOPED_BLINK_UMA histogram?

Moved

> 
>
https://codereview.chromium.org/2660213003/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h
(right):
> 
>
https://codereview.chromium.org/2660213003/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h:174: struct
> PrepareRequestReturnValues {
> delete unused struct.

oops!

> 
>
https://codereview.chromium.org/2660213003/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h:179:
Resource*
> prepareRequest(FetchRequest&,
> Please document this method. It is a bit strange for a method to be called
> "prepareRequest" but return a Resource*, so I think a little explanation is
> necessary.

Changed the function so that it makes more sense. I'm not opposed to documenting
it more (and let me know if that's needed), but I hope current form makes more
sense.
Also, took out the static resource creation, as it's not really related to the
"request preparation" phase.

[Charlie Harrison, 2017-01-31 19:00:40]

LGTM even though the Resource*& makes me queasy :)

[Yoav Weiss, 2017-01-31 19:40:02]

On 2017/01/31 19:00:40, Charlie Harrison-slow til Feb3 wrote:
> LGTM even though the Resource*& makes me queasy :)

I'm open to suggestions :D Would a ** be better?

[Nate Chapin, 2017-01-31 19:40:21]

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

[Nate Chapin, 2017-01-31 19:40:22]

Makes sense, just a parameter nitpick:

https://codereview.chromium.org/2660213003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2660213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:460:
Resource*& resource) {
"Resource*&" is dubious. Maybe return an enum:

enum class PrepareRequestPolicy {
  Abort,
  Continue,
  Block
};

...and call resourceForBlockedRequest() is Block is returned?

[Yoav Weiss, 2017-01-31 20:16:24]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 20:16:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yoav Weiss, 2017-01-31 20:44:20]

On 2017/01/31 19:40:22, Nate Chapin wrote:
> Makes sense, just a parameter nitpick:
> 
>
https://codereview.chromium.org/2660213003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
> (right):
> 
>
https://codereview.chromium.org/2660213003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:460:
> Resource*& resource) {
> "Resource*&" is dubious. Maybe return an enum:
> 
> enum class PrepareRequestPolicy {
>   Abort,
>   Continue,
>   Block
> };
> 
> ...and call resourceForBlockedRequest() is Block is returned?

Changed as suggested. PTAL

[Charlie Harrison, 2017-01-31 20:46:46]

still lgtm (non-owners)

[Yoav Weiss, 2017-01-31 20:56:38]

On 2017/01/31 20:46:46, Charlie Harrison-slow til Feb3 wrote:
> still lgtm (non-owners)
Thanks! :)

japhet@ can you take a look?

(since this dir moved out of core, I'm no longer an owner)

[Nate Chapin, 2017-01-31 20:59:01]

LGTM

[commit-bot: I haz the power, 2017-02-01 01:08:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 01:08:37]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2017-02-01 05:28:24]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2017-02-01 05:28:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-01 05:39:00]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485926904015470,
"parent_rev": "d5eb39411c1c5fc9400a711dde056b14ed0a4509", "commit_rev":
"ab7722c427586b8b0b7e0f82e2f3e7d5d67ec46a"}

[commit-bot: I haz the power, 2017-02-01 05:40:08]

Description was changed from

==========
Split requestResource into request preparation and loading

This CL is refactoring of requestResource in order to split the
request preparation phase from the request loading phase. 
This would enable to separate these phases for link prefetch, in order to
delay its actual loading.

BUG=651160
==========

to

==========
Split requestResource into request preparation and loading

This CL is refactoring of requestResource in order to split the
request preparation phase from the request loading phase. 
This would enable to separate these phases for link prefetch, in order to
delay its actual loading.

BUG=651160

Review-Url: https://codereview.chromium.org/2660213003
Cr-Commit-Position: refs/heads/master@{#447454}
Committed:
https://chromium.googlesource.com/chromium/src/+/ab7722c427586b8b0b7e0f82e2f3...
==========

[commit-bot: I haz the power, 2017-02-01 05:40:11]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/ab7722c427586b8b0b7e0f82e2f3...