PlzNavigate: support for should_replace_current_entry

PlzNavigate: support for should_replace_current_entry

This CL enables support for replacing the current entry when browser-side navigation is enabled.
In particular, it moves the should_replace_current_entry param from
StartNavigationParams to CommonNavigationParams.

BUG=475027
TBR=nasko@chromium.org

Committed: https://crrev.com/280a4647f315d88f76ec0b233fa97cf4bf9c4890
Cr-Commit-Position: refs/heads/master@{#340454}

[clamy, 2015-06-11 15:22:18]

clamy@chromium.org changed reviewers:
+ avi@chromium.org, carlosk@chromium.org, creis@chromium.org,
fdegans@chromium.org, nasko@chromium.org

[clamy, 2015-06-11 15:25:10]

@avi, creis, nasko: PTAL
@carlosk, fdegans: FYI

This is what I was mentioning in my mail about client-side redirects. It's
basically plumbing parameters between processes. I would have hoped to reuse
some of the existing things (like should_replace_current_entry, but it does not
seem to work).

[Avi (use Gerrit), 2015-06-11 19:09:01]

This whole review hinges for me on what "locking the history list" means. And I
haven't a clue.

What exactly does it mean? In which cases is it true, and which cases is it
false? I don't necessarily need the complete list of "it is true for x1...x25
and false for y1...y38" but I need enough examples to build a mental model.

https://codereview.chromium.org/1182453002/diff/1/content/common/navigation_p...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1182453002/diff/1/content/common/navigation_p...
content/common/navigation_params.h:83: bool lock_history_list;
The meaning of this is unclear.

The comment says "Whether the navigation should generate a new entry...." If the
variable were named "generate_new_entry" then it would be clear what the boolean
meant. But since the name is "lock_history_list", is it opposite? If we want a
new entry we set this to false?

Please clarify what true and false mean for this variable.

[Charlie Reis, 2015-06-12 21:16:22]

I'm not happy with adding this concept to Chromium.  "Locking the back forward
list" is a Blink concept that roughly correlates with whether the current entry
should be replaced, and we should not have both concepts in Chromium.

This is also not specific to client-redirects, since it applies to
location.replace, replaceState, the initial non-about:blank navigation in
subframes, and other cases.

Like Avi, I don't yet understand how this is solving the problem that you have,
because I suspect that some existing information might be sufficient for what
you need (or could be adapted for it without adding this).

[clamy, 2015-06-23 16:46:50]

Following our email thread, here is an update of the patch that only uses
should_replace_current_entry. It does have to explicitly load the request with
the awfully-named RedirectWithLockedBackForward list unfortunately, as just
using should_replace_current_entry does not lead to having InertCommit as the
commit type when we expect it (and several ClassifyNavigation browser tests fail
because of that).

[clamy, 2015-07-15 12:25:41]

@creis, avi: friendly ping :)

[Avi (use Gerrit), 2015-07-15 14:39:28]

This lgtm. Charlie?

https://codereview.chromium.org/1182453002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1182453002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:4540: // TODO(clamy): see if this shoudl
also be the case with subframes (and
s/shoudl/should/

[Charlie Reis, 2015-07-15 22:36:56]

On 2015/07/15 12:25:41, clamy wrote:
> @creis, avi: friendly ping :)

Sorry for the delay-- your previous message came in while I was on vacation and
got lost in my inbox.

Also sorry to hold this up further, but please don't land this yet.  It
conflicts with my in-progress changes for cross-process location.replace in
general (which is broken: https://crbug.com/317872) and for OOPIFs:
https://codereview.chromium.org/1225593003/

I'm close to finishing up that CL, but I have to change the classification type
from EXISTING_PAGE to NEW_PAGE so that we don't break Web UI bindings and
SiteInstance assignments.  I'm hoping to wrap it up this week.

My CL does a similar change to the load type but in a more general way, since
it's needed for many cases: top-level cross-process location.replace and OOPIF
location.replace.  Some of your additional changes here may be necessary for
PlzNavigate on top of that, but I think it makes sense to re-evaluate your
changes after my CL lands.  Hope that's not too much trouble.

https://codereview.chromium.org/1182453002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1182453002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:4544: load_type =
blink::WebFrameLoadType::RedirectWithLockedBackForwardList;
This conflicts with https://codereview.chromium.org/1225593003/, and I've
renamed the load type to ReplacesCurrentItem in
https://codereview.chromium.org/1232223002/.

[clamy, 2015-07-24 15:37:40]

Thanks! Following your answer, I moved this CL to just having the
should_replace_current_entry param in PlzNavigate as well. For further changes
that may impact how we load the request, I will wait until you are done with
cross-site location support. Do you also want me to wait to land this patch?

[Charlie Reis, 2015-07-24 18:25:41]

On 2015/07/24 15:37:40, clamy wrote:
> Thanks! Following your answer, I moved this CL to just having the
> should_replace_current_entry param in PlzNavigate as well. For further changes
> that may impact how we load the request, I will wait until you are done with
> cross-site location support. Do you also want me to wait to land this patch?

We'll still conflict with each other, but only in mostly mechanical ways at this
point so I'm ok with it.  LGTM pending one question below.

Also, sorry that my side of the fix is taking a while.  It's proving quite
disruptive, and I've split it into several pieces (fixing WillSendRequest in
https://codereview.chromium.org/1225593003/, creating initial blank FNEs in
https://codereview.chromium.org/1255573005, and changing how location.replace is
classified and handled in https://codereview.chromium.org/1250163002/, which is
still in progress).

https://codereview.chromium.org/1182453002/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1182453002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4793: !frame_->parent()) {
Why is !frame_->parent() here?  You can do location.replace in a subframe, so I
would expect should_replace_current_entry to be true in that case.

[clamy, 2015-07-27 11:39:41]

Thanks!
@nasko: I'm TBRing you for the frame_messages.h review, since the changes there
are very minimal.

https://codereview.chromium.org/1182453002/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1182453002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4793: !frame_->parent()) {
On 2015/07/24 18:25:41, Charlie Reis wrote:
> Why is !frame_->parent() here?  You can do location.replace in a subframe, so
I
> would expect should_replace_current_entry to be true in that case.

We were initially worried with Nasko that this would mess up the classification
of navigations in subframes. However, since you are working on location.replace
in subframes, this probably won't be an issue. I removed this particular check
in the latest patcheset.

[clamy, 2015-07-27 11:39:54]

The CQ bit was checked by clamy@chromium.org

[clamy, 2015-07-27 11:39:54]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1182453002/#ps80001
(title: "Addressed Charlie's comments")

[commit-bot: I haz the power, 2015-07-27 11:39:56]

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

[nasko, 2015-07-27 12:46:41]

IPC LGTM and a small drive-by nit.

https://codereview.chromium.org/1182453002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1182453002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:4800: if (data_source &&
render_view_->history_list_length_ > 0) {
nit: No need for {} for single line if statements.

[commit-bot: I haz the power, 2015-07-27 12:55:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-27 12:56:20]

Patchset 5 (id:??) landed as
https://crrev.com/280a4647f315d88f76ec0b233fa97cf4bf9c4890
Cr-Commit-Position: refs/heads/master@{#340454}