FrameFetchContext should handle all FrameLoadTypes

FrameFetchContext should handle all FrameLoadTypes

Now, determineWebCachePolicy() does not care for FrameLoadTypes
that are used in edge cases.

This patch adopts the corresponding logic for such types.

BUG=704431
Review-Url: https://codereview.chromium.org/2775533003
Cr-Commit-Position: refs/heads/master@{#463579}
Committed: https://chromium.googlesource.com/chromium/src/+/f2d2a76a59caaf3065c36a959ce0fa7b7ad42280

[Takashi Toyoshima, 2017-03-23 06:19:31]

Description was changed from

==========
FrameFetchContext should handle FrameLoadTypes for site isolation correctly

Now, determineWebCachePolicy() does not care for FrameLoadTypes that are
used when browser side navigation is enabled.

This patch adopts the corresponding logic for such types.

BUG=704431
==========

to

==========
FrameFetchContext should handle FrameLoadTypes for site isolation correctly

Now, determineWebCachePolicy() does not care for FrameLoadTypes that are
used when browser side navigation is enabled.

This patch adopts the corresponding logic for such types.

BUG=704431
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Takashi Toyoshima, 2017-03-23 06:19:49]

Description was changed from

==========
FrameFetchContext should handle FrameLoadTypes for site isolation correctly

Now, determineWebCachePolicy() does not care for FrameLoadTypes that are
used when browser side navigation is enabled.

This patch adopts the corresponding logic for such types.

BUG=704431
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
FrameFetchContext should handle FrameLoadTypes for site isolation

Now, determineWebCachePolicy() does not care for FrameLoadTypes that are
used when browser side navigation is enabled.

This patch adopts the corresponding logic for such types.

BUG=704431
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Takashi Toyoshima, 2017-03-23 06:20:14]

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

[Takashi Toyoshima, 2017-03-23 06:20:30]

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

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

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

[Takashi Toyoshima, 2017-03-23 06:30:31]

cc: clamy

this change is not related to the topic I talked in the last paris-tokyo sync,
but just in case.

[commit-bot: I haz the power, 2017-03-23 06:50:55]

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

[commit-bot: I haz the power, 2017-03-23 06:50:56]

Dry run: 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_...)

[yhirano, 2017-03-23 12:08:59]

Is there any tests?

[Takashi Toyoshima, 2017-03-23 12:13:45]

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

[Takashi Toyoshima, 2017-03-23 12:13:45]

Let me add creis@ because I'm actually not sure what should be tested for these
extra frame types here.

[Charlie Reis, 2017-03-23 16:12:40]

Sorry, I'm a bit confused here.  The change itself looks like it's resolving
some TODOs from the reload project, but the CL description talks about site
isolation and browser-side navigation/PlzNavigate.  (Note that those are two
different things and PlzNavigate isn't on by default, so I'm not sure what
you're referring to.)  Can you clarify the CL description?

For the change itself (and questions about tests), Nate's probably a better
reviewer than me, since I'm not that familiar with this area of Blink.  (I can
kind of see how the first part might be ok, but I don't understand the
FrameLoadTypeInitialHistoryLoad part.)

[Takashi Toyoshima, 2017-03-24 04:15:36]

Sorry for lack of explaining background for this change.
These TODOs were just added by https://codereview.chromium.org/1867493003 since
I just noticed it while working on the change, and these types are originally
not taken care at all here.

Anyway, I will wait for Nate's comments.

[Charlie Reis, 2017-03-24 05:25:42]

On 2017/03/24 04:15:36, Takashi Toyoshima wrote:
> Sorry for lack of explaining background for this change.
> These TODOs were just added by https://codereview.chromium.org/1867493003
since
> I just noticed it while working on the change, and these types are originally
> not taken care at all here.
> 
> Anyway, I will wait for Nate's comments.

Ok, thanks.  If the CL isn't related to site isolation or PlzNavigate (i.e.,
browser-side navigation), then let's remove those references from the CL
description.

[Nate Chapin, 2017-03-28 21:47:16]

lgtm

[Takashi Toyoshima, 2017-03-30 09:12:46]

Description was changed from

==========
FrameFetchContext should handle FrameLoadTypes for site isolation

Now, determineWebCachePolicy() does not care for FrameLoadTypes that are
used when browser side navigation is enabled.

This patch adopts the corresponding logic for such types.

BUG=704431
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
FrameFetchContext should handle all FrameLoadTypes

Now, determineWebCachePolicy() does not care for FrameLoadTypes
that are used in edge cases.

This patch adopts the corresponding logic for such types.

BUG=704431
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Takashi Toyoshima, 2017-04-03 09:32:20]

Description was changed from

==========
FrameFetchContext should handle all FrameLoadTypes

Now, determineWebCachePolicy() does not care for FrameLoadTypes
that are used in edge cases.

This patch adopts the corresponding logic for such types.

BUG=704431
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
FrameFetchContext should handle all FrameLoadTypes

Now, determineWebCachePolicy() does not care for FrameLoadTypes
that are used in edge cases.

This patch adopts the corresponding logic for such types.

BUG=704431
==========

[Takashi Toyoshima, 2017-04-03 09:37:20]

I updated comments for WebFrameLoadType.h in Patch Set 2.
If you are familiar to WebFrameLoadType in terms of navigation, please review my
comments.

Nate, and core/loader reviewers:
I would add some unit tests to the Patch Set 3. I'm still checking real use
cases of these types so that my tests can be to the point. So I will ask another
review for core/loader when I'm ready.

[Charlie Reis, 2017-04-03 21:00:03]

creis@chromium.org changed reviewers:
+ clamy@chromium.org
- creis@chromium.org

[Charlie Reis, 2017-04-03 21:00:03]

Sounds like clamy@ is a better reviewer for the comments than me, so I'll move
myself to CC.

[clamy, 2017-04-06 16:13:34]

Thanks! lgtm % nits.

https://codereview.chromium.org/2775533003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameLoadType.h (right):

https://codereview.chromium.org/2775533003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:11: // TODO(clamy, toyoshim):
Now WebFrameLoadType represents multiple concepts
nit: s/Now/Currently

https://codereview.chromium.org/2775533003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:39: // WebHistoryCommitType,
but in terms of cache policy, it should work in the
nit: s/WebHistoryCommitType/the WebHistoryCommitType

https://codereview.chromium.org/2775533003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:40: // same manner with
Standard and BackFoward respectively.
nit: s/with/as

https://codereview.chromium.org/2775533003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:41: // ReplaceCurrentItem is
used to determine if current navigation should replace
nit: s/if/if the

https://codereview.chromium.org/2775533003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:42: // current history item,
but in terms of cache policy, it should work in the
nit: s/current/the current

https://codereview.chromium.org/2775533003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:43: // same manner with
Standard.
nit: s/with/as

[kinuko, 2017-04-07 00:25:11]

Thanks for adding the comment, that's helpful. (lgtm/2, fwiw)

[Takashi Toyoshima, 2017-04-11 07:29:40]

https://codereview.chromium.org/2775533003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameLoadType.h (right):

https://codereview.chromium.org/2775533003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:11: // TODO(clamy, toyoshim):
Now WebFrameLoadType represents multiple concepts
On 2017/04/06 16:13:33, clamy (slow) wrote:
> nit: s/Now/Currently

Done.

https://codereview.chromium.org/2775533003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:39: // WebHistoryCommitType,
but in terms of cache policy, it should work in the
On 2017/04/06 16:13:33, clamy (slow) wrote:
> nit: s/WebHistoryCommitType/the WebHistoryCommitType

Done.

https://codereview.chromium.org/2775533003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:40: // same manner with
Standard and BackFoward respectively.
On 2017/04/06 16:13:33, clamy (slow) wrote:
> nit: s/with/as

Done.

https://codereview.chromium.org/2775533003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:41: // ReplaceCurrentItem is
used to determine if current navigation should replace
On 2017/04/06 16:13:33, clamy (slow) wrote:
> nit: s/if/if the

Done.

https://codereview.chromium.org/2775533003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:42: // current history item,
but in terms of cache policy, it should work in the
On 2017/04/06 16:13:33, clamy (slow) wrote:
> nit: s/current/the current

Done.

https://codereview.chromium.org/2775533003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:43: // same manner with
Standard.
On 2017/04/06 16:13:33, clamy (slow) wrote:
> nit: s/with/as

Done.

[Takashi Toyoshima, 2017-04-11 07:30:05]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2017-04-11 07:30:05]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
kinuko@chromium.org, clamy@chromium.org
Link to the patchset: https://codereview.chromium.org/2775533003/#ps80001
(title: "adopt great renaming in comments")

[commit-bot: I haz the power, 2017-04-11 07:30:15]

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-04-11 08:49:24]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491895805012990,
"parent_rev": "d8c46c166645ad80352881e692745cd2c30aa24f", "commit_rev":
"f2d2a76a59caaf3065c36a959ce0fa7b7ad42280"}

[commit-bot: I haz the power, 2017-04-11 08:50:21]

Description was changed from

==========
FrameFetchContext should handle all FrameLoadTypes

Now, determineWebCachePolicy() does not care for FrameLoadTypes
that are used in edge cases.

This patch adopts the corresponding logic for such types.

BUG=704431
==========

to

==========
FrameFetchContext should handle all FrameLoadTypes

Now, determineWebCachePolicy() does not care for FrameLoadTypes
that are used in edge cases.

This patch adopts the corresponding logic for such types.

BUG=704431

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

[commit-bot: I haz the power, 2017-04-11 08:50:22]

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