Reload: FrameMsg_Navigate_Type cleanup to rename RELOAD_MAIN_RESOURCE

Reload: FrameMsg_Navigate_Type cleanup to rename RELOAD_MAIN_RESOURCE

To use FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE consistently
instead of FrameMsg_Navigate_Type::RELOAD in content, rename
RELOAD_MAIN_RESOURCE to RELOAD and override old behavior.

This change introduces two functional changes:

 1. RELOAD_ORIGINAL_REQUEST_URL is changed to revalidate
    only main resource instead of revalidating all.
    (in render_frame_impl.cc ReloadFrameLoadTypeFor)

 2. In LoadDataURL(), replace flag is set for usual reload
    operations. This was the original behavior, but unexpectedly
    changed in newly introduced ReloadMainResource cases.

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

[Takashi Toyoshima, 2016-12-19 10:01:39]

Description was changed from

==========
Reload: FrameMsg_Navigate_Type cleanup to rename RELOAD_MAIN_RESOURCE

To use FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE consistently
instead of FrameMsg_Navigate_Type::RELOAD in content, rename
RELOAD_MAIN_RESOURCE to RELOAD and override old behavior.

BUG=670232
==========

to

==========
Reload: FrameMsg_Navigate_Type cleanup to rename RELOAD_MAIN_RESOURCE

To use FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE consistently
instead of FrameMsg_Navigate_Type::RELOAD in content, rename
RELOAD_MAIN_RESOURCE to RELOAD and override old behavior.

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

[Takashi Toyoshima, 2016-12-19 10:02:35]

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

[commit-bot: I haz the power, 2016-12-19 10:02:51]

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

[Takashi Toyoshima, 2016-12-19 10:02:52]

Description was changed from

==========
Reload: FrameMsg_Navigate_Type cleanup to rename RELOAD_MAIN_RESOURCE

To use FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE consistently
instead of FrameMsg_Navigate_Type::RELOAD in content, rename
RELOAD_MAIN_RESOURCE to RELOAD and override old behavior.

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

to

==========
Reload: FrameMsg_Navigate_Type cleanup to rename RELOAD_MAIN_RESOURCE

To use FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE consistently
instead of FrameMsg_Navigate_Type::RELOAD in content, rename
RELOAD_MAIN_RESOURCE to RELOAD and override old behavior.

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

[Takashi Toyoshima, 2016-12-19 10:02:52]

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

[commit-bot: I haz the power, 2016-12-19 11:20:29]

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

[commit-bot: I haz the power, 2016-12-19 11:20:29]

Dry run: This issue passed the CQ dry run.

[Takashi Toyoshima, 2016-12-19 12:24:34]

please take a look after the holiday vacation.
I'd start working at Jan. 10 in 2017. So this review isn't urgent at all.

[Takashi Toyoshima, 2017-01-11 06:52:39]

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

[commit-bot: I haz the power, 2017-01-11 06:52:58]

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-01-11 06:53:45]

Hi, happy new year.
Can you start reviewing this?

[commit-bot: I haz the power, 2017-01-11 08:33:50]

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

[commit-bot: I haz the power, 2017-01-11 08:33:50]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-01-11 18:21:55]

Thanks for the ping!  Generally looks good, but one concern below.

https://codereview.chromium.org/2583123002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/2583123002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:676: return WebFrameLoadType::Reload;
This looks like a possible behavior change, since WebFrameLoadType::Reload is
still used differently than WebFrameLoadType::ReloadMainResource.  For example,
see RenderFrameImpl::LoadDataURL and the comment in WebFrameLoadType.h.

Do we need to update those as well?

[Takashi Toyoshima, 2017-01-12 11:11:00]

https://codereview.chromium.org/2583123002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/2583123002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:676: return WebFrameLoadType::Reload;
Since FrameMsg_Navigate_Type::RELOAD isn't used any more, this just change a
behavior for RELOAD_ORIGINAL_REQUEST_URL, and I think this should be changed
together to return ReloadMainResource when I introduced it.

I also feel I should have added "load_type ==
WebFrameLoadType::ReloadMainResource" in replace logic of LoadDataURL() though I
still do not understand completely how and when this code is called.

I already switched to use ReloadMainResource in almost all places in Blink side
though location.reload() still use Reload unless the experimental flag is
enabled. Once the study is fully launched, I'd also change Reload's behavior
completely as I'm doing in content side. This effort is tracked by
crbug.com/674404.

[Charlie Reis, 2017-01-13 00:19:47]

Thanks for the explanations-- LGTM with a few suggestions.

https://codereview.chromium.org/2583123002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/2583123002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:676: return WebFrameLoadType::Reload;
On 2017/01/12 11:11:00, toyoshim wrote:
> Since FrameMsg_Navigate_Type::RELOAD isn't used any more, this just change a
> behavior for RELOAD_ORIGINAL_REQUEST_URL, and I think this should be changed
> together to return ReloadMainResource when I introduced it.

Ok.  I'm alright with changing that behavior here as long as it's intentional,
and so that we're aware (in case there's a regression).  Can you mention the
change in the CL description?

> 
> I also feel I should have added "load_type ==
> WebFrameLoadType::ReloadMainResource" in replace logic of LoadDataURL() though
I
> still do not understand completely how and when this code is called.

I think that's primarily for Android Webview's loadDataWithBaseURL (sigh).  I
think you're right-- can you go ahead and update it to include
ReloadMainResource in this CL, as long as we're changing behavior?

> 
> I already switched to use ReloadMainResource in almost all places in Blink
side
> though location.reload() still use Reload unless the experimental flag is
> enabled. Once the study is fully launched, I'd also change Reload's behavior
> completely as I'm doing in content side. This effort is tracked by
> crbug.com/674404.

Ok, so the WebFrameLoadType cleanup is blocked on the experiment finishing. 
Sounds good.

[Takashi Toyoshima, 2017-01-13 06:34:40]

Description was changed from

==========
Reload: FrameMsg_Navigate_Type cleanup to rename RELOAD_MAIN_RESOURCE

To use FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE consistently
instead of FrameMsg_Navigate_Type::RELOAD in content, rename
RELOAD_MAIN_RESOURCE to RELOAD and override old behavior.

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

to

==========
Reload: FrameMsg_Navigate_Type cleanup to rename RELOAD_MAIN_RESOURCE

To use FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE consistently
instead of FrameMsg_Navigate_Type::RELOAD in content, rename
RELOAD_MAIN_RESOURCE to RELOAD and override old behavior.

This change will make one functional change that RELOAD_ORIGINAL_REQUEST_URL
is changed to revalidate only main resource instead of revalidating all.
(See render_frame_impl.cc ReloadFrameLoadTypeFor function)

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

[Takashi Toyoshima, 2017-01-13 06:45:09]

Description was changed from

==========
Reload: FrameMsg_Navigate_Type cleanup to rename RELOAD_MAIN_RESOURCE

To use FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE consistently
instead of FrameMsg_Navigate_Type::RELOAD in content, rename
RELOAD_MAIN_RESOURCE to RELOAD and override old behavior.

This change will make one functional change that RELOAD_ORIGINAL_REQUEST_URL
is changed to revalidate only main resource instead of revalidating all.
(See render_frame_impl.cc ReloadFrameLoadTypeFor function)

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

to

==========
Reload: FrameMsg_Navigate_Type cleanup to rename RELOAD_MAIN_RESOURCE

To use FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE consistently
instead of FrameMsg_Navigate_Type::RELOAD in content, rename
RELOAD_MAIN_RESOURCE to RELOAD and override old behavior.

This change introduces two functional changes:

 1. RELOAD_ORIGINAL_REQUEST_URL is changed to revalidate
    only main resource instead of revalidating all.
    (in render_frame_impl.cc ReloadFrameLoadTypeFor)

 2. In LoadDataURL(), replace flag is set for usual reload
    operations. This was the original behavior, but unexpectedly
    changed in newly introduced ReloadMainResource cases.

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

[Takashi Toyoshima, 2017-01-13 06:45:23]

https://codereview.chromium.org/2583123002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/2583123002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:676: return WebFrameLoadType::Reload;
On 2017/01/13 00:19:47, Charlie Reis wrote:
> On 2017/01/12 11:11:00, toyoshim wrote:
> > Since FrameMsg_Navigate_Type::RELOAD isn't used any more, this just change a
> > behavior for RELOAD_ORIGINAL_REQUEST_URL, and I think this should be changed
> > together to return ReloadMainResource when I introduced it.
> 
> Ok.  I'm alright with changing that behavior here as long as it's intentional,
> and so that we're aware (in case there's a regression).  Can you mention the
> change in the CL description?
> 
> > 
> > I also feel I should have added "load_type ==
> > WebFrameLoadType::ReloadMainResource" in replace logic of LoadDataURL()
though
> I
> > still do not understand completely how and when this code is called.
> 
> I think that's primarily for Android Webview's loadDataWithBaseURL (sigh).  I
> think you're right-- can you go ahead and update it to include
> ReloadMainResource in this CL, as long as we're changing behavior?
> 
> > 
> > I already switched to use ReloadMainResource in almost all places in Blink
> side
> > though location.reload() still use Reload unless the experimental flag is
> > enabled. Once the study is fully launched, I'd also change Reload's behavior
> > completely as I'm doing in content side. This effort is tracked by
> > crbug.com/674404.
> 
> Ok, so the WebFrameLoadType cleanup is blocked on the experiment finishing. 
> Sounds good.

Done.

[Takashi Toyoshima, 2017-01-13 06:45:34]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2017-01-13 06:45:34]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2583123002/#ps40001
(title: "replace logic update")

[commit-bot: I haz the power, 2017-01-13 06:45: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-01-13 08:31:45]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484289934323260,
"parent_rev": "07023fe24b20ec9ac60ed4fc72ef569e19e38b87", "commit_rev":
"63de737e764be064c7a3d28a5463f68abcbde975"}

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

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

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

Prior attempt to commit was detected, but we were not able to check whether the
issue was successfully committed. Please check Git history manually and re-check
CQ or close this issue as needed.

[Takashi Toyoshima, 2017-01-13 08:42:36]

looks correctly submitted as 63de737e764be064c7a3d28a5463f68abcbde975