PlzNavigate: identify same-page browser-initiated navigation.

PlzNavigate: same-page navigation.

This patch handle same-page history and fragment navigations.

With PlzNavigate, these navigations were not identified as
such, and the browser issued a network request for them.
This patch fixes it and a test is added to prevent regressions.

Care has been taken to avoid race conditions between the
browser and the renderer. For instance:
1) The user initiates a same-page navigation. So no network request is
   made and the navigation goes to the renderer.
2) In the meantime, a renderer-initiated navigation occurs and is
   committed.
3) The first navigation arrives in the renderer, but it is no more a
   same page navigation but a normal one. The problem is that no network
   requests was made and the request could not be committed anymore.
In that case, the request is simply dropped. This edge case is very
rare, however.

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

Review-Url: https://codereview.chromium.org/2584513003
Cr-Commit-Position: refs/heads/master@{#449122}
Committed: https://chromium.googlesource.com/chromium/src/+/92f1868b4b0a675218d0c1b6bd7fa96e3e1a3a9d

[arthursonzogni, 2016-12-15 15:30:48]

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

[commit-bot: I haz the power, 2016-12-15 15:31:21]

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, 2016-12-15 17:33:57]

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

[commit-bot: I haz the power, 2016-12-15 17:33:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[arthursonzogni, 2016-12-16 08:58:00]

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

[commit-bot: I haz the power, 2016-12-16 08:58:14]

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

[arthursonzogni, 2016-12-16 10:35:41]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-12-16 11:00:02]

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

[commit-bot: I haz the power, 2016-12-16 11:00:03]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2016-12-16 12:48:36]

arthursonzogni@chromium.org changed reviewers:
+ clamy@chromium.org

[arthursonzogni, 2016-12-16 12:48:37]

Hi Camille,

Please, can you review this patch?
You are an OWNER of content/ and I will ask another reviewer for the remaining
files once you are happy with the change.

Thanks!

[clamy, 2016-12-20 15:16:22]

Thanks! I'm generally happy with the change, a few comments below. Feel free to
add other reviewers, as this may change the content/ part.

One thing to note: it'd be good to use is_same_document for the constructor of
the NavigationHandle created from the NavigationRequest. One thing to note as
well: I'm not sure we handle is_same_document_history_load properly.

https://codereview.chromium.org/2584513003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2584513003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2584:
!request_params.is_same_document_navigation)
Please add curly braces since the if condition spans more than one line.

https://codereview.chromium.org/2584513003/diff/60001/content/common/navigati...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/2584513003/diff/60001/content/common/navigati...
content/common/navigation_params.cc:24: // TODO(clamy): pushState/popState
should not send requests to the
Can you remove the TODO entirely? pushState & popState already don't make
network requests.

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

https://codereview.chromium.org/2584513003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1188: // between. In that
case, this navigation is dropped.
Can you prefix the comment with // PlzNavigate ?
I would also rephrase it as the following:
// If the loader classifies this navigation as a different document navigation
while the browser intended the navigation to be same-document, drop the
navigation. A different navigation must have committed while the IPC was sent.

[arthursonzogni, 2016-12-21 08:46:41]

Patchset #1 (id:20001) has been deleted

[arthursonzogni, 2016-12-21 08:47:00]

Patchset #1 (id:40001) has been deleted

[arthursonzogni, 2016-12-21 10:11:07]

arthursonzogni@chromium.org changed reviewers:
+ jochen@chromium.org

[arthursonzogni, 2016-12-21 10:11:08]

Hi jochen,

Do you think you could review this CL? (third_party/Webkit/*) ?
Thanks!

https://codereview.chromium.org/2584513003/diff/60001/content/common/navigati...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/2584513003/diff/60001/content/common/navigati...
content/common/navigation_params.cc:24: // TODO(clamy): pushState/popState
should not send requests to the
On 2016/12/20 15:16:22, clamy wrote:
> Can you remove the TODO entirely? pushState & popState already don't make
> network requests.

Done.

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

https://codereview.chromium.org/2584513003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1188: // between. In that
case, this navigation is dropped.
On 2016/12/20 15:16:22, clamy wrote:
> Can you prefix the comment with // PlzNavigate ?
> I would also rephrase it as the following:
> // If the loader classifies this navigation as a different document navigation
> while the browser intended the navigation to be same-document, drop the
> navigation. A different navigation must have committed while the IPC was sent.

Done.

[arthursonzogni, 2016-12-21 10:12:42]

Thanks Camille!

https://codereview.chromium.org/2584513003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2584513003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2584:
!request_params.is_same_document_navigation)
On 2016/12/20 15:16:22, clamy wrote:
> Please add curly braces since the if condition spans more than one line.

Done.

[arthursonzogni, 2016-12-21 10:13:59]

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

[commit-bot: I haz the power, 2016-12-21 10:14:07]

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

[jochen (gone - plz use gerrit), 2016-12-21 10:30:58]

https://codereview.chromium.org/2584513003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2584513003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1192: return;
but if another navigation committed meanwhile, won't we need to do a "real" load
to get back to the original doc?

[commit-bot: I haz the power, 2016-12-21 12:22:56]

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

[commit-bot: I haz the power, 2016-12-21 12:22:57]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2016-12-21 13:24:29]

https://codereview.chromium.org/2584513003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2584513003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1192: return;
On 2016/12/21 10:30:58, jochen wrote:
> but if another navigation committed meanwhile, won't we need to do a "real"
load
> to get back to the original doc?

You mean relaunching the navigation, but this time from the renderer. That could
be a good solution.
Dropping the navigation is easier thought, and I think the difference is
imperceptible for the user. Let me explain:

If the users press ENTER immediately **after** the URL is modified in the
browser, the renderer-initiated page is displayed, the URL bar lose the focus
and the user press ENTER nowhere.
If the user press Enter an imperceptible moment **before** the URL is modified
in the browser, I think it's okay to display the renderer-initiated page too.
The user couldn't be sure at 100% that the URL bar's entry was modified before
or after they pressed ENTER.

If the browser-initiated navigation is dropped, the users will think that the
page navigated by itself just before they pressed enter, while it happened in
reality an imperceptible moment after they pressed enter.

No user could tell what solution we are using under the hood.
I think it doesn't worth trying to relaunch the navigation as other problems
that I am not aware of could occur.

For instance, if the navigation is relaunched, the page history will be (1 -> 2
-> 1#1). But the user will think it is (1 -> 1#1). They could be confuse when
they will do a back-navigation. It is better if the page history is (1 -> 2) and
the user think it is (1 -> 2) too.

Do you agree?

[clamy, 2016-12-22 17:30:26]

Thanks! A few comments below.

https://codereview.chromium.org/2584513003/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2584513003/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:76: class
ConnectionListener
Did you find this class in use somewhere in Chrome? We *heavily* discourage use
of locks inside chromium: in particular, this may alter interleaving of thread
tasks and lead to different results in the test compared to the actual code.

Instead, if the goal is to check that no network request was made, you could
check that we got a WebContentsObserver::ReadyToCommitNavigation without calling
NavigationThrottle::WillStartRequest &
NavigationThrottle::WillProcessNavigation. This would also be a good place to
check that we properly report the navigation as same page in the
NavigationHandle.

https://codereview.chromium.org/2584513003/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:7081: //
Ensure that browser-initiated same-document navigations are detected and
Could you also add a test for the fallback/drop case?

https://codereview.chromium.org/2584513003/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:7083: //
see crbug.com/663777
nit: comments start with a capital letter and end with a dot :).

https://codereview.chromium.org/2584513003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2584513003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1192: return;
On 2016/12/21 13:24:29, arthursonzogni wrote:
> On 2016/12/21 10:30:58, jochen wrote:
> > but if another navigation committed meanwhile, won't we need to do a "real"
> load
> > to get back to the original doc?
> 
> You mean relaunching the navigation, but this time from the renderer. That
could
> be a good solution.
> Dropping the navigation is easier thought, and I think the difference is
> imperceptible for the user. Let me explain:
> 
> If the users press ENTER immediately **after** the URL is modified in the
> browser, the renderer-initiated page is displayed, the URL bar lose the focus
> and the user press ENTER nowhere.
> If the user press Enter an imperceptible moment **before** the URL is modified
> in the browser, I think it's okay to display the renderer-initiated page too.
> The user couldn't be sure at 100% that the URL bar's entry was modified before
> or after they pressed ENTER.
> 
> If the browser-initiated navigation is dropped, the users will think that the
> page navigated by itself just before they pressed enter, while it happened in
> reality an imperceptible moment after they pressed enter.
> 
> No user could tell what solution we are using under the hood.
> I think it doesn't worth trying to relaunch the navigation as other problems
> that I am not aware of could occur.
> 
> For instance, if the navigation is relaunched, the page history will be (1 ->
2
> -> 1#1). But the user will think it is (1 -> 1#1). They could be confuse when
> they will do a back-navigation. It is better if the page history is (1 -> 2)
and
> the user think it is (1 -> 2) too.
> 
> Do you agree?

Actually, the time can be longer if for whatever reason the renderer was hung.

That said, I'm not sure if doing a fallback would be better from a user
perspective. Considering we just committed a new page, going back to the browser
and doing a network request in that case and committing the old page back might
be a bit weird.

If we want to preserve this behavior though, it's easy to send the request to
the browser from here. Just setCheckForBrowserSideNavigation on the
ResourceRequest to true and proceed. When we get to decidePolicyForNavigation in
startLoad the request will be sent to browser.

[arthursonzogni, 2016-12-23 14:16:02]

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

[commit-bot: I haz the power, 2016-12-23 14:16:19]

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, 2016-12-23 17:28:50]

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

[commit-bot: I haz the power, 2016-12-23 17:28:51]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-01-02 07:39:52]

what surprises me is that this decision is done in the renderer.

shouldn't the renderer in a PlzNavigate world blindly do whatever the browser
told it to do? even if there's a race, the race should be resolved in the
browser, not in the renderer, no?

[arthursonzogni, 2017-01-02 12:47:17]

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

[commit-bot: I haz the power, 2017-01-02 12:47:26]

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-01-02 12:49:16]

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

[commit-bot: I haz the power, 2017-01-02 12:49:18]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[arthursonzogni, 2017-01-02 13:10:14]

Patchset #4 (id:120001) has been deleted

[arthursonzogni, 2017-01-02 13:10:43]

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

[commit-bot: I haz the power, 2017-01-02 13:10:50]

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-01-02 16:05:27]

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

[commit-bot: I haz the power, 2017-01-02 16:05:28]

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

[arthursonzogni, 2017-01-03 17:43:43]

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

[commit-bot: I haz the power, 2017-01-03 17:44:07]

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-01-03 19:16:09]

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

[commit-bot: I haz the power, 2017-01-03 19:16:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[arthursonzogni, 2017-01-04 09:16:30]

Patchset #6 (id:180001) has been deleted

[arthursonzogni, 2017-01-04 09:32:19]

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

[commit-bot: I haz the power, 2017-01-04 09:32:25]

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-01-04 11:29:50]

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

[commit-bot: I haz the power, 2017-01-04 11:29:51]

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

[arthursonzogni, 2017-01-04 14:22:59]

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

[commit-bot: I haz the power, 2017-01-04 14:23:16]

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

[arthursonzogni, 2017-01-04 14:23:35]

Patchset #4 (id:140001) has been deleted

[arthursonzogni, 2017-01-04 14:23:46]

Patchset #4 (id:160001) has been deleted

[arthursonzogni, 2017-01-04 14:24:00]

Patchset #4 (id:200001) has been deleted

[commit-bot: I haz the power, 2017-01-04 15:56:27]

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

[commit-bot: I haz the power, 2017-01-04 15:56:28]

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

[arthursonzogni, 2017-01-05 10:02:56]

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

[commit-bot: I haz the power, 2017-01-05 10:03:18]

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

[arthursonzogni, 2017-01-05 10:03:35]

Patchset #4 (id:220001) has been deleted

[arthursonzogni, 2017-01-05 10:03:50]

Patchset #4 (id:240001) has been deleted

[arthursonzogni, 2017-01-05 10:31:56]

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

[commit-bot: I haz the power, 2017-01-05 10:32:09]

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-01-05 12:17:45]

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

[commit-bot: I haz the power, 2017-01-05 12:17:46]

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

[arthursonzogni, 2017-01-05 16:03:30]

Patchset #5 (id:280001) has been deleted

[arthursonzogni, 2017-01-05 16:04:08]

Patchset #5 (id:300001) has been deleted

[arthursonzogni, 2017-01-05 16:04:25]

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

[commit-bot: I haz the power, 2017-01-05 16:04:48]

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

[arthursonzogni, 2017-01-05 16:12:48]

The CQ bit was unchecked by arthursonzogni@chromium.org

[arthursonzogni, 2017-01-05 16:16:44]

Patchset #5 (id:320001) has been deleted

[arthursonzogni, 2017-01-05 16:21:09]

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

[commit-bot: I haz the power, 2017-01-05 16:21:32]

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

[arthursonzogni, 2017-01-05 17:04:22]

On 2017/01/02 07:39:52, jochen wrote:
> what surprises me is that this decision is done in the renderer.
> 
> shouldn't the renderer in a PlzNavigate world blindly do whatever the browser
> told it to do? even if there's a race, the race should be resolved in the
> browser, not in the renderer, no?

@jochen: Yes, I think the browser should resolve the majority of the race
condition issues. The problem here is that the browser submit a request without
any content in it (i.e. a same page navigation). The browser can't know for sure
that the renderer state will not change between the time it takes its decision
and when it is applied in the renderer. It's not really a problem with
different-page navigation because the request hold the page content and can
replace the renderer state whatever it is.

The request is dropped in these cases for instance:
*
NavigationControllerBrowserTest.RaceCrossOriginNavigationAndSamePageHistoryNavigation.
The browser submit two navigations. The first one to a different page, but the
IPC is delayed such that the browser doesn't know it was committed in the
renderer. Then the browser submit a request to the original page. The browser
thinks it is a same-page navigation, but it is a different-page navigation from
the renderer point of view.
* When the renderer submit a request to about:blank, it is submitted directly as
if PlzNavigate wasn't used. A same-page navigation could be dropped depending on
when it happens. It used to be the same with data-url, but it's no more the
case.

@clamy: I applied your suggestion. Now NavigationHandle.IsSamePage() works. It
solves that last failing content_browsertest :)
I extended my commit such that same page history loads are also considered to be
same page.

@nasko [+cc]: Hi Nasko, please, could you take a look at what happens in the
test: RaceCrossOriginNavigationAndSamePageHistoryNavigation in the file:
content/browser/frame_host/navigation_controller_impl_browsertest.cc
The behavior with PlzNavigate is different. Maybe dropping the navigation is
preferable, I don't know, but then the final url is not the same.

https://codereview.chromium.org/2584513003/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2584513003/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:76: class
ConnectionListener
On 2016/12/22 17:30:25, clamy wrote:
> Did you find this class in use somewhere in Chrome? We *heavily* discourage
use
> of locks inside chromium: in particular, this may alter interleaving of thread
> tasks and lead to different results in the test compared to the actual code.
> 
> Instead, if the goal is to check that no network request was made, you could
> check that we got a WebContentsObserver::ReadyToCommitNavigation without
calling
> NavigationThrottle::WillStartRequest &
> NavigationThrottle::WillProcessNavigation. This would also be a good place to
> check that we properly report the navigation as same page in the
> NavigationHandle.

Yes I found it there:
https://cs.chromium.org/chromium/src/chrome/browser/net/predictor_browsertest.cc
They use a lock too.
It is a listener on the embedded_test_server, I like it because it doesn't make
any assumptions on how chromes works.

I will try your suggestion.

https://codereview.chromium.org/2584513003/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:7081: //
Ensure that browser-initiated same-document navigations are detected and
On 2016/12/22 17:30:25, clamy wrote:
> Could you also add a test for the fallback/drop case?

I don't have any idea how to test it.

https://codereview.chromium.org/2584513003/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:7083: //
see crbug.com/663777
On 2016/12/22 17:30:25, clamy wrote:
> nit: comments start with a capital letter and end with a dot :).

Done.

[commit-bot: I haz the power, 2017-01-05 17:48:58]

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

[commit-bot: I haz the power, 2017-01-05 17:48:59]

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

[jochen (gone - plz use gerrit), 2017-01-09 09:08:37]

third_party/WebKit lgtm

[arthursonzogni, 2017-01-09 12:22:23]

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

[commit-bot: I haz the power, 2017-01-09 12:22:32]

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

[arthursonzogni, 2017-01-09 12:28:45]

Patchset #6 (id:350001) has been deleted

[arthursonzogni, 2017-01-09 12:29:19]

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

[commit-bot: I haz the power, 2017-01-09 12:29:25]

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-01-09 15:44:41]

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

[commit-bot: I haz the power, 2017-01-09 15:44:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[clamy, 2017-01-09 16:58:48]

Thanks! A few comments below.

https://codereview.chromium.org/2584513003/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2584513003/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:7081: //
Ensure that browser-initiated same-document navigations are detected and
On 2017/01/05 17:04:22, arthursonzogni wrote:
> On 2016/12/22 17:30:25, clamy wrote:
> > Could you also add a test for the fallback/drop case?
> 
> I don't have any idea how to test it.

I'd suggest adding a RenderViewImplTest. See
RenderViewImplTest.OnNavigationHTTPPost for an idea on how to check whether the
renderer emitted an IPC or not. In that case, we want to check that we don't get
a DidCommitProvisionalLoad but either a BeginNavigation or nothing depending on
the option we chose.

https://codereview.chromium.org/2584513003/diff/390001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2584513003/diff/390001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:212:
if (navigation_handle->IsSamePage())
I don't think we want to call ReadyToCommitNavigation for a same page
navigation.

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6182: //   
-> a.com/title1.html
Why do we have a different history state in the PlzNavigate case?

[nasko, 2017-01-10 00:18:59]

nasko@chromium.org changed reviewers:
+ nasko@chromium.org

[nasko, 2017-01-10 00:19:00]

Went through the implementation code only, not through tests yet. Sending a
round of comments.

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:225:
net::AreURLsInPageNavigation(frame_tree_node->current_url(), dest_url);
Why can't we use NavigationControllerImpl::IsURLInPageNavigation?

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:426: false, //
is_same_document_navigation
Why are we guaranteed here it isn't same document? Can't the URL we want to go
be just a different fragment from the last committed URL?

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3335:
navigation_handle_->GetURL() == params.url) {
Why is the URL check needed here? Also, if it is a fragment change, wouldn't
that still be a same-page navigation, but fail the strict URL match here?

https://codereview.chromium.org/2584513003/diff/390001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2584513003/diff/390001/content/common/navigat...
content/common/navigation_params.h:235: bool is_same_document_fragment_change,
We are starting to accumulate a bunch of bool params that are all unique and
exclusive. Maybe we should introduce an enum for the navigation type (though bad
name) and avoid having the three separate bool params?

[clamy, 2017-01-10 10:52:54]

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3335:
navigation_handle_->GetURL() == params.url) {
On 2017/01/10 00:19:00, nasko wrote:
> Why is the URL check needed here? Also, if it is a fragment change, wouldn't
> that still be a same-page navigation, but fail the strict URL match here?

This is checking whether we are comitting a navigation that matches the handle
we created in the first place. If the navigation is the one that created the
handle, the strict url check should match.

https://codereview.chromium.org/2584513003/diff/390001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2584513003/diff/390001/content/common/navigat...
content/common/navigation_params.h:235: bool is_same_document_fragment_change,
On 2017/01/10 00:19:00, nasko wrote:
> We are starting to accumulate a bunch of bool params that are all unique and
> exclusive. Maybe we should introduce an enum for the navigation type (though
bad
> name) and avoid having the three separate bool params?

We could also extend the FrameMsg_Navigate_Type::Value (and maybe rename it
NavigationType). In that case, we could have the same_document value be a flag
on the type, a bit like we do with TransitionType.

[arthursonzogni, 2017-01-11 17:30:03]

Patchset #8 (id:410001) has been deleted

[arthursonzogni, 2017-01-11 17:30:38]

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

[commit-bot: I haz the power, 2017-01-11 17:31:02]

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-01-11 21:02:58]

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

[commit-bot: I haz the power, 2017-01-11 21:03:00]

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

[arthursonzogni, 2017-01-12 16:55:14]

Patchset #11 (id:490001) has been deleted

[arthursonzogni, 2017-01-12 17:01:08]

Patchset #11 (id:510001) has been deleted

[arthursonzogni, 2017-01-12 17:04:06]

Patchset #11 (id:530001) has been deleted

[arthursonzogni, 2017-01-12 17:06:00]

Patchset #11 (id:550001) has been deleted

[arthursonzogni, 2017-01-12 17:06:46]

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

[commit-bot: I haz the power, 2017-01-12 17:07:17]

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

[arthursonzogni, 2017-01-12 17:32:30]

Thanks!

A few comments below:

https://codereview.chromium.org/2584513003/diff/390001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2584513003/diff/390001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:212:
if (navigation_handle->IsSamePage())
On 2017/01/09 16:58:48, clamy (slow) wrote:
> I don't think we want to call ReadyToCommitNavigation for a same page
> navigation.

I don't know. I will filter the call to ReadyToCommitNavigation for the moment
and we will see later what should be done.

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6182: //   
-> a.com/title1.html
On 2017/01/09 16:58:48, clamy (slow) wrote:
> Why do we have a different history state in the PlzNavigate case?

The navigation is relaunched from the renderer by putting an
request.setCheckFromBrowserSideNavigation(true);

It doesn't make sense for history-navigation because renderer-side
history-navigation doesn't exists. So it becomes a renderer-side
regular-navigation only.

It's very hard to relaunch the navigation without having problems. It's probably
better to drop the navigation right know such that it resolves the
race-condition quickly in a way we can predict.

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:225:
net::AreURLsInPageNavigation(frame_tree_node->current_url(), dest_url);
On 2017/01/10 00:19:00, nasko wrote:
> Why can't we use NavigationControllerImpl::IsURLInPageNavigation?

I didn't know this function existed. I just tried it and we can't use it. This
function simply checks that url's origin match and the argument
"renderer_says_in_page" is true. it doesn't check that the two paths match. The
purpose of this function is to ensure that the browser can trust the renderer
when it claims the navigation to be in-page. It doesn't fit very well with our
issue.

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:426: false, //
is_same_document_navigation
On 2017/01/10 00:19:00, nasko wrote:
> Why are we guaranteed here it isn't same document? Can't the URL we want to go
> be just a different fragment from the last committed URL?

You are right. It was something I forgot. I will fix this.
Whatever happens, this attribute is only used with PlzNavigate and we are in a
non-PlzNavigate code. But it's still better to have the correct value or to put
a comment somewhere.

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3335:
navigation_handle_->GetURL() == params.url) {
On 2017/01/10 10:52:54, clamy wrote:
> On 2017/01/10 00:19:00, nasko wrote:
> > Why is the URL check needed here? Also, if it is a fragment change, wouldn't
> > that still be a same-page navigation, but fail the strict URL match here?
> 
> This is checking whether we are comitting a navigation that matches the handle
> we created in the first place. If the navigation is the one that created the
> handle, the strict url check should match.

Yes, we have to check that the current navigation_handle is the one that
initiated the navigation.

Sometimes, it's not the case, for instance:
The browser initiate a same-page navigation. The renderer commit it, but before
that the confirmation is sent back to the browser, the browser initiate another
navigation and replace the current navigation_handle by the new one.
In that case, the new navigation_handle is not reused but a new one is created.

I preserve the old behavior by creating a new navigation_handle if the current
one is not available. Is there anything that could goes wrong with this?

https://codereview.chromium.org/2584513003/diff/390001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2584513003/diff/390001/content/common/navigat...
content/common/navigation_params.h:235: bool is_same_document_fragment_change,
On 2017/01/10 10:52:54, clamy wrote:
> On 2017/01/10 00:19:00, nasko wrote:
> > We are starting to accumulate a bunch of bool params that are all unique and
> > exclusive. Maybe we should introduce an enum for the navigation type (though
> bad
> > name) and avoid having the three separate bool params?
> 
> We could also extend the FrameMsg_Navigate_Type::Value (and maybe rename it
> NavigationType). In that case, we could have the same_document value be a flag
> on the type, a bit like we do with TransitionType.

It's a good thing to do. I like the idea with FrameMsg_Navigate_Type. Maybe it
would be better to do a separate commit as there would produce a lot of simple
change in at least 15 files.

[nasko, 2017-01-13 02:51:10]

I wonder why we needed so many changes to unit test files. Passing sequence
numbers from tests that don't care about them doesn't seem right to me.

https://codereview.chromium.org/2584513003/diff/390001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2584513003/diff/390001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:212:
if (navigation_handle->IsSamePage())
On 2017/01/12 17:32:29, arthursonzogni wrote:
> On 2017/01/09 16:58:48, clamy (slow) wrote:
> > I don't think we want to call ReadyToCommitNavigation for a same page
> > navigation.
> 
> I don't know. I will filter the call to ReadyToCommitNavigation for the moment
> and we will see later what should be done.

This means that users of WebContentsObserver will have to account for this
difference in behavior, which might make their code more complex. I wonder what
would be the issue with dispatching ReadyToCommitNavigation for same-page cases?

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2584513003/diff/390001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:225:
net::AreURLsInPageNavigation(frame_tree_node->current_url(), dest_url);
On 2017/01/12 17:32:30, arthursonzogni wrote:
> On 2017/01/10 00:19:00, nasko wrote:
> > Why can't we use NavigationControllerImpl::IsURLInPageNavigation?
> 
> I didn't know this function existed. I just tried it and we can't use it. This
> function simply checks that url's origin match and the argument
> "renderer_says_in_page" is true. it doesn't check that the two paths match.
The
> purpose of this function is to ensure that the browser can trust the renderer
> when it claims the navigation to be in-page. It doesn't fit very well with our
> issue.

Acknowledged.

https://codereview.chromium.org/2584513003/diff/390001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2584513003/diff/390001/content/common/navigat...
content/common/navigation_params.h:235: bool is_same_document_fragment_change,
On 2017/01/12 17:32:30, arthursonzogni wrote:
> On 2017/01/10 10:52:54, clamy wrote:
> > On 2017/01/10 00:19:00, nasko wrote:
> > > We are starting to accumulate a bunch of bool params that are all unique
and
> > > exclusive. Maybe we should introduce an enum for the navigation type
(though
> > bad
> > > name) and avoid having the three separate bool params?
> > 
> > We could also extend the FrameMsg_Navigate_Type::Value (and maybe rename it
> > NavigationType). In that case, we could have the same_document value be a
flag
> > on the type, a bit like we do with TransitionType.
> 
> It's a good thing to do. I like the idea with FrameMsg_Navigate_Type. Maybe it
> would be better to do a separate commit as there would produce a lot of simple
> change in at least 15 files.

Separate commit is definitely a good idea. It will also show us whether it is a
good change to make or not, without having a ton of other context.

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(left):

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6171:
EXPECT_EQ(start_url, web_contents->GetLastCommittedURL());
Why drop this expectation?

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6170: //
With browser-side-navigation, the history navigation is dropped.
I thought the discussion we had concluded that we would like to just issue a new
navigation when the race condition is hit.

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6171: //
Verify the expected origin through JavaScript. It also has the additional
This comment doesn't seem correct, as the code doesn't verify the origin, it
only checks for the string 'alive'.

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6179:
return;
Let's use an else block so it is more clear that they are exclusive.

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (left):

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2621:
main_test_rfh()->PrepareForCommit();
Doesn't dropping these change the nature of the test?

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2633:
back_params.url = url1;
Why is setting of the URL dropped here?

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:538: !IsSamePage())
This should fit just fine on the previous line.

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right):

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:909: // See
crbug.com/663777.
nit: Combine on previous line.

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:228: !is_history_navigation;
Can you put a comment why is_history_navigation needs to be false here?

https://codereview.chromium.org/2584513003/diff/590001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/2584513003/diff/590001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:525: false, 0, 0);
Why were those changes necessary? This test case doesn't test same page
navigations at all.

https://codereview.chromium.org/2584513003/diff/590001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2584513003/diff/590001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5980: 
nit: Why just an empty line here?

https://codereview.chromium.org/2584513003/diff/590001/content/test/test_web_...
File content/test/test_web_contents.cc (right):

https://codereview.chromium.org/2584513003/diff/590001/content/test/test_web_...
content/test/test_web_contents.cc:162: params.http_status_code = 0;
Shouldn't this be a valid HTTP status code?

https://codereview.chromium.org/2584513003/diff/590001/net/base/url_util.h
File net/base/url_util.h (right):

https://codereview.chromium.org/2584513003/diff/590001/net/base/url_util.h#ne...
net/base/url_util.h:155: NET_EXPORT bool AreURLsInPageNavigation(const GURL&
existing_url,
nit: It is a bit strange to have "navigation" in the //net/ code, which only
deals with networking and has no concept of navigation. Can the method be named
by what it checks - whether the two URLs only differ in fragments.

[arthursonzogni, 2017-01-13 12:57:09]

Patchset #13 (id:610001) has been deleted

[arthursonzogni, 2017-01-13 12:57:32]

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

[commit-bot: I haz the power, 2017-01-13 12:57:46]

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-01-13 12:59:51]

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

[commit-bot: I haz the power, 2017-01-13 12:59:52]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[arthursonzogni, 2017-01-13 15:06:28]

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

[arthursonzogni, 2017-01-13 15:06:48]

Patchset #13 (id:630001) has been deleted

[commit-bot: I haz the power, 2017-01-13 15:06:49]

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

[arthursonzogni, 2017-01-13 15:11:32]

Patchset #13 (id:650001) has been deleted

[arthursonzogni, 2017-01-13 15:11:48]

Patchset #13 (id:590002) has been deleted

[arthursonzogni, 2017-01-13 15:18:37]

Thanks Nasko!

The problem with not setting the document_sequence_number is that it leads to
store a zero for all FrameNavigationEntry.document_sequence_number.
Since they are all the same, NavigationControllerImpl::FindFramesToNavigate will
classify every history navigation to be same-document.
Since we correctly handle same-document history navigationn now, it breaks some
tests that are different-document history-navigation.

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(left):

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6171:
EXPECT_EQ(start_url, web_contents->GetLastCommittedURL());
On 2017/01/13 02:51:09, nasko wrote:
> Why drop this expectation?

It's a mistake, thanks!

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6170: //
With browser-side-navigation, the history navigation is dropped.
On 2017/01/13 02:51:09, nasko wrote:
> I thought the discussion we had concluded that we would like to just issue a
new
> navigation when the race condition is hit.

It's difficult to relaunch every navigations. I have problems with history
navigation especially.
Maybe we can discuss about it again.

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6171: //
Verify the expected origin through JavaScript. It also has the additional
On 2017/01/13 02:51:09, nasko wrote:
> This comment doesn't seem correct, as the code doesn't verify the origin, it
> only checks for the string 'alive'.

Done.

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6179:
return;
On 2017/01/13 02:51:09, nasko wrote:
> Let's use an else block so it is more clear that they are exclusive.

Done.

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (left):

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2621:
main_test_rfh()->PrepareForCommit();
On 2017/01/13 02:51:10, nasko wrote:
> Doesn't dropping these change the nature of the test?

renderer-initiated navigations doesn't do a round-trip with the browser
{beginNavigation, commitNavigation} before  didCommitProvisionalLoad when they
are same-site.

If it change the nature of the test, it's for the best, no? According to me, the
previous lines are wrong, or at least, it doesn't reflect what really happens in
Chrome. 

FYI, before fixing it, I got an error due to the fact that if
SendRendererInitiatedNavigationRequest is called, the navigation is detected to
be same-site on the browser-side and commit directly and there is no waiting for
the beforeunload event. Then prepareForCommit doesn't  work because the
navigation has already committed.

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2633:
back_params.url = url1;
On 2017/01/13 02:51:09, nasko wrote:
> Why is setting of the URL dropped here?

back_params is a copy of self_params. We redo the navigation but this time it's
an history navigation. The url is the same.
Setting the url is a no-op.

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:538: !IsSamePage())
On 2017/01/13 02:51:10, nasko wrote:
> This should fit just fine on the previous line.

Done.

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:228: !is_history_navigation;
On 2017/01/13 02:51:10, nasko wrote:
> Can you put a comment why is_history_navigation needs to be false here?

Yes, it is more than useful.
Done.

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:266: // TODO(clamy): Check if
some PageState should be provided here.
Do we know now? Does history navigations could call this method?

https://codereview.chromium.org/2584513003/diff/590001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/2584513003/diff/590001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:525: false, 0, 0);
On 2017/01/13 02:51:10, nasko wrote:
> Why were those changes necessary? This test case doesn't test same page
> navigations at all.

Yes these navigation are not same-document.
Without what I did, the document_sequence_number would be always 0. Then history
navigations are classified to be same-document because every items share the
same document_sequence_number.

https://codereview.chromium.org/2584513003/diff/590001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2584513003/diff/590001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5980: 
On 2017/01/13 02:51:10, nasko wrote:
> nit: Why just an empty line here?

Done.

[arthursonzogni, 2017-01-13 17:26:16]

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

[commit-bot: I haz the power, 2017-01-13 17:26:49]

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-01-13 18:19:58]

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

[commit-bot: I haz the power, 2017-01-13 18:20:00]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[nasko, 2017-01-13 19:36:57]

I'm not entirely convinced that we need the changes in FrameLoader.cpp. If that
ends up being the case, we can simplify this CL greatly by not passing the new
boolean down to the renderer, which will keep the current (non-PlzNavigate) path
unchanged.

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6170: //
With browser-side-navigation, the history navigation is dropped.
On 2017/01/13 15:18:37, arthursonzogni wrote:
> On 2017/01/13 02:51:09, nasko wrote:
> > I thought the discussion we had concluded that we would like to just issue a
> new
> > navigation when the race condition is hit.
> 
> It's difficult to relaunch every navigations. I have problems with history
> navigation especially.

Why would you need to relaunch every navigation? I think the resolution in the
renderer is already present - it converts the SameDocument load type into a
DifferentDocument load type, which should cause a new navigation to be started.

> Maybe we can discuss about it again.

Sure, feel free to start an email discussion or schedule a VC chat.

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (left):

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2621:
main_test_rfh()->PrepareForCommit();
On 2017/01/13 15:18:37, arthursonzogni wrote:
> On 2017/01/13 02:51:10, nasko wrote:
> > Doesn't dropping these change the nature of the test?
> 
> renderer-initiated navigations doesn't do a round-trip with the browser
> {beginNavigation, commitNavigation} before  didCommitProvisionalLoad when they
> are same-site.
> 
> If it change the nature of the test, it's for the best, no? According to me,
the
> previous lines are wrong, or at least, it doesn't reflect what really happens
in
> Chrome. 
> 
> FYI, before fixing it, I got an error due to the fact that if
> SendRendererInitiatedNavigationRequest is called, the navigation is detected
to
> be same-site on the browser-side and commit directly and there is no waiting
for
> the beforeunload event. Then prepareForCommit doesn't  work because the
> navigation has already committed.

Acknowledged.

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2334: //
navigations. See https://crbug.com/522193.
nit: No need for just empty space change.

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6181:
web_contents, "domAutomationController.send('alive')", &is_alive));
Why not use the origin? It gives you extra information to ensure we committed
the expected origin and not something else. Having the liveness check is a nice
to have, but it is more important that we committed the expected origin
similarly to non-PlzNavigate mode.

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:234: !is_history_navigation;
Thanks for the comment! I was discussing this logic with creis@ and he pointed
out that we should be matching here the logic that Blink uses to determine the
same type of navigation. Looking at
FrameLoader::shouldPerformFragmentNavigation, it does a few extra checks, which
we should ensure we match.

Brings up another question - even if we have a history navigation, we might not
need to make the network request. Assume we go from "a.com/#bar" to
"a.com/#foo", it will be a is_same_document_history_load and we need to ensure
we handle this too.

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:275: // Please note that no
history-navigation uses this method as well.
nit: Put the comments together before the DCHECK. You can put this one first, if
you want to ensure the other ones is closer to the DCHECK.

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2638: // If a network
request was made, update the LoFi state.
nit: Why change Preview back to LoFi? This is change unrelated to the goal of
this CL.

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3325: // because a
NavigationHandle is created.
I'm not sure I can follow this comment well. Did you meant to say that the
handle is already created somewhere else before getting here?

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3334: // created for
same-page navigations.
This comment seems to contradict the new comment above. Can we clarify both of
these?

https://codereview.chromium.org/2584513003/diff/740001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/2584513003/diff/740001/content/renderer/rende...
content/renderer/render_frame_impl.cc:576:
static_cast<WebURLRequest::PreviewsState>(common_params.previews_state));
Hmm, removing this line seems incorrect. It is unrelated to the goal of this CL.

https://codereview.chromium.org/2584513003/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2584513003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1198: // This navigation
is no more same-document. The navigation is simply dropped.
Looking at the code in RenderFrameImpl::NavigateInternal, it should already
detect the race condition and should have changed the load type to be different
document. Why do we need to special case this case here at all?

[arthursonzogni, 2017-01-16 10:50:31]

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

[commit-bot: I haz the power, 2017-01-16 10:50:45]

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

[clamy, 2017-01-16 12:16:11]

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2584513003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6170: //
With browser-side-navigation, the history navigation is dropped.
On 2017/01/13 19:36:56, nasko wrote:
> On 2017/01/13 15:18:37, arthursonzogni wrote:
> > On 2017/01/13 02:51:09, nasko wrote:
> > > I thought the discussion we had concluded that we would like to just issue
a
> > new
> > > navigation when the race condition is hit.
> > 
> > It's difficult to relaunch every navigations. I have problems with history
> > navigation especially.
> 
> Why would you need to relaunch every navigation? I think the resolution in the
> renderer is already present - it converts the SameDocument load type into a
> DifferentDocument load type, which should cause a new navigation to be
started.
> 
> > Maybe we can discuss about it again.
> 
> Sure, feel free to start an email discussion or schedule a VC chat.

In order to properly resolve it as a different document navigation it needs to
be sent back to the browser. Since we're trying to commit the navigation, it
won't be sent to the browser by default (we have checkForBrowserSideNavigation
set to false in commits). We somehow need to set it to true, so that the now
different document navigation will be sent to the browser.

We could avoid passing the boolean to the FrameLoader by creating the
WebURLRequest with checkForBrowserSideNavigation set to true when we expect to
commit a same site navigation, and false when we expect a different document
load. That said, I'm not sure it's clearer compared to setting it to false when
we detect that the same document navigation won't be able to commit in the
FrameLoader.

We should really think about whether we want to try to relaunch the navigation
instead of dropping it altogether. Another issue with relaunching it from the
renderer is that we drop the information that it was browser-initiated, which is
problematic.

[commit-bot: I haz the power, 2017-01-16 12:29:57]

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

[commit-bot: I haz the power, 2017-01-16 12:29:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[arthursonzogni, 2017-01-16 17:42:20]

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

[commit-bot: I haz the power, 2017-01-16 17:42:34]

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-01-16 18:35:31]

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

[commit-bot: I haz the power, 2017-01-16 18:35:33]

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

[arthursonzogni, 2017-01-17 09:33:23]

Description was changed from

==========
PlzNavigate: identify same-page browser-initiated navigation.

With PlzNavigate, same-page browser-initiated navigations were not
identified as such, and the browser issued a network request for them.
This patch fixes it and a test is added to prevent regressions.

Care has been taken to avoid race conditions between the
browser and the renderer. For instance:
1) The user initiates a same-page navigation. So no network request is
   made and the navigation goes to the renderer.
2) In the meantime, a renderer-initiated navigation occurs and is
   committed.
3) The first navigation arrives in the renderer, but it is no more a
   same page navigation but a normal one. The problem is that no network
   requests was made and the request could not be committed anymore.
In that case, the request is simply dropped. This edge case is very
rare, however.

BUG=663777

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
==========

to

==========
PlzNavigate: same-page navigation.

This patch handle same-page history and fragment navigations.

With PlzNavigate, these navigations were not identified as
such, and the browser issued a network request for them.
This patch fixes it and a test is added to prevent regressions.

Care has been taken to avoid race conditions between the
browser and the renderer. For instance:
1) The user initiates a same-page navigation. So no network request is
   made and the navigation goes to the renderer.
2) In the meantime, a renderer-initiated navigation occurs and is
   committed.
3) The first navigation arrives in the renderer, but it is no more a
   same page navigation but a normal one. The problem is that no network
   requests was made and the request could not be committed anymore.
In that case, the request is simply dropped. This edge case is very
rare, however.

BUG=663777

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
==========

[arthursonzogni, 2017-01-17 10:25:39]

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

[commit-bot: I haz the power, 2017-01-17 10:25:46]

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

[arthursonzogni, 2017-01-17 10:26:28]

Patchset #19 (id:800001) has been deleted

[arthursonzogni, 2017-01-17 10:26:44]

Patchset #19 (id:820001) has been deleted

[arthursonzogni, 2017-01-17 10:27:06]

Patchset #19 (id:840001) has been deleted

[commit-bot: I haz the power, 2017-01-17 11:56:42]

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

[commit-bot: I haz the power, 2017-01-17 11:56:43]

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

[arthursonzogni, 2017-01-17 14:57:55]

Patchset #18 (id:780001) has been deleted

[arthursonzogni, 2017-01-17 14:58:48]

Patchset #18 (id:860001) has been deleted

[arthursonzogni, 2017-01-17 15:00:25]

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

[commit-bot: I haz the power, 2017-01-17 15:00:36]

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

[arthursonzogni, 2017-01-17 15:24:48]

Thanks!
I apply what @nasko and @clamy suggested: merging all the boolean
(is_same_document_history_load, is_same_document_fragment_change, ...) into
FrameMsg_Navigate_Type.

The navigation classification of every navigation is now made into one place:
GetNavigationType(...)

A few answers to the previous comments below:

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2334: //
navigations. See https://crbug.com/522193.
On 2017/01/13 19:36:56, nasko wrote:
> nit: No need for just empty space change.

Done.

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6181:
web_contents, "domAutomationController.send('alive')", &is_alive));
On 2017/01/13 19:36:56, nasko wrote:
> Why not use the origin? It gives you extra information to ensure we committed
> the expected origin and not something else. Having the liveness check is a
nice
> to have, but it is more important that we committed the expected origin
> similarly to non-PlzNavigate mode.

Done.

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:234: !is_history_navigation;
On 2017/01/13 19:36:57, nasko wrote:
> Thanks for the comment! I was discussing this logic with creis@ and he pointed
> out that we should be matching here the logic that Blink uses to determine the
> same type of navigation. Looking at
> FrameLoader::shouldPerformFragmentNavigation, it does a few extra checks,
which
> we should ensure we match.
> 
> Brings up another question - even if we have a history navigation, we might
not
> need to make the network request. Assume we go from "a.com/#bar" to
> "a.com/#foo", it will be a is_same_document_history_load and we need to ensure
> we handle this too.

Thanks for shouldPerformFragmentNavigation. I will try to do a function like
this in the navigation_request.

Concerning your other question, I don't understand what is the problem.
In that case we will get : 
is_same_document_fragment_change == false;
is_same_document_history_load    == true;
And since NavigationHandle::IsSamePage() is true when one of the last two
booleans are true, no network request will be made.

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2638: // If a network
request was made, update the LoFi state.
On 2017/01/13 19:36:57, nasko wrote:
> nit: Why change Preview back to LoFi? This is change unrelated to the goal of
> this CL.

Thanks! I missed this line when resolving merge conflicts after a Git rebase.

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3325: // because a
NavigationHandle is created.
On 2017/01/13 19:36:57, nasko wrote:
> I'm not sure I can follow this comment well. Did you meant to say that the
> handle is already created somewhere else before getting here?

Yes.

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3334: // created for
same-page navigations.
On 2017/01/13 19:36:57, nasko wrote:
> This comment seems to contradict the new comment above. Can we clarify both of
> these?

Okay, I will try to move/rephrase comments.

https://codereview.chromium.org/2584513003/diff/740001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/2584513003/diff/740001/content/renderer/rende...
content/renderer/render_frame_impl.cc:576:
static_cast<WebURLRequest::PreviewsState>(common_params.previews_state));
On 2017/01/13 19:36:57, nasko wrote:
> Hmm, removing this line seems incorrect. It is unrelated to the goal of this
CL.

Oops sorry!

https://codereview.chromium.org/2584513003/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2584513003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1198: // This navigation
is no more same-document. The navigation is simply dropped.
On 2017/01/13 19:36:57, nasko wrote:
> Looking at the code in RenderFrameImpl::NavigateInternal, it should already
> detect the race condition and should have changed the load type to be
different
> document. Why do we need to special case this case here at all?

It's because the response body stream is empty.
The browser wanted the navigation to be same-document. So no network response
was needed to navigate. The problem is that now, the renderer is not okay to do
a same-document navigation, so the main frame resource data is needed, but we
don't have it.

It's forbidden with PlzNavigate to request the main frame resource from the
renderer, so it will fail.

We can try several things from here:
 * Relaunch the navigation as if it was renderer-initiated  by writing
request.setCheckForBrowserSideNavigation(true). Nevertheless, there is a problem
when it's an history-navigation (they are always browser-initiated). The
PlzNavigate path through RenderFrameImpl::BeginNavigation doesn't allow
history-navigation.
 * Drop the navigation request.

[nasko, 2017-01-19 00:43:36]

Thanks for taking the FrameMsg_Navigate_Type approach. The code does look a bit
more verbose, but it is more clear what is happening and doesn't require keeping
track of multiple booleans.

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2584513003/diff/740001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3334: // created for
same-page navigations.
On 2017/01/17 15:24:48, arthursonzogni wrote:
> On 2017/01/13 19:36:57, nasko wrote:
> > This comment seems to contradict the new comment above. Can we clarify both
of
> > these?
> 
> Okay, I will try to move/rephrase comments.

This is awesome! Thanks for the changes, it is much easier to follow this way.

https://codereview.chromium.org/2584513003/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2584513003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1198: // This navigation
is no more same-document. The navigation is simply dropped.
On 2017/01/17 15:24:48, arthursonzogni wrote:
> On 2017/01/13 19:36:57, nasko wrote:
> > Looking at the code in RenderFrameImpl::NavigateInternal, it should already
> > detect the race condition and should have changed the load type to be
> different
> > document. Why do we need to special case this case here at all?
> 
> It's because the response body stream is empty.
> The browser wanted the navigation to be same-document. So no network response
> was needed to navigate. The problem is that now, the renderer is not okay to
do
> a same-document navigation, so the main frame resource data is needed, but we
> don't have it.
> 
> It's forbidden with PlzNavigate to request the main frame resource from the
> renderer, so it will fail.
> 
> We can try several things from here:
>  * Relaunch the navigation as if it was renderer-initiated  by writing
> request.setCheckForBrowserSideNavigation(true). Nevertheless, there is a
problem
> when it's an history-navigation (they are always browser-initiated). 

How about JS executing "window.history.back()" or similar? Those are
renderer-initiated signals to the browser to do history navigation. Wouldn't it
be more correct to look like one of these?

> The PlzNavigate path through RenderFrameImpl::BeginNavigation doesn't allow
> history-navigation.
>  * Drop the navigation request.

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6173: GURL
another_url(embedded_test_server()->GetURL("b.com", "/title1.html"));
Can we use some other way of verifying the navigation was dropped? Doing a new
navigation affects navigation state, so if the browser-side state in
NavigationController is confused because of the dropped history navigation, we
would not be able to detect it this way. I'm not sure what would be a better way
to check as of now, but the fact that the browser doesn't know the history
navigation didn't commit seems problematic, doesn't it?

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (left):

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:255: // TODO(clamy): Check if
some PageState should be provided here.
Why did this TODO disappear? Are we sure PageState should/should not be
provided?

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:259: // No fragment-navigation
uses this method because the navigation takes place
nit: Empty line before the comment.

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:392:
FrameMsg_Navigate_Type::IsSameDocument(common_params_.navigation_type);
Why not call the method as part of the parameter passing? Does it become harder
to read?

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:451: is_history_navigation,        
  // is_history_navigation
Maybe pass the FrameNavigationEntry, as we can extract the data we need from it?
It is a bit nicer to read and you can also save the frame_entry.method()
parameter too.

[arthursonzogni, 2017-01-19 15:22:17]

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

[commit-bot: I haz the power, 2017-01-19 15:22:34]

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-01-19 17:14:50]

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

[commit-bot: I haz the power, 2017-01-19 17:14:51]

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

[arthursonzogni, 2017-01-19 17:25:21]

Patchset #22 (id:960001) has been deleted

[arthursonzogni, 2017-01-19 17:34:16]

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

[commit-bot: I haz the power, 2017-01-19 17:34:37]

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

[arthursonzogni, 2017-01-19 17:49:21]

Thanks!
Some answers to the comments below:

https://codereview.chromium.org/2584513003/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2584513003/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1198: // This navigation
is no more same-document. The navigation is simply dropped.
On 2017/01/19 00:43:35, nasko (out Jan 19th) wrote:
> On 2017/01/17 15:24:48, arthursonzogni wrote:
> > On 2017/01/13 19:36:57, nasko wrote:
> > > Looking at the code in RenderFrameImpl::NavigateInternal, it should
already
> > > detect the race condition and should have changed the load type to be
> > different
> > > document. Why do we need to special case this case here at all?
> > 
> > It's because the response body stream is empty.
> > The browser wanted the navigation to be same-document. So no network
response
> > was needed to navigate. The problem is that now, the renderer is not okay to
> do
> > a same-document navigation, so the main frame resource data is needed, but
we
> > don't have it.
> > 
> > It's forbidden with PlzNavigate to request the main frame resource from the
> > renderer, so it will fail.
> > 
> > We can try several things from here:
> >  * Relaunch the navigation as if it was renderer-initiated  by writing
> > request.setCheckForBrowserSideNavigation(true). Nevertheless, there is a
> problem
> > when it's an history-navigation (they are always browser-initiated). 
> 
> How about JS executing "window.history.back()" or similar? Those are
> renderer-initiated signals to the browser to do history navigation. Wouldn't
it
> be more correct to look like one of these?
> 
> > The PlzNavigate path through RenderFrameImpl::BeginNavigation doesn't allow
> > history-navigation.
> >  * Drop the navigation request.

For renderer-initiated history-navigation. The renderer send a
ViewHostMsg_GoToEntryAtOffset to the browser. Then it uses the browser-initiated
path.

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6173: GURL
another_url(embedded_test_server()->GetURL("b.com", "/title1.html"));
On 2017/01/19 00:43:35, nasko wrote:
> Can we use some other way of verifying the navigation was dropped? Doing a new
> navigation affects navigation state, so if the browser-side state in
> NavigationController is confused because of the dropped history navigation, we
> would not be able to detect it this way. I'm not sure what would be a better
way
> to check as of now, but the fact that the browser doesn't know the history
> navigation didn't commit seems problematic, doesn't it?

Being sure that something didn't happens is always harder that being sure that
something happens :-). Adding another navigation is not ideal, but it's the only
thing I could imagine.

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (left):

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:255: // TODO(clamy): Check if
some PageState should be provided here.
On 2017/01/19 00:43:35, nasko wrote:
> Why did this TODO disappear? Are we sure PageState should/should not be
> provided?
Yes I believe that no PageState should be provided.
The DCHECK(...IsHistory...) I added should warn us that this assumption is
false.

In the next patch, I will add a comment to explain what are the navigations
types I expect to happens and why.

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:259: // No fragment-navigation
uses this method because the navigation takes place
On 2017/01/19 00:43:35, nasko wrote:
> nit: Empty line before the comment.

Acknowledged.

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:392:
FrameMsg_Navigate_Type::IsSameDocument(common_params_.navigation_type);
On 2017/01/19 00:43:35, nasko wrote:
> Why not call the method as part of the parameter passing? Does it become
harder
> to read?

What you suggest looks good.
Done.

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:451: is_history_navigation,        
  // is_history_navigation
On 2017/01/19 00:43:35, nasko wrote:
> Maybe pass the FrameNavigationEntry, as we can extract the data we need from
it?
> It is a bit nicer to read and you can also save the frame_entry.method()
> parameter too.

Perfect!

[commit-bot: I haz the power, 2017-01-19 20:18:59]

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

[commit-bot: I haz the power, 2017-01-19 20:19:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nasko, 2017-01-23 19:10:13]

Almost there!

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6173: GURL
another_url(embedded_test_server()->GetURL("b.com", "/title1.html"));
On 2017/01/19 17:49:21, arthursonzogni wrote:
> On 2017/01/19 00:43:35, nasko wrote:
> > Can we use some other way of verifying the navigation was dropped? Doing a
new
> > navigation affects navigation state, so if the browser-side state in
> > NavigationController is confused because of the dropped history navigation,
we
> > would not be able to detect it this way. I'm not sure what would be a better
> way
> > to check as of now, but the fact that the browser doesn't know the history
> > navigation didn't commit seems problematic, doesn't it?
> 
> Being sure that something didn't happens is always harder that being sure that
> something happens :-). Adding another navigation is not ideal, but it's the
only
> thing I could imagine.

If we will be performing navigations, let's use a session history navigation,
probably another back navigation to ensure that we didn't confuse browser-side
state by dropping the navigation in the renderer.

https://codereview.chromium.org/2584513003/diff/1000001/content/browser/frame...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2584513003/diff/1000001/content/browser/frame...
content/browser/frame_host/navigation_request.cc:82: case
FrameMsg_Navigate_Type::UNSPECIFIED:
Let's see if we can just remove UNSPECIFIED and rely on the compiler to catch
mistakes for us.

https://codereview.chromium.org/2584513003/diff/1000001/content/common/frame_...
File content/common/frame_message_enums.h (right):

https://codereview.chromium.org/2584513003/diff/1000001/content/common/frame_...
content/common/frame_message_enums.h:47: UNSPECIFIED,
Can we avoid having such value? We should know the type of navigation at all
times, right?

https://codereview.chromium.org/2584513003/diff/1000001/third_party/WebKit/pu...
File third_party/WebKit/public/platform/WebURLRequest.h (right):

https://codereview.chromium.org/2584513003/diff/1000001/third_party/WebKit/pu...
third_party/WebKit/public/platform/WebURLRequest.h:340: // should be dropped if
the page's URL has changed in-between.
Would we drop the navigation if another same page navigation happened to be the
racy one? If not, then changing the URL means a different document, so let's be
more explicit. "if a different document was loaded in the frame in-between".

[arthursonzogni, 2017-01-24 10:14:45]

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

[commit-bot: I haz the power, 2017-01-24 10:15:04]

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-01-24 12:08:48]

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

[commit-bot: I haz the power, 2017-01-24 12:08:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[arthursonzogni, 2017-01-24 15:17:05]

Thanks Nasko! I updated the patch.
A few comments below:

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6173: GURL
another_url(embedded_test_server()->GetURL("b.com", "/title1.html"));
On 2017/01/23 19:10:13, nasko wrote:
> On 2017/01/19 17:49:21, arthursonzogni wrote:
> > On 2017/01/19 00:43:35, nasko wrote:
> > > Can we use some other way of verifying the navigation was dropped? Doing a
> new
> > > navigation affects navigation state, so if the browser-side state in
> > > NavigationController is confused because of the dropped history
navigation,
> we
> > > would not be able to detect it this way. I'm not sure what would be a
better
> > way
> > > to check as of now, but the fact that the browser doesn't know the history
> > > navigation didn't commit seems problematic, doesn't it?
> > 
> > Being sure that something didn't happens is always harder that being sure
that
> > something happens :-). Adding another navigation is not ideal, but it's the
> only
> > thing I could imagine.
> 
> If we will be performing navigations, let's use a session history navigation,
> probably another back navigation to ensure that we didn't confuse browser-side
> state by dropping the navigation in the renderer.

Done. Is it what you want?

https://codereview.chromium.org/2584513003/diff/1000001/content/browser/frame...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2584513003/diff/1000001/content/browser/frame...
content/browser/frame_host/navigation_request.cc:82: case
FrameMsg_Navigate_Type::UNSPECIFIED:
On 2017/01/23 19:10:13, nasko wrote:
> Let's see if we can just remove UNSPECIFIED and rely on the compiler to catch
> mistakes for us.

Acknowledged.

https://codereview.chromium.org/2584513003/diff/1000001/content/common/frame_...
File content/common/frame_message_enums.h (right):

https://codereview.chromium.org/2584513003/diff/1000001/content/common/frame_...
content/common/frame_message_enums.h:47: UNSPECIFIED,
On 2017/01/23 19:10:13, nasko wrote:
> Can we avoid having such value? We should know the type of navigation at all
> times, right?

It is used in the default constructor of CommonNavigationParams 

The default constructor is used in a bunch of test and some renderer-initiated
code. It looks like a lot of them are replacing the navigation_type attribute
after this constructor (which is the right thing to do) but I don't know if
nothing has been forgotten somewhere (and in the future tests...).
I put this special value to avoid silent-failures, which will be hard to debug.

I will use Frame_Navigate_Type::DIFFERENT_DOCUMENT instead. Tell me if it is
still okay.

https://codereview.chromium.org/2584513003/diff/1000001/third_party/WebKit/pu...
File third_party/WebKit/public/platform/WebURLRequest.h (right):

https://codereview.chromium.org/2584513003/diff/1000001/third_party/WebKit/pu...
third_party/WebKit/public/platform/WebURLRequest.h:340: // should be dropped if
the page's URL has changed in-between.
On 2017/01/23 19:10:13, nasko (out for Jan 24) wrote:
> Would we drop the navigation if another same page navigation happened to be
the
> racy one? If not, then changing the URL means a different document, so let's
be
> more explicit. "if a different document was loaded in the frame in-between".

You are right. Done.

[nasko, 2017-01-24 16:59:36]

LGTM with a couple of comments.

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2584513003/diff/920001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6173: GURL
another_url(embedded_test_server()->GetURL("b.com", "/title1.html"));
On 2017/01/24 15:17:04, arthursonzogni wrote:
> On 2017/01/23 19:10:13, nasko wrote:
> > On 2017/01/19 17:49:21, arthursonzogni wrote:
> > > On 2017/01/19 00:43:35, nasko wrote:
> > > > Can we use some other way of verifying the navigation was dropped? Doing
a
> > new
> > > > navigation affects navigation state, so if the browser-side state in
> > > > NavigationController is confused because of the dropped history
> navigation,
> > we
> > > > would not be able to detect it this way. I'm not sure what would be a
> better
> > > way
> > > > to check as of now, but the fact that the browser doesn't know the
history
> > > > navigation didn't commit seems problematic, doesn't it?
> > > 
> > > Being sure that something didn't happens is always harder that being sure
> that
> > > something happens :-). Adding another navigation is not ideal, but it's
the
> > only
> > > thing I could imagine.
> > 
> > If we will be performing navigations, let's use a session history
navigation,
> > probably another back navigation to ensure that we didn't confuse
browser-side
> > state by dropping the navigation in the renderer.
> 
> Done. Is it what you want?

Indeed! Thanks!

https://codereview.chromium.org/2584513003/diff/1000001/content/common/frame_...
File content/common/frame_message_enums.h (right):

https://codereview.chromium.org/2584513003/diff/1000001/content/common/frame_...
content/common/frame_message_enums.h:47: UNSPECIFIED,
On 2017/01/24 15:17:04, arthursonzogni wrote:
> On 2017/01/23 19:10:13, nasko wrote:
> > Can we avoid having such value? We should know the type of navigation at all
> > times, right?
> 
> It is used in the default constructor of CommonNavigationParams 
> 
> The default constructor is used in a bunch of test and some renderer-initiated
> code. It looks like a lot of them are replacing the navigation_type attribute
> after this constructor (which is the right thing to do) but I don't know if
> nothing has been forgotten somewhere (and in the future tests...).
> I put this special value to avoid silent-failures, which will be hard to
debug.
> 
> I will use Frame_Navigate_Type::DIFFERENT_DOCUMENT instead. Tell me if it is
> still okay.

Using DIFFERENT_DOCUMENT sgtm, since it is the most frequently used action.

https://codereview.chromium.org/2584513003/diff/1060001/content/browser/frame...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2584513003/diff/1060001/content/browser/frame...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6183: //
Navigate back another time.
nit: "Navigate back again."

https://codereview.chromium.org/2584513003/diff/1060001/content/renderer/rend...
File content/renderer/render_frame_impl_browsertest.cc (right):

https://codereview.chromium.org/2584513003/diff/1060001/content/renderer/rend...
content/renderer/render_frame_impl_browsertest.cc:345:
common_params.navigation_type = FrameMsg_Navigate_Type::DIFFERENT_DOCUMENT;
Since DIFFERENT_DOCUMENT is the default value, this isn't strictly necessary, is
it?

[arthursonzogni, 2017-01-25 13:13:59]

https://codereview.chromium.org/2584513003/diff/1060001/content/browser/frame...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2584513003/diff/1060001/content/browser/frame...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6183: //
Navigate back another time.
On 2017/01/24 16:59:36, nasko (out for Jan 24) wrote:
> nit: "Navigate back again."

Done.

https://codereview.chromium.org/2584513003/diff/1060001/content/renderer/rend...
File content/renderer/render_frame_impl_browsertest.cc (right):

https://codereview.chromium.org/2584513003/diff/1060001/content/renderer/rend...
content/renderer/render_frame_impl_browsertest.cc:345:
common_params.navigation_type = FrameMsg_Navigate_Type::DIFFERENT_DOCUMENT;
On 2017/01/24 16:59:36, nasko (out for Jan 24) wrote:
> Since DIFFERENT_DOCUMENT is the default value, this isn't strictly necessary,
is
> it?

Yes! This isn't strictly necessary. I just wanted to make it explicit.
This was one of the few places where it was not explicitly set.
Should I remove it?

[arthursonzogni, 2017-01-25 13:21:26]

arthursonzogni@chromium.org changed reviewers:
+ jam@chromium.org, marq@chromium.org

[arthursonzogni, 2017-01-25 13:21:27]

Hi @marq and @jam,

I need a review for a little change. I move a function from ios/ to url/ such
that I could reuse it.
Please, could you take a look? Thanks!

marq:
  ios/web/navigation/navigation_manager_impl.mm

jam:
  url/url_util.cc
  url/url_util.h

[marq (ping after 24h), 2017-01-25 13:24:51]

ios/ LGTM

[jam, 2017-01-25 18:29:06]

jam@chromium.org changed reviewers:
+ brettw@chromium.org
- jam@chromium.org

[jam, 2017-01-25 18:29:30]

On 2017/01/25 13:21:27, arthursonzogni wrote:
> Hi @marq and @jam,
> 
> I need a review for a little change. I move a function from ios/ to url/ such
> that I could reuse it.
> Please, could you take a look? Thanks!
> 
> marq:
>   ios/web/navigation/navigation_manager_impl.mm
> 
> jam:
>   url/url_util.cc
>   url/url_util.h

I'm not an owner in url/
Brett: ptal

[brettw, 2017-01-26 20:44:46]

https://codereview.chromium.org/2584513003/diff/1100001/url/url_util.h
File url/url_util.h (right):

https://codereview.chromium.org/2584513003/diff/1100001/url/url_util.h#newcod...
url/url_util.h:183: URL_EXPORT bool IsFragmentAddedOrUpdated(const GURL&
old_url,
Several points:

Regarding the naming: This code uses the term "ref" for the reference fragment
rather than "fragment". The naming of "changed" implies a temporal change which
may make sense in terms of the caller in navigator_impl.cc but makes less sense
to me in terms of a function taking two parameters.

This function is being used to compute whether the navigation is in-page. If the
2nd parameter has no ref or the URLs are equal, it doesn't imply to me that the
navigation is *not* in page. This seems like pretty strange semantics to me. Are
you sure this is what you want? If it is, this seems to be something specific to
the rules of how Blink sends us navigation messages rather than an intrinsic
property of URLs, and this exact behavior would be better implemented in the
navigation layer.

KURL has a function equalIgnoringFragmentIdentifier. This seems very easy to
reason about. I would be happy adding a GURL::EqualsIgnoringRef function and
then you wrap that in the navigation layer if there are extra rules you want to
implement on top of that about navigating from refs to no refs.



url_util is at a conceptually lower layer than GURL: it provides the common
backing for GURL and KURL. Functions dealing with GURL should go on the GURL
object.



Implementation:

The implementation is pretty heavyweight, involving reallocating and
recanonicalizing two URLs just for comparison. At this low layer, I think the
operations should be more optimized. To do this, call
Parsed::CountCharactersBefore(REF, <not_sure>) and compare the string pieces of
the computed prefix to see if the URLs are equivalent ignoring the ref.

[brettw, 2017-01-26 20:51:58]

If we do GURL::EqualsIgnoringRef then it should treat "google.com/foo#" and
"google.com/foo" as equal. We should have unit tests for this.

[arthursonzogni, 2017-01-27 12:23:03]

Patchset #29 (id:1120001) has been deleted

[arthursonzogni, 2017-01-27 12:26:36]

Patchset #29 (id:1140001) has been deleted

[arthursonzogni, 2017-01-27 12:57:57]

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

[commit-bot: I haz the power, 2017-01-27 12:58:15]

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-01-27 15:05:27]

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

[commit-bot: I haz the power, 2017-01-27 15:05:28]

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

[arthursonzogni, 2017-01-27 15:35:38]

Thanks @brettw and @marq!

@brettw:
I applied your suggestion. It's much better like this!
I am not fully aware of special cases that could happens with GURL. Let me know
if I did something wrong.

@marq:
I undo the change you reviewed :). I make use of GURL::EqualsIgnoringRef()
inside AreURLsInPageNavigation instead.
I will assume that you are still okay with this, let me know if it isn't.

[brettw, 2017-02-07 06:37:48]

LGTM with tweak.

https://codereview.chromium.org/2584513003/diff/1180001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/2584513003/diff/1180001/url/gurl.h#newcode390
url/gurl.h:390: static bool EqualsIgnoringRef(const GURL& x, const GURL& y);
I think I would prefer a non-static function:
  bool EqualsIgnoringRef(const GURL& other) const;
I think this is the typical form for non-operator comparison functions we use
elsewhere in the code (I think GURL had one before operator overloading was
allowed by the style guide).

https://codereview.chromium.org/2584513003/diff/1180001/url/url_util_unittest.cc
File url/url_util_unittest.cc (right):

https://codereview.chromium.org/2584513003/diff/1180001/url/url_util_unittest...
url/url_util_unittest.cc:452: {"http://a.com#foo", "http://b.com#bar", false},
Can you add a case below this of one with URL with a ref and text, and one with
no # at all?

[arthursonzogni, 2017-02-07 09:51:55]

Patchset #31 (id:1200001) has been deleted

[arthursonzogni, 2017-02-07 09:54:07]

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

[commit-bot: I haz the power, 2017-02-07 09:54:22]

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-02-07 11:14:34]

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

[commit-bot: I haz the power, 2017-02-07 11:14:36]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[arthursonzogni, 2017-02-07 12:04:50]

https://codereview.chromium.org/2584513003/diff/1180001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/2584513003/diff/1180001/url/gurl.h#newcode390
url/gurl.h:390: static bool EqualsIgnoringRef(const GURL& x, const GURL& y);
On 2017/02/07 06:37:47, brettw (digging out) wrote:
> I think I would prefer a non-static function:
>   bool EqualsIgnoringRef(const GURL& other) const;
> I think this is the typical form for non-operator comparison functions we use
> elsewhere in the code (I think GURL had one before operator overloading was
> allowed by the style guide).

Done.

https://codereview.chromium.org/2584513003/diff/1180001/url/url_util_unittest.cc
File url/url_util_unittest.cc (right):

https://codereview.chromium.org/2584513003/diff/1180001/url/url_util_unittest...
url/url_util_unittest.cc:452: {"http://a.com#foo", "http://b.com#bar", false},
On 2017/02/07 06:37:48, brettw (digging out) wrote:
> Can you add a case below this of one with URL with a ref and text, and one
with
> no # at all?

What do you mean by "text"?
I will do as if you said: "one with a ref and the other without". In that case,
it's a good idea. Thanks!

Before committing, I'll wait for the confirmation that it is what you wanted.

[brettw, 2017-02-07 17:27:31]

Still LGTM

https://codereview.chromium.org/2584513003/diff/1240001/url/url_util_unittest.cc
File url/url_util_unittest.cc (right):

https://codereview.chromium.org/2584513003/diff/1240001/url/url_util_unittest...
url/url_util_unittest.cc:456: {"http://a.com#foo", "http://b.com", false},
Yeah, this is what I meant. Thanks!

[nasko, 2017-02-07 17:29:47]

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

[commit-bot: I haz the power, 2017-02-07 17:30:07]

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-02-07 17:34:55]

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

[commit-bot: I haz the power, 2017-02-07 17:34:57]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[arthursonzogni, 2017-02-08 09:12:25]

Description was changed from

==========
PlzNavigate: same-page navigation.

This patch handle same-page history and fragment navigations.

With PlzNavigate, these navigations were not identified as
such, and the browser issued a network request for them.
This patch fixes it and a test is added to prevent regressions.

Care has been taken to avoid race conditions between the
browser and the renderer. For instance:
1) The user initiates a same-page navigation. So no network request is
   made and the navigation goes to the renderer.
2) In the meantime, a renderer-initiated navigation occurs and is
   committed.
3) The first navigation arrives in the renderer, but it is no more a
   same page navigation but a normal one. The problem is that no network
   requests was made and the request could not be committed anymore.
In that case, the request is simply dropped. This edge case is very
rare, however.

BUG=663777

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
==========

to

==========
PlzNavigate: same-page navigation.

This patch handle same-page history and fragment navigations.

With PlzNavigate, these navigations were not identified as
such, and the browser issued a network request for them.
This patch fixes it and a test is added to prevent regressions.

Care has been taken to avoid race conditions between the
browser and the renderer. For instance:
1) The user initiates a same-page navigation. So no network request is
   made and the navigation goes to the renderer.
2) In the meantime, a renderer-initiated navigation occurs and is
   committed.
3) The first navigation arrives in the renderer, but it is no more a
   same page navigation but a normal one. The problem is that no network
   requests was made and the request could not be committed anymore.
In that case, the request is simply dropped. This edge case is very
rare, however.

BUG=663777
==========

[arthursonzogni, 2017-02-08 09:15:58]

Description was changed from

==========
PlzNavigate: same-page navigation.

This patch handle same-page history and fragment navigations.

With PlzNavigate, these navigations were not identified as
such, and the browser issued a network request for them.
This patch fixes it and a test is added to prevent regressions.

Care has been taken to avoid race conditions between the
browser and the renderer. For instance:
1) The user initiates a same-page navigation. So no network request is
   made and the navigation goes to the renderer.
2) In the meantime, a renderer-initiated navigation occurs and is
   committed.
3) The first navigation arrives in the renderer, but it is no more a
   same page navigation but a normal one. The problem is that no network
   requests was made and the request could not be committed anymore.
In that case, the request is simply dropped. This edge case is very
rare, however.

BUG=663777
==========

to

==========
PlzNavigate: same-page navigation.

This patch handle same-page history and fragment navigations.

With PlzNavigate, these navigations were not identified as
such, and the browser issued a network request for them.
This patch fixes it and a test is added to prevent regressions.

Care has been taken to avoid race conditions between the
browser and the renderer. For instance:
1) The user initiates a same-page navigation. So no network request is
   made and the navigation goes to the renderer.
2) In the meantime, a renderer-initiated navigation occurs and is
   committed.
3) The first navigation arrives in the renderer, but it is no more a
   same page navigation but a normal one. The problem is that no network
   requests was made and the request could not be committed anymore.
In that case, the request is simply dropped. This edge case is very
rare, however.

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

[arthursonzogni, 2017-02-08 09:16:09]

Patchset #33 (id:1260001) has been deleted

[arthursonzogni, 2017-02-08 09:18:56]

The CQ bit was checked by arthursonzogni@chromium.org

[arthursonzogni, 2017-02-08 09:18:57]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
nasko@chromium.org, marq@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2584513003/#ps1280001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-02-08 09:19:11]

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-02-08 10:47:02]

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

[commit-bot: I haz the power, 2017-02-08 10:47:19]

Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[arthursonzogni, 2017-02-08 13:25:38]

The CQ bit was checked by arthursonzogni@chromium.org

[commit-bot: I haz the power, 2017-02-08 13:26:04]

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-02-08 15:28:55]

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

[commit-bot: I haz the power, 2017-02-08 15:28:58]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[arthursonzogni, 2017-02-08 15:31:15]

The CQ bit was checked by arthursonzogni@chromium.org

[commit-bot: I haz the power, 2017-02-08 15:32:22]

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

[arthursonzogni, 2017-02-08 16:57:14]

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

[commit-bot: I haz the power, 2017-02-08 16:57:44]

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-02-08 17:05:30]

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

[commit-bot: I haz the power, 2017-02-08 17:05:32]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[arthursonzogni, 2017-02-08 18:40:42]

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

[commit-bot: I haz the power, 2017-02-08 18:41:11]

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-02-08 21:15:28]

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

[commit-bot: I haz the power, 2017-02-08 21:15:30]

Dry run: This issue passed the CQ dry run.

[jam, 2017-02-08 22:47:04]

The CQ bit was checked by jam@chromium.org

[jam, 2017-02-08 22:47:05]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
jochen@chromium.org, nasko@chromium.org, marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2584513003/#ps1340001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-02-08 22:48:06]

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-02-08 23:00:09]

CQ is committing da patch.

Bot data: {"patchset_id": 1340001, "attempt_start_ts": 1486594024525620,
"parent_rev": "80f3afb13a970dc64b835ce91f3f1c16466c836d", "commit_rev":
"92f1868b4b0a675218d0c1b6bd7fa96e3e1a3a9d"}

[commit-bot: I haz the power, 2017-02-08 23:00:49]

Description was changed from

==========
PlzNavigate: same-page navigation.

This patch handle same-page history and fragment navigations.

With PlzNavigate, these navigations were not identified as
such, and the browser issued a network request for them.
This patch fixes it and a test is added to prevent regressions.

Care has been taken to avoid race conditions between the
browser and the renderer. For instance:
1) The user initiates a same-page navigation. So no network request is
   made and the navigation goes to the renderer.
2) In the meantime, a renderer-initiated navigation occurs and is
   committed.
3) The first navigation arrives in the renderer, but it is no more a
   same page navigation but a normal one. The problem is that no network
   requests was made and the request could not be committed anymore.
In that case, the request is simply dropped. This edge case is very
rare, however.

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

to

==========
PlzNavigate: same-page navigation.

This patch handle same-page history and fragment navigations.

With PlzNavigate, these navigations were not identified as
such, and the browser issued a network request for them.
This patch fixes it and a test is added to prevent regressions.

Care has been taken to avoid race conditions between the
browser and the renderer. For instance:
1) The user initiates a same-page navigation. So no network request is
   made and the navigation goes to the renderer.
2) In the meantime, a renderer-initiated navigation occurs and is
   committed.
3) The first navigation arrives in the renderer, but it is no more a
   same page navigation but a normal one. The problem is that no network
   requests was made and the request could not be committed anymore.
In that case, the request is simply dropped. This edge case is very
rare, however.

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

Review-Url: https://codereview.chromium.org/2584513003
Cr-Commit-Position: refs/heads/master@{#449122}
Committed:
https://chromium.googlesource.com/chromium/src/+/92f1868b4b0a675218d0c1b6bd7f...
==========

[commit-bot: I haz the power, 2017-02-08 23:00:52]

Committed patchset #36 (id:1340001) as
https://chromium.googlesource.com/chromium/src/+/92f1868b4b0a675218d0c1b6bd7f...