Make prepareRequest() a separate callback of FetchContext

Make prepareRequest() a separate callback of FetchContext

- Split prepareRequest() from dispatchWillSendRequest() and willStartLoading()
- Move AppCache hook code into prepareRequest(), which looks more relevant
- Rename willStartLoading() to recordLoadingActivity() and makes it take const request

This still doesn't make dispatchWillSendRequest() take
const ResourceRequest& (see crbug.com/632580), but (I think) makes things
slightly more consistent & moves fetching logic out of context into fetcher
abstraction implementors like ResourceFetcher.

BUG=695279, 632580
Review-Url: https://codereview.chromium.org/2747203002
Cr-Commit-Position: refs/heads/master@{#459016}
Committed: https://chromium.googlesource.com/chromium/src/+/059d59531760005f8bfa5a03eab31fb9e3203ff5

[kinuko, 2017-03-14 17:00:32]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 17:01:09]

Dry run: 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-03-14 17:07:59]

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

[commit-bot: I haz the power, 2017-03-14 17:08:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[kinuko, 2017-03-15 02:58:16]

Description was changed from

==========
Make prepareRequest() a separate callback of FetchContext

This still doesn't make dispatchWillSendRequest() take
const ResourceRequest& (see crbug.com/632580), but moves fetching
logic out of context into fetcher abstraction implementors.

BUG=695279
==========

to

==========
Make prepareRequest() a separate callback of FetchContext

- Split prepareRequest() from dispatchWillSendRequest() and willStartLoading()

This still doesn't make dispatchWillSendRequest() take
const ResourceRequest& (see crbug.com/632580), but moves fetching
logic out of context into fetcher abstraction implementors.

BUG=695279
==========

[kinuko, 2017-03-15 02:58:22]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 02:59:08]

Dry run: 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-03-15 03:01:39]

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

[commit-bot: I haz the power, 2017-03-15 03:01:40]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kinuko, 2017-03-15 03:03:23]

Description was changed from

==========
Make prepareRequest() a separate callback of FetchContext

- Split prepareRequest() from dispatchWillSendRequest() and willStartLoading()

This still doesn't make dispatchWillSendRequest() take
const ResourceRequest& (see crbug.com/632580), but moves fetching
logic out of context into fetcher abstraction implementors.

BUG=695279
==========

to

==========
Make prepareRequest() a separate callback of FetchContext

- Split prepareRequest() from dispatchWillSendRequest() and willStartLoading()

This still doesn't make dispatchWillSendRequest() take
const ResourceRequest& (see crbug.com/632580), but moves fetching
logic out of context into fetcher abstraction implementors.

BUG=695279, 632580
==========

[kinuko, 2017-03-15 03:03:28]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[kinuko, 2017-03-15 03:03:35]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-03-15 03:03:47]

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

[kinuko, 2017-03-15 03:04:34]

Description was changed from

==========
Make prepareRequest() a separate callback of FetchContext

- Split prepareRequest() from dispatchWillSendRequest() and willStartLoading()

This still doesn't make dispatchWillSendRequest() take
const ResourceRequest& (see crbug.com/632580), but moves fetching
logic out of context into fetcher abstraction implementors.

BUG=695279, 632580
==========

to

==========
Make prepareRequest() a separate callback of FetchContext

- Split prepareRequest() from dispatchWillSendRequest() and willStartLoading()
  (Don't want to put fetching logic inside FetchContext)
- Move AppCache hook code into prepareRequest(), which looks more relevant

This still doesn't make dispatchWillSendRequest() take
const ResourceRequest& (see crbug.com/632580), but moves fetching
logic out of context into fetcher abstraction implementors.

BUG=695279, 632580
==========

[kinuko, 2017-03-15 03:04:52]

Description was changed from

==========
Make prepareRequest() a separate callback of FetchContext

- Split prepareRequest() from dispatchWillSendRequest() and willStartLoading()
  (Don't want to put fetching logic inside FetchContext)
- Move AppCache hook code into prepareRequest(), which looks more relevant

This still doesn't make dispatchWillSendRequest() take
const ResourceRequest& (see crbug.com/632580), but moves fetching
logic out of context into fetcher abstraction implementors.

BUG=695279, 632580
==========

to

==========
Make prepareRequest() a separate callback of FetchContext

- Split prepareRequest() from dispatchWillSendRequest() and willStartLoading()
- Move AppCache hook code into prepareRequest(), which looks more relevant

This still doesn't make dispatchWillSendRequest() take
const ResourceRequest& (see crbug.com/632580), but moves fetching
logic out of context into fetcher abstraction implementors.

BUG=695279, 632580
==========

[kinuko, 2017-03-15 03:47:32]

Description was changed from

==========
Make prepareRequest() a separate callback of FetchContext

- Split prepareRequest() from dispatchWillSendRequest() and willStartLoading()
- Move AppCache hook code into prepareRequest(), which looks more relevant

This still doesn't make dispatchWillSendRequest() take
const ResourceRequest& (see crbug.com/632580), but moves fetching
logic out of context into fetcher abstraction implementors.

BUG=695279, 632580
==========

to

==========
Make prepareRequest() a separate callback of FetchContext

- Split prepareRequest() from dispatchWillSendRequest() and willStartLoading()
- Move AppCache hook code into prepareRequest(), which looks more relevant
- Rename willStartLoading() to recordLoadingActivity() and makes it take const
request

This still doesn't make dispatchWillSendRequest() take
const ResourceRequest& (see crbug.com/632580), but moves fetching
logic out of context into fetcher abstraction implementors.

BUG=695279, 632580
==========

[kinuko, 2017-03-15 04:46:47]

kinuko@chromium.org changed reviewers:
+ japhet@chromium.org, yoav@yoav.ws

[commit-bot: I haz the power, 2017-03-15 04:46:54]

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

[commit-bot: I haz the power, 2017-03-15 04:46:55]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-03-15 04:48:15]

Description was changed from

==========
Make prepareRequest() a separate callback of FetchContext

- Split prepareRequest() from dispatchWillSendRequest() and willStartLoading()
- Move AppCache hook code into prepareRequest(), which looks more relevant
- Rename willStartLoading() to recordLoadingActivity() and makes it take const
request

This still doesn't make dispatchWillSendRequest() take
const ResourceRequest& (see crbug.com/632580), but moves fetching
logic out of context into fetcher abstraction implementors.

BUG=695279, 632580
==========

to

==========
Make prepareRequest() a separate callback of FetchContext

- Split prepareRequest() from dispatchWillSendRequest() and willStartLoading()
- Move AppCache hook code into prepareRequest(), which looks more relevant
- Rename willStartLoading() to recordLoadingActivity() and makes it take const
request

This still doesn't make dispatchWillSendRequest() take
const ResourceRequest& (see crbug.com/632580), but (I think) makes things
slightly more consistent & moves fetching logic out of context into fetcher
abstraction implementors like ResourceFetcher.

BUG=695279, 632580
==========

[kinuko, 2017-03-15 04:49:18]

kinuko@chromium.org changed reviewers:
+ hiroshige@chromium.org

[kinuko, 2017-03-15 04:49:19]

I think this is slightly more consistent, what do you think?

https://codereview.chromium.org/2747203002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/FetchContext.h (right):

https://codereview.chromium.org/2747203002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/FetchContext.h:94: // This
internally dispatches WebFrameClient::willSendRequest hook
'and' is missing before 'hook'

[kinuko, 2017-03-22 14:08:39]

Friendly ping. This is similar to Nate's
https://codereview.chromium.org/2203613003/ but doesn't make willSendRequest
take const-ref yet (no changes in inspector), basically mostly
refactoring/relocating only.

[Nate Chapin, 2017-03-22 19:12:59]

lgtm

https://codereview.chromium.org/2747203002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2747203002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:451: // This is an
initial requests, not for redirects.
This comment doesn't add much that the if() statement doesn't already state.
Maybe:
// Progress doesn't care about redirects, only notify it when an
// initial request is sent.
... or something like that?

https://codereview.chromium.org/2747203002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/FetchContext.h (right):

https://codereview.chromium.org/2747203002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/FetchContext.h:94: // This
internally dispatches WebFrameClient::willSendRequest hook
On 2017/03/15 04:49:19, kinuko wrote:
> 'and' is missing before 'hook'

Nit: 'hooks'

[kinuko, 2017-03-23 05:12:17]

Thanks!

https://codereview.chromium.org/2747203002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2747203002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:451: // This is an
initial requests, not for redirects.
On 2017/03/22 19:12:59, Nate Chapin wrote:
> This comment doesn't add much that the if() statement doesn't already state.
> Maybe:
> // Progress doesn't care about redirects, only notify it when an
> // initial request is sent.
> ... or something like that?

Sounds good, done.

https://codereview.chromium.org/2747203002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/FetchContext.h (right):

https://codereview.chromium.org/2747203002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/FetchContext.h:94: // This
internally dispatches WebFrameClient::willSendRequest hook
On 2017/03/22 19:12:59, Nate Chapin wrote:
> On 2017/03/15 04:49:19, kinuko wrote:
> > 'and' is missing before 'hook'
> 
> Nit: 'hooks'

Done.

[kinuko, 2017-03-23 05:17:21]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2017-03-23 05:17:21]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/2747203002/#ps60001
(title: "addressed comments")

[commit-bot: I haz the power, 2017-03-23 05:17:35]

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-03-23 05:33:00]

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

[commit-bot: I haz the power, 2017-03-23 05:33:02]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[kinuko, 2017-03-23 06:03:54]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 06:04:09]

Dry run: 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-03-23 07:00:03]

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

[commit-bot: I haz the power, 2017-03-23 07:00:05]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-03-23 07:34:44]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2017-03-23 07:35:02]

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-03-23 07:42:35]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490254484581090,
"parent_rev": "57f4387cd58afa6949c3fa158271d49690ca8d81", "commit_rev":
"059d59531760005f8bfa5a03eab31fb9e3203ff5"}

[commit-bot: I haz the power, 2017-03-23 07:43:34]

Description was changed from

==========
Make prepareRequest() a separate callback of FetchContext

- Split prepareRequest() from dispatchWillSendRequest() and willStartLoading()
- Move AppCache hook code into prepareRequest(), which looks more relevant
- Rename willStartLoading() to recordLoadingActivity() and makes it take const
request

This still doesn't make dispatchWillSendRequest() take
const ResourceRequest& (see crbug.com/632580), but (I think) makes things
slightly more consistent & moves fetching logic out of context into fetcher
abstraction implementors like ResourceFetcher.

BUG=695279, 632580
==========

to

==========
Make prepareRequest() a separate callback of FetchContext

- Split prepareRequest() from dispatchWillSendRequest() and willStartLoading()
- Move AppCache hook code into prepareRequest(), which looks more relevant
- Rename willStartLoading() to recordLoadingActivity() and makes it take const
request

This still doesn't make dispatchWillSendRequest() take
const ResourceRequest& (see crbug.com/632580), but (I think) makes things
slightly more consistent & moves fetching logic out of context into fetcher
abstraction implementors like ResourceFetcher.

BUG=695279, 632580

Review-Url: https://codereview.chromium.org/2747203002
Cr-Commit-Position: refs/heads/master@{#459016}
Committed:
https://chromium.googlesource.com/chromium/src/+/059d59531760005f8bfa5a03eab3...
==========

[commit-bot: I haz the power, 2017-03-23 07:43:36]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/059d59531760005f8bfa5a03eab3...