Pull to refresh: Use new reload type RELOAD_MAIN_RESOURCE

Pull to refresh: Use new reload type RELOAD_MAIN_RESOURCE

Current implementation for the new behavior is a little tricky, and
should be replaced with a right fix, this change.

This change introduces new reload type RELOAD_MAIN_RESOURCE for content.
We use this type for IPC, and convert it to FrameLoadTypeReloadMainResource
to ask a right kind of reload to Blink.

FrameLoadTypeReloadMainResource was previously known as FrameLoadTypeSame.
The type didn't explain the behavior, but it meant we validate the main
resource and follows protocols for sub resources.

Even with this patch, sub resources will be validated on preloading
for some reasons if DevTools is open. It should be fixed by followup changes.

BUG=558829
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/bada0dc96fe81de27879058e78225e1747b2d57f
Cr-Commit-Position: refs/heads/master@{#394336}

[Takashi Toyoshima, 2016-05-13 06:54:51]

Description was changed from

==========
Pull to refresh: Use new reload type RELOAD_MAIN_RESOURCE

Current implementation for the new behavior is a little tricky, and
should be replaced with a right fix, this change.

This change introduces new reload type RELOAD_MAIN_RESOURCE for content.
We use this type for IPC, and convert it to FrameLoadTypeReloadMainResource
to ask a right kind of reload to Blink.

FrameLoadTypeReloadMainResource was previously known as FrameLoadTypeSame.
The type didn't explain the behavior, but it meant we validate the main
resource and follows protocols for sub resources.

BUG=558829
==========

to

==========
Pull to refresh: Use new reload type RELOAD_MAIN_RESOURCE

Current implementation for the new behavior is a little tricky, and
should be replaced with a right fix, this change.

This change introduces new reload type RELOAD_MAIN_RESOURCE for content.
We use this type for IPC, and convert it to FrameLoadTypeReloadMainResource
to ask a right kind of reload to Blink.

FrameLoadTypeReloadMainResource was previously known as FrameLoadTypeSame.
The type didn't explain the behavior, but it meant we validate the main
resource and follows protocols for sub resources.

BUG=558829
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Takashi Toyoshima, 2016-05-13 07:25:15]

Description was changed from

==========
Pull to refresh: Use new reload type RELOAD_MAIN_RESOURCE

Current implementation for the new behavior is a little tricky, and
should be replaced with a right fix, this change.

This change introduces new reload type RELOAD_MAIN_RESOURCE for content.
We use this type for IPC, and convert it to FrameLoadTypeReloadMainResource
to ask a right kind of reload to Blink.

FrameLoadTypeReloadMainResource was previously known as FrameLoadTypeSame.
The type didn't explain the behavior, but it meant we validate the main
resource and follows protocols for sub resources.

BUG=558829
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Pull to refresh: Use new reload type RELOAD_MAIN_RESOURCE

Current implementation for the new behavior is a little tricky, and
should be replaced with a right fix, this change.

This change introduces new reload type RELOAD_MAIN_RESOURCE for content.
We use this type for IPC, and convert it to FrameLoadTypeReloadMainResource
to ask a right kind of reload to Blink.

FrameLoadTypeReloadMainResource was previously known as FrameLoadTypeSame.
The type didn't explain the behavior, but it meant we validate the main
resource and follows protocols for sub resources.

Even with this patch, sub resources will be validated if DevTools is open.
This should be fixed by following changes.

BUG=558829
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Takashi Toyoshima, 2016-05-13 10:23:59]

Description was changed from

==========
Pull to refresh: Use new reload type RELOAD_MAIN_RESOURCE

Current implementation for the new behavior is a little tricky, and
should be replaced with a right fix, this change.

This change introduces new reload type RELOAD_MAIN_RESOURCE for content.
We use this type for IPC, and convert it to FrameLoadTypeReloadMainResource
to ask a right kind of reload to Blink.

FrameLoadTypeReloadMainResource was previously known as FrameLoadTypeSame.
The type didn't explain the behavior, but it meant we validate the main
resource and follows protocols for sub resources.

Even with this patch, sub resources will be validated if DevTools is open.
This should be fixed by following changes.

BUG=558829
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Pull to refresh: Use new reload type RELOAD_MAIN_RESOURCE

Current implementation for the new behavior is a little tricky, and
should be replaced with a right fix, this change.

This change introduces new reload type RELOAD_MAIN_RESOURCE for content.
We use this type for IPC, and convert it to FrameLoadTypeReloadMainResource
to ask a right kind of reload to Blink.

FrameLoadTypeReloadMainResource was previously known as FrameLoadTypeSame.
The type didn't explain the behavior, but it meant we validate the main
resource and follows protocols for sub resources.

Even with this patch, sub resources will be validated on preloading
for some reasons if DevTools is open. It should be fixed by followup changes.

BUG=558829
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Takashi Toyoshima, 2016-05-13 10:24:41]

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

[Takashi Toyoshima, 2016-05-13 10:25:20]

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

[Takashi Toyoshima, 2016-05-13 10:25:40]

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

[Takashi Toyoshima, 2016-05-13 10:43:24]

kinuko@ for blink loader, creis@ for content side navigation, palmer@ for
frame_message_enums.h.
Could you take a look?

[Charlie Reis, 2016-05-13 23:27:54]

content/ LGTM, apart from the question below.

https://codereview.chromium.org/1975753006/diff/40001/content/public/common/c...
File content/public/common/content_features.cc (right):

https://codereview.chromium.org/1975753006/diff/40001/content/public/common/c...
content/public/common/content_features.cc:52:
"NonValidatingReloadOnRefreshContentV2",
Not sure I understand this change.  Is one of the other reviewers familiar with
this feature name and why it's changing?  If so, I'll defer.

[kinuko, 2016-05-15 14:03:56]

lgtm

https://codereview.chromium.org/1975753006/diff/40001/content/public/common/c...
File content/public/common/content_features.cc (right):

https://codereview.chromium.org/1975753006/diff/40001/content/public/common/c...
content/public/common/content_features.cc:52:
"NonValidatingReloadOnRefreshContentV2",
On 2016/05/13 23:27:53, Charlie Reis (slow to reply) wrote:
> Not sure I understand this change.  Is one of the other reviewers familiar
with
> this feature name and why it's changing?  If so, I'll defer.

Guessed the slight renaming is to run a new/different Finch experiment for this
new change? (Toyoshim, could you clarify your intention?)

https://codereview.chromium.org/1975753006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoaderTypes.h (right):

https://codereview.chromium.org/1975753006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoaderTypes.h:38:
FrameLoadTypeReloadMainResource, // used in loading same URL again
nit: Maybe we could start adding more explicit/verbose comment for this type

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

https://codereview.chromium.org/1975753006/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:18: ReloadMainResource, // used
in loading same URL again
ditto

[Takashi Toyoshima, 2016-05-16 03:49:02]

https://codereview.chromium.org/1975753006/diff/40001/content/public/common/c...
File content/public/common/content_features.cc (right):

https://codereview.chromium.org/1975753006/diff/40001/content/public/common/c...
content/public/common/content_features.cc:52:
"NonValidatingReloadOnRefreshContentV2",
Yes. I'm replacing actual implementation for the pull to refresh in this CL. So,
by changing this study name, I want to double-check if the implementation is
equivalent before and after this CL.

If I don't change this name, it isn't impossible but very difficult to get
separate data for each implementation.

[Takashi Toyoshima, 2016-05-16 04:39:38]

I added clearer comments in PS4, but Initial* may need more comments from
someone know better on out-of-process iframes. I'd leave TODO for that.

https://codereview.chromium.org/1975753006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoaderTypes.h (right):

https://codereview.chromium.org/1975753006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoaderTypes.h:38:
FrameLoadTypeReloadMainResource, // used in loading same URL again
OK. So probably we do not have the same explanation in two files.
I'll add detail comments in public/web/WebFrameLoadType.h, and want to just say
"See WebFrameLoadType in ..." here.

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

https://codereview.chromium.org/1975753006/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:18: ReloadMainResource, // used
in loading same URL again
On 2016/05/15 14:03:56, kinuko wrote:
> ditto

Done.

[Takashi Toyoshima, 2016-05-16 04:42:14]

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

https://codereview.chromium.org/1975753006/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:19: // BackFoward:
Hum... wording looks bad. Sorry, let me rephrasing this.

[Takashi Toyoshima, 2016-05-16 04:53:08]

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

https://codereview.chromium.org/1975753006/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:19: // BackForward:
I fixed typos, and tried to improve some descriptions.

[Charlie Reis, 2016-05-16 21:25:39]

LGTM.

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

https://codereview.chromium.org/1975753006/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:14: // TODO: Add better
explanations for InitialInChildFrame and InitialHistoryLoad.
nit: The convention is TODO(username).  It's not a requirement that you fix it,
just an indication that you know about the TODO so that people can ask you about
it.  (For example, I'm not sure what improvements you're looking for.)

[Takashi Toyoshima, 2016-05-17 03:28:08]

Thanks!
I'm pinging palmer@ for the final approval to submit this.

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

https://codereview.chromium.org/1975753006/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:14: // TODO: Add better
explanations for InitialInChildFrame and InitialHistoryLoad.
OK. I wasn't expert for them and thought you or clamy know better descriptions
for them. If my comments were not wrong and good enough for you, probably I can
just remove the TODO.

[palmer, 2016-05-17 18:09:11]

LGTM.

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

https://codereview.chromium.org/1975753006/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:23: //   Revalidates cached
entries even if the entries are fresh. Used in standard
Nit: I wouldn't use "standard" here, because it sounds like it might mean or be
related to |Standard| (the enum value). Up on line 18 you used the word "usual";
you could use that here, too.

    Used in usual reload.

You could also say "common" or "likely", if you prefer. But whatever you choose,
use the same word throughout.

https://codereview.chromium.org/1975753006/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:26: //   Revalidates a cached
entry for the main resource if exists, but follows
Nit: "...if one exists, ..."

https://codereview.chromium.org/1975753006/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:27: //   protocols for other
sub resources. Blink internally uses this for the same
Nit: I think the most common spelling is "subresources". Searching the code, I
see many instances of "subresource" and "sub-resource" (and |SUB_RESOURCE|), but
few instances of "sub resource".

This probably doesn't matter very much. :)

https://codereview.chromium.org/1975753006/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:31: //   Same with Standard,
but replaces current navigation entry in the history.
Nit: with -> as

https://codereview.chromium.org/1975753006/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:33: //   Used in the first load
for a sub frame.
Nit: "sub frame" is very rare in the code; "subframe" and "sub-frame" are very
common.

[Takashi Toyoshima, 2016-05-18 03:28:03]

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

https://codereview.chromium.org/1975753006/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:23: //   Revalidates cached
entries even if the entries are fresh. Used in standard
Thanks. That makes much sense.

https://codereview.chromium.org/1975753006/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:26: //   Revalidates a cached
entry for the main resource if exists, but follows
On 2016/05/17 18:09:11, palmer wrote:
> Nit: "...if one exists, ..."

Done.

https://codereview.chromium.org/1975753006/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:27: //   protocols for other
sub resources. Blink internally uses this for the same
On 2016/05/17 18:09:11, palmer wrote:
> Nit: I think the most common spelling is "subresources". Searching the code, I
> see many instances of "subresource" and "sub-resource" (and |SUB_RESOURCE|),
but
> few instances of "sub resource".
> 
> This probably doesn't matter very much. :)

Done.

https://codereview.chromium.org/1975753006/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:31: //   Same with Standard,
but replaces current navigation entry in the history.
On 2016/05/17 18:09:11, palmer wrote:
> Nit: with -> as

Done.

https://codereview.chromium.org/1975753006/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameLoadType.h:33: //   Used in the first load
for a sub frame.
On 2016/05/17 18:09:11, palmer wrote:
> Nit: "sub frame" is very rare in the code; "subframe" and "sub-frame" are very
> common.

Done.

[Takashi Toyoshima, 2016-05-18 03:30:44]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2016-05-18 03:30:45]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
palmer@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1975753006/#ps100001
(title: "review #14 and #16")

[commit-bot: I haz the power, 2016-05-18 03:31:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1975753006/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1975753006/100001

[commit-bot: I haz the power, 2016-05-18 05:35:59]

Description was changed from

==========
Pull to refresh: Use new reload type RELOAD_MAIN_RESOURCE

Current implementation for the new behavior is a little tricky, and
should be replaced with a right fix, this change.

This change introduces new reload type RELOAD_MAIN_RESOURCE for content.
We use this type for IPC, and convert it to FrameLoadTypeReloadMainResource
to ask a right kind of reload to Blink.

FrameLoadTypeReloadMainResource was previously known as FrameLoadTypeSame.
The type didn't explain the behavior, but it meant we validate the main
resource and follows protocols for sub resources.

Even with this patch, sub resources will be validated on preloading
for some reasons if DevTools is open. It should be fixed by followup changes.

BUG=558829
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Pull to refresh: Use new reload type RELOAD_MAIN_RESOURCE

Current implementation for the new behavior is a little tricky, and
should be replaced with a right fix, this change.

This change introduces new reload type RELOAD_MAIN_RESOURCE for content.
We use this type for IPC, and convert it to FrameLoadTypeReloadMainResource
to ask a right kind of reload to Blink.

FrameLoadTypeReloadMainResource was previously known as FrameLoadTypeSame.
The type didn't explain the behavior, but it meant we validate the main
resource and follows protocols for sub resources.

Even with this patch, sub resources will be validated on preloading
for some reasons if DevTools is open. It should be fixed by followup changes.

BUG=558829
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-05-18 05:36:01]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-18 05:37:07]

Description was changed from

==========
Pull to refresh: Use new reload type RELOAD_MAIN_RESOURCE

Current implementation for the new behavior is a little tricky, and
should be replaced with a right fix, this change.

This change introduces new reload type RELOAD_MAIN_RESOURCE for content.
We use this type for IPC, and convert it to FrameLoadTypeReloadMainResource
to ask a right kind of reload to Blink.

FrameLoadTypeReloadMainResource was previously known as FrameLoadTypeSame.
The type didn't explain the behavior, but it meant we validate the main
resource and follows protocols for sub resources.

Even with this patch, sub resources will be validated on preloading
for some reasons if DevTools is open. It should be fixed by followup changes.

BUG=558829
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Pull to refresh: Use new reload type RELOAD_MAIN_RESOURCE

Current implementation for the new behavior is a little tricky, and
should be replaced with a right fix, this change.

This change introduces new reload type RELOAD_MAIN_RESOURCE for content.
We use this type for IPC, and convert it to FrameLoadTypeReloadMainResource
to ask a right kind of reload to Blink.

FrameLoadTypeReloadMainResource was previously known as FrameLoadTypeSame.
The type didn't explain the behavior, but it meant we validate the main
resource and follows protocols for sub resources.

Even with this patch, sub resources will be validated on preloading
for some reasons if DevTools is open. It should be fixed by followup changes.

BUG=558829
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/bada0dc96fe81de27879058e78225e1747b2d57f
Cr-Commit-Position: refs/heads/master@{#394336}
==========

[commit-bot: I haz the power, 2016-05-18 05:37:08]

Patchset 6 (id:??) landed as
https://crrev.com/bada0dc96fe81de27879058e78225e1747b2d57f
Cr-Commit-Position: refs/heads/master@{#394336}