Move FrameFetchContext::dispatchDidLoadResourceFromMemoryCache logic into ResourceFetcher

Move FrameFetchContext::dispatchDidLoadResourceFromMemoryCache logic into ResourceFetcher

Motivation: want to make FrameFetchContext thinner that only has the
code that hides Frame abstraction

BUG=695279
Review-Url: https://codereview.chromium.org/2751143003
Cr-Commit-Position: refs/heads/master@{#460676}
Committed: https://chromium.googlesource.com/chromium/src/+/918293741a54580488930933602b2d4984378b97

[kinuko, 2017-03-16 05:29:10]

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

[commit-bot: I haz the power, 2017-03-16 05:29:30]

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-16 05:31:29]

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

[commit-bot: I haz the power, 2017-03-16 05:31:30]

Dry run: Try jobs failed on following builders:
  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-16 05:33:04]

Description was changed from

==========
Move FrameFetchContext::dispatchDidLoadResourceFromMemoryCache logic into
ResourceFetcher

Motivation: want to make FrameFetchContext thinner that only has the
code that hides Frame abstraction

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

to

==========
Move FrameFetchContext::dispatchDidLoadResourceFromMemoryCache logic into
ResourceFetcher

Motivation: want to make FrameFetchContext thinner that only has the
code that hides Frame abstraction

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

[kinuko, 2017-03-16 07:05:47]

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

[commit-bot: I haz the power, 2017-03-16 07:06:01]

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-16 08:21:26]

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

[commit-bot: I haz the power, 2017-03-16 08:21:27]

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_...)

[kinuko, 2017-03-16 09:21:19]

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

[commit-bot: I haz the power, 2017-03-16 09:21:32]

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-16 10:47:04]

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

[commit-bot: I haz the power, 2017-03-16 10:47:05]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kinuko, 2017-03-20 06:37:31]

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

[commit-bot: I haz the power, 2017-03-20 06:37:41]

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-20 06:46:23]

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

[commit-bot: I haz the power, 2017-03-20 06:46:23]

Dry run: Try jobs failed on following builders:
  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-22 08:10:42]

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

[commit-bot: I haz the power, 2017-03-22 08:11:00]

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-22 09:52:36]

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

[commit-bot: I haz the power, 2017-03-22 09:52:37]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-03-22 14:12:04]

kinuko@chromium.org changed reviewers:
+ japhet@chromium.org, yhirano@chromium.org

[kinuko, 2017-03-22 14:13:26]

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

[kinuko, 2017-03-22 14:13:27]

What do you think?  Code moving only, similar change to crrev.com/2747203002/

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

https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:468: 
Code below is basically same as what dispatchDidResponseInternal was doing

https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:556:
resourceResponse);
Moved most of the code in this method out of FetchContext into ResourceFetcher

[Nate Chapin, 2017-03-22 19:02:02]

Awesome!

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

https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:464:
probe::markResourceAsCached(frame(), identifier);
Is it important to do this after dispatchDidLoadResourceFromMemoryCache() and
dispatchWillSendRequest()? If not, can we leave it in
dispatchDidLoadResourceFromMemoryCache()?

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

https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/FetchContext.h:59: enum class
ResourceLoadStartType { kNotFromMemoryCache, kFromMemoryCache };
Nit: it's not really "LoadStart" for the kNotFromMemoryCache case at response
time. Maybe we could have a ResourceResponseType enum just for
FetchContext::dispatchDidReceiveResponse() and just use the
ResourceFetcher::RevalidationPolicy for ResourceFetcher::requestLoadStarted()?

I think the only reason we had a special enum for requestLoadStarted() instead
of using the RevalidationPolicy was that requestLoadStarted() used to be public
and called from outside ResourceFetcher. But it's private now, so I don't think
there's a good reason not to just use RevalidationPolicy.

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

https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:335: const
FetchRequest& request) {
Nit: take const ResourceRequest& instead of const FetchRequest&, since this
function doesn't use anything else from the FetchRequest.

https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:337:
identifier, request.resourceRequest(), resource->response());
We're sending a different ResoruceRequest for
dispatchDidLoadResourceFromMemoryCache() and dispatchWillSendRequest(). Can we
send the same one? I don't think it matters at the moment, but it might cause
confusion later.

[yhirano, 2017-03-23 12:07:23]

lgtm if Nate's comments are addressed.

[kinuko, 2017-03-24 13:47:14]

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

[commit-bot: I haz the power, 2017-03-24 13:47:36]

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-24 13:51:51]

Thanks!  Updated.

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

https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:464:
probe::markResourceAsCached(frame(), identifier);
On 2017/03/22 19:02:02, Nate Chapin wrote:
> Is it important to do this after dispatchDidLoadResourceFromMemoryCache() and
> dispatchWillSendRequest()? If not, can we leave it in
> dispatchDidLoadResourceFromMemoryCache()?

I tried it in my initial patch, but if we reverse the order inspector's
Network.requestServedFromCache notification is not dispatched at all and it
breaks multiple tests.  It looks willSendRequest needs to precede because this
probe method only takes identifier and inspector can't find associated request
otherwise.

(Needing to have this line here feels a bit unfortunate though.  I added a
comment to note the ordering issue)

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

https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/FetchContext.h:59: enum class
ResourceLoadStartType { kNotFromMemoryCache, kFromMemoryCache };
On 2017/03/22 19:02:02, Nate Chapin wrote:
> Nit: it's not really "LoadStart" for the kNotFromMemoryCache case at response
> time. Maybe we could have a ResourceResponseType enum just for
> FetchContext::dispatchDidReceiveResponse() and just use the
> ResourceFetcher::RevalidationPolicy for ResourceFetcher::requestLoadStarted()?
> 
> I think the only reason we had a special enum for requestLoadStarted() instead
> of using the RevalidationPolicy was that requestLoadStarted() used to be
public
> and called from outside ResourceFetcher. But it's private now, so I don't
think
> there's a good reason not to just use RevalidationPolicy.

Good point, done.

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

https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:335: const
FetchRequest& request) {
On 2017/03/22 19:02:02, Nate Chapin wrote:
> Nit: take const ResourceRequest& instead of const FetchRequest&, since this
> function doesn't use anything else from the FetchRequest.

Done.

https://codereview.chromium.org/2751143003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:337:
identifier, request.resourceRequest(), resource->response());
On 2017/03/22 19:02:02, Nate Chapin wrote:
> We're sending a different ResoruceRequest for
> dispatchDidLoadResourceFromMemoryCache() and dispatchWillSendRequest(). Can we
> send the same one? I don't think it matters at the moment, but it might cause
> confusion later.

Done.

[commit-bot: I haz the power, 2017-03-24 15:34:51]

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

[commit-bot: I haz the power, 2017-03-24 15:34:52]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2017-03-28 21:50:02]

LGTM, sorry for the delay.

[kinuko, 2017-03-29 00:08:45]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2017-03-29 00:08:46]

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

[commit-bot: I haz the power, 2017-03-29 00:09:48]

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-29 00:24:43]

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

[commit-bot: I haz the power, 2017-03-29 00:24:44]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[kinuko, 2017-03-29 05:50:54]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2017-03-29 05:51:20]

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-29 06:59:45]

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

[commit-bot: I haz the power, 2017-03-29 06:59:47]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[kinuko, 2017-03-30 04:39:22]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2017-03-30 04:39:44]

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-30 05:54:33]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490848762091240,
"parent_rev": "1a21f83e0deacdee7ec43cf1f2b93e47d51438b3", "commit_rev":
"918293741a54580488930933602b2d4984378b97"}

[commit-bot: I haz the power, 2017-03-30 05:56:13]

Description was changed from

==========
Move FrameFetchContext::dispatchDidLoadResourceFromMemoryCache logic into
ResourceFetcher

Motivation: want to make FrameFetchContext thinner that only has the
code that hides Frame abstraction

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

to

==========
Move FrameFetchContext::dispatchDidLoadResourceFromMemoryCache logic into
ResourceFetcher

Motivation: want to make FrameFetchContext thinner that only has the
code that hides Frame abstraction

BUG=695279

Review-Url: https://codereview.chromium.org/2751143003
Cr-Commit-Position: refs/heads/master@{#460676}
Committed:
https://chromium.googlesource.com/chromium/src/+/918293741a54580488930933602b...
==========

[commit-bot: I haz the power, 2017-03-30 05:56:14]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/918293741a54580488930933602b...