Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2381503003:
PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… (Closed)
DescriptionPlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga…
This test is failing in PlzNav because
https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_controller_impl_browsertest.cc?q=%22Simulate+the+user+hitting+Enter%22&sq=package:chromium&l=5943&dr=C
is creating a new navigation entry, so the GoBack() goes to the wrong
place.
It's doing that because
FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is
true.
That in turn is because SendDidCommitProvisionalLoad is getting a commit
type of WebStandardCommit (not WebHistoryInertCommit like it does in
non-PlzNav).
*That* is because here
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/FrameLoader.cpp?rcl=0&l=839
the url being checked is the already-redirected one in the PlzNavigate
case (sub.a.com), but m_documentLoader->urlForHistory() is the
pre-redirected one (a.com/server-redirect?sub...).
So, this works in non-PlzNav because Blink tells us that it should
replace the current entry (i.e. it's like a reload).
Instead of relying on Blink to determine that, check if the render frame
node's last committed url is the original url we navigated to there, and
that we're not just mucking with history navigation, in which case we
convert the normal load to a reload.
R=nasko@chromium.org,clamy@chromium.org
BUG=630103, 475027, 536102
Committed: https://crrev.com/28bbbb16194c7337e22c3e0bda752ae6b4b681cf
Cr-Commit-Position: refs/heads/master@{#429302}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : get correct reload behaviour #Patch Set 4 : . #Patch Set 5 : . #
Total comments: 5
Patch Set 6 : . #Patch Set 7 : try reload in requestnav #Patch Set 8 : . #Patch Set 9 : . #
Total comments: 4
Patch Set 10 : change for both plznav and non-plznav (2) #Patch Set 11 : add virtual url test that fails with change #Patch Set 12 : fix for data url with virtual entry #Patch Set 13 : improve test (3) #
Total comments: 8
Patch Set 14 : rebase #Patch Set 15 : test on android again #Patch Set 16 : try in navigation controller impl instead #Patch Set 17 : another try #Patch Set 18 : disable logging #Patch Set 19 : android webview fix #Patch Set 20 : remove logging #
Total comments: 4
Patch Set 21 : update comment #
Messages
Total messages: 129 (94 generated)
Description was changed from ========== PlzNav: Possible fix for NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNavigationEntry R=nasko@chromium.org BUG=630103,475027 ========== to ========== PlzNav: Possible fix for NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNavigationEntry R=nasko@chromium.org BUG=630103,475027 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== PlzNav: Possible fix for NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNavigationEntry R=nasko@chromium.org BUG=630103,475027 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNav: Possible fix for NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNavigationEntry (Sorry, long explanation follows, maybe a lot of this is obvious to you.) This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). Anyway... I'm not sure who should be figuring out if it's a sort-of-reload URL. The test works if the LoadURL says it should be doing a replace as the diff here. It seemed semi-plausible to me at first because maybe we shouldn't be relying on Blink to tell us whether to replace navigation entries? But then maybe that doesn't make sense, because now we're doing the equivalent of relying on the omnibox to tell us. Any better ideas to enlightenment me? R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== PlzNav: Possible fix for NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNavigationEntry (Sorry, long explanation follows, maybe a lot of this is obvious to you.) This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). Anyway... I'm not sure who should be figuring out if it's a sort-of-reload URL. The test works if the LoadURL says it should be doing a replace as the diff here. It seemed semi-plausible to me at first because maybe we shouldn't be relying on Blink to tell us whether to replace navigation entries? But then maybe that doesn't make sense, because now we're doing the equivalent of relying on the omnibox to tell us. Any better ideas to enlightenment me? R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNav: Possible fix for NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNavigationEntry (Sorry, long explanation follows, maybe a lot of this is obvious to you.) This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). Anyway... I'm not sure who should be figuring out if it's a sort-of-reload URL. The test works if the LoadURL says it should be doing a replace as the diff here. It seemed semi-plausible to me at first because maybe we shouldn't be relying on Blink to tell us whether to replace navigation entries? But then maybe that doesn't make sense, because now we're doing the equivalent of relying on the omnibox to tell us. Any ideas to enlightenment me? R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== PlzNav: Possible fix for NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNavigationEntry (Sorry, long explanation follows, maybe a lot of this is obvious to you.) This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). Anyway... I'm not sure who should be figuring out if it's a sort-of-reload URL. The test works if the LoadURL says it should be doing a replace as the diff here. It seemed semi-plausible to me at first because maybe we shouldn't be relying on Blink to tell us whether to replace navigation entries? But then maybe that doesn't make sense, because now we're doing the equivalent of relying on the omnibox to tell us. Any ideas to enlightenment me? R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNav: Possible fix for NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNavigationEntry (Sorry, long explanation follows, maybe a lot of this is obvious to you.) This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). Anyway... I'm not sure who should be figuring out if it's a sort-of-reload URL. The test works if the LoadURL says it should be doing a replace as the diff here. It seemed semi-plausible to me at first because maybe we shouldn't be relying on Blink to tell us whether to replace navigation entries? But then maybe that doesn't make sense, because now we're doing the equivalent of relying on the omnibox to tell us. Any ideas to enlightenment me? R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
scottmg@chromium.org changed reviewers: + clamy@chromium.org
Long CL description, since I don't really know what the right fix is. :) I will dig some more, but any pointers appreciated on where the correct place might be to fix this.
I think the FrameLoader should receive a FrameLoadType that we determine based on the information the browser sent us. We probably want to use that one and not override it with what Blink computes.
I think the FrameLoader should receive a FrameLoadType that we determine based on the information the browser sent us. We probably want to use that one and not override it with what Blink computes.
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== PlzNav: Possible fix for NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNavigationEntry (Sorry, long explanation follows, maybe a lot of this is obvious to you.) This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). Anyway... I'm not sure who should be figuring out if it's a sort-of-reload URL. The test works if the LoadURL says it should be doing a replace as the diff here. It seemed semi-plausible to me at first because maybe we shouldn't be relying on Blink to tell us whether to replace navigation entries? But then maybe that doesn't make sense, because now we're doing the equivalent of relying on the omnibox to tell us. Any ideas to enlightenment me? R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNav: Possible fix for NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNavigationEntry (Sorry, long explanation follows, maybe a lot of this is obvious to you.) This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). Anyway... I'm not sure who should be figuring out if it's a sort-of-reload URL. The test works if the LoadURL says it should be doing a replace as the diff here. It seemed semi-plausible to me at first because maybe we shouldn't be relying on Blink to tell us whether to replace navigation entries? But then maybe that doesn't make sense, because now we're doing the equivalent of relying on the omnibox to tell us. Any ideas to enlightenment me? R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== PlzNav: Possible fix for NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNavigationEntry (Sorry, long explanation follows, maybe a lot of this is obvious to you.) This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). Anyway... I'm not sure who should be figuring out if it's a sort-of-reload URL. The test works if the LoadURL says it should be doing a replace as the diff here. It seemed semi-plausible to me at first because maybe we shouldn't be relying on Blink to tell us whether to replace navigation entries? But then maybe that doesn't make sense, because now we're doing the equivalent of relying on the omnibox to tell us. Any ideas to enlightenment me? R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNav: Possible fix for NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNavigationEntry This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame's last committed url is the original url we navigated to, and in that case set CommonNavigationParams.should_replace_current_entry so that we explicitly tell Blink to replace via WebFrameLoadType. R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== PlzNav: Possible fix for NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNavigationEntry This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame's last committed url is the original url we navigated to, and in that case set CommonNavigationParams.should_replace_current_entry so that we explicitly tell Blink to replace via WebFrameLoadType. R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame's last committed url is the original url we navigated to, and in that case set CommonNavigationParams.should_replace_current_entry so that we explicitly tell Blink to replace via WebFrameLoadType. R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by scottmg@chromium.org
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by scottmg@chromium.org
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by scottmg@chromium.org
Description was changed from ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame's last committed url is the original url we navigated to, and in that case set CommonNavigationParams.should_replace_current_entry so that we explicitly tell Blink to replace via WebFrameLoadType. R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame's last committed url is the original url we navigated to, and in that case set CommonNavigationParams.navigation_type so that we explicitly tell Blink to replace via WebFrameLoadType. R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame's last committed url is the original url we navigated to, and in that case set CommonNavigationParams.navigation_type so that we explicitly tell Blink to replace via WebFrameLoadType. R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame's last committed url is the original url we navigated to, that we're doing a redirect, and that the page transition is a reload, in which case we tell Blink to do a reload of the main resource, rather than a normal load. R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
I think this is ready for a look. It's a bit of an ugly condition, but it seems to pass tests*. Happy to hear if you have a simpler suggestion! * https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... has some layout tests failures, but I'm not sure why they're running here, as they're filtered out in FlagExpectations\enable-browser-side-navigation.
Description was changed from ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame's last committed url is the original url we navigated to, that we're doing a redirect, and that the page transition is a reload, in which case we tell Blink to do a reload of the main resource, rather than a normal load. R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame's last committed url is the original url we navigated to, that we're doing a redirect, and that the page transition is a reload, in which case we tell Blink to do a reload of the main resource, rather than a normal load. R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
clamy@chromium.org changed reviewers: + creis@chromium.org
+creis whose opinion I'd like on this. https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6054: ui::PAGE_TRANSITION_RELOAD, std::string()); Are we sure that in the case described above we would get a ui::PAGE_TRANSITION_RELOAD? I know that in NavigationController we do have a special case for the user wanted to navigate to the same url but didn't actually click on the reload button. I suspect we may not get a value of RELOAD in that case. https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:389: navigation_handle_->WasServerRedirect() && Is the redirect part needed? Otherwise, when we create the nav request, we could just determine that we're trying to navigate to the same url we navigated to, and that this is like a reload. This way, we don't need to do a comparison of the original url.
I'm not sure that the hash based navigation is what is throwing this off. I
tried rewriting the test a bit to use pushState and it still failed the same
way.
--- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
@@ -6130,11 +6130,18 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
// Navigate to a simple page and then perform an in-page navigation.
GURL start_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
EXPECT_TRUE(NavigateToURL(shell(), start_url));
+ EXPECT_EQ(1, web_contents->GetController().GetEntryCount());
GURL same_page_url(
- embedded_test_server()->GetURL("a.com", "/title1.html#foo"));
- EXPECT_TRUE(NavigateToURL(shell(), same_page_url));
- EXPECT_EQ(2, web_contents->GetController().GetEntryCount());
+ embedded_test_server()->GetURL("a.com", "/title2.html"));
+ {
+ TestNavigationObserver observer(web_contents);
+ std::string script =
+ "history.pushState({}, '', '" + same_page_url.spec() + "')";
+ EXPECT_TRUE(ExecuteScript(root, script));
+ observer.Wait();
+ EXPECT_EQ(2, web_contents->GetController().GetEntryCount());
+ }
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6054:
ui::PAGE_TRANSITION_RELOAD, std::string());
On 2016/10/05 12:47:04, clamy wrote:
> Are we sure that in the case described above we would get a
> ui::PAGE_TRANSITION_RELOAD? I know that in NavigationController we do have a
> special case for the user wanted to navigate to the same url but didn't
actually
> click on the reload button. I suspect we may not get a value of RELOAD in that
> case.
You are correct. Selecting the current URL in the omnibox and hitting enter does
not result in a RELOAD transition. The usage of LINK is *intentional*. When the
commit comes back to the browser, it matches the pending navigation entry and we
classify it as SAME_PAGE.
The question is why is this not classified as SAME_PAGE in PlzNavigate here. It
looks like we have a browser test for that -
NavigationTypeClassification_SamePage, which is passing.
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:389:
navigation_handle_->WasServerRedirect() &&
On 2016/10/05 12:47:04, clamy wrote:
> Is the redirect part needed? Otherwise, when we create the nav request, we
could
> just determine that we're trying to navigate to the same url we navigated to,
> and that this is like a reload. This way, we don't need to do a comparison of
> the original url.
Yeah, I'm not sure why we have the redirect check here. Actually, having a
redirect when loading an URL shouldn't cause us to reload the original, should
it?
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:393:
FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE;
I'm a bit afraid of such change. I do want us to do overall, as I'd like to
eliminate the SAME_PAGE classification, however in my naive CL for doing that,
there are lots of failures. https://codereview.chromium.org/2246363003/
On 2016/10/05 22:07:09, nasko (slow) wrote:
> I'm not sure that the hash based navigation is what is throwing this off. I
> tried rewriting the test a bit to use pushState and it still failed the same
> way.
>
> --- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc
> +++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
> @@ -6130,11 +6130,18 @@
IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
> // Navigate to a simple page and then perform an in-page navigation.
> GURL start_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
> EXPECT_TRUE(NavigateToURL(shell(), start_url));
> + EXPECT_EQ(1, web_contents->GetController().GetEntryCount());
>
> GURL same_page_url(
> - embedded_test_server()->GetURL("a.com", "/title1.html#foo"));
> - EXPECT_TRUE(NavigateToURL(shell(), same_page_url));
> - EXPECT_EQ(2, web_contents->GetController().GetEntryCount());
> + embedded_test_server()->GetURL("a.com", "/title2.html"));
> + {
> + TestNavigationObserver observer(web_contents);
> + std::string script =
> + "history.pushState({}, '', '" + same_page_url.spec() + "')";
> + EXPECT_TRUE(ExecuteScript(root, script));
> + observer.Wait();
> + EXPECT_EQ(2, web_contents->GetController().GetEntryCount());
> + }
>
Yeah, sorry I shouldn't have linked with line numbers. It's not the hash nav,
it's the navigate with type LINK (simulating "press enter on the url" that's
problematic.
>
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
> File content/browser/frame_host/navigation_controller_impl_browsertest.cc
> (right):
>
>
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
> content/browser/frame_host/navigation_controller_impl_browsertest.cc:6054:
> ui::PAGE_TRANSITION_RELOAD, std::string());
> On 2016/10/05 12:47:04, clamy wrote:
> > Are we sure that in the case described above we would get a
> > ui::PAGE_TRANSITION_RELOAD? I know that in NavigationController we do have a
> > special case for the user wanted to navigate to the same url but didn't
> actually
> > click on the reload button. I suspect we may not get a value of RELOAD in
that
> > case.
>
> You are correct. Selecting the current URL in the omnibox and hitting enter
does
> not result in a RELOAD transition. The usage of LINK is *intentional*. When
the
> commit comes back to the browser, it matches the pending navigation entry and
we
> classify it as SAME_PAGE.
> The question is why is this not classified as SAME_PAGE in PlzNavigate here.
It
> looks like we have a browser test for that -
> NavigationTypeClassification_SamePage, which is passing.
I'll look into that, thanks.
>
>
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
> File content/browser/frame_host/navigation_request.cc (right):
>
>
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
> content/browser/frame_host/navigation_request.cc:389:
> navigation_handle_->WasServerRedirect() &&
> On 2016/10/05 12:47:04, clamy wrote:
> > Is the redirect part needed? Otherwise, when we create the nav request, we
> could
> > just determine that we're trying to navigate to the same url we navigated
to,
> > and that this is like a reload. This way, we don't need to do a comparison
of
> > the original url.
>
> Yeah, I'm not sure why we have the redirect check here. Actually, having a
> redirect when loading an URL shouldn't cause us to reload the original, should
> it?
>
>
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
> content/browser/frame_host/navigation_request.cc:393:
> FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE;
> I'm a bit afraid of such change. I do want us to do overall, as I'd like to
> eliminate the SAME_PAGE classification, however in my naive CL for doing that,
> there are lots of failures. https://codereview.chromium.org/2246363003/
On 2016/10/05 22:27:57, scottmg wrote:
> On 2016/10/05 22:07:09, nasko (slow) wrote:
> > I'm not sure that the hash based navigation is what is throwing this off. I
> > tried rewriting the test a bit to use pushState and it still failed the same
> > way.
> >
> > --- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc
> > +++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
> > @@ -6130,11 +6130,18 @@
> IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
> > // Navigate to a simple page and then perform an in-page navigation.
> > GURL start_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
> > EXPECT_TRUE(NavigateToURL(shell(), start_url));
> > + EXPECT_EQ(1, web_contents->GetController().GetEntryCount());
> >
> > GURL same_page_url(
> > - embedded_test_server()->GetURL("a.com", "/title1.html#foo"));
> > - EXPECT_TRUE(NavigateToURL(shell(), same_page_url));
> > - EXPECT_EQ(2, web_contents->GetController().GetEntryCount());
> > + embedded_test_server()->GetURL("a.com", "/title2.html"));
> > + {
> > + TestNavigationObserver observer(web_contents);
> > + std::string script =
> > + "history.pushState({}, '', '" + same_page_url.spec() + "')";
> > + EXPECT_TRUE(ExecuteScript(root, script));
> > + observer.Wait();
> > + EXPECT_EQ(2, web_contents->GetController().GetEntryCount());
> > + }
> >
>
> Yeah, sorry I shouldn't have linked with line numbers. It's not the hash nav,
> it's the navigate with type LINK (simulating "press enter on the url" that's
> problematic.
>
> >
>
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
> > File content/browser/frame_host/navigation_controller_impl_browsertest.cc
> > (right):
> >
> >
>
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
> > content/browser/frame_host/navigation_controller_impl_browsertest.cc:6054:
> > ui::PAGE_TRANSITION_RELOAD, std::string());
> > On 2016/10/05 12:47:04, clamy wrote:
> > > Are we sure that in the case described above we would get a
> > > ui::PAGE_TRANSITION_RELOAD? I know that in NavigationController we do have
a
> > > special case for the user wanted to navigate to the same url but didn't
> > actually
> > > click on the reload button. I suspect we may not get a value of RELOAD in
> that
> > > case.
> >
> > You are correct. Selecting the current URL in the omnibox and hitting enter
> does
> > not result in a RELOAD transition. The usage of LINK is *intentional*. When
> the
> > commit comes back to the browser, it matches the pending navigation entry
and
> we
> > classify it as SAME_PAGE.
> > The question is why is this not classified as SAME_PAGE in PlzNavigate here.
> It
> > looks like we have a browser test for that -
> > NavigationTypeClassification_SamePage, which is passing.
>
> I'll look into that, thanks.
Sorry, took me a while to get back to this. I remember now -- the reason is that
there's no redirect in NavigationTypeClassification_SamePage, so Blink has the
same url in its history logic, so when the same url comes in,
determineFrameLoadType() is able to convert the standard load into
FrameLoadTypeReloadMainResource. (That's what the long CL description was trying
to get at. :)
>
> >
> >
>
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
> > File content/browser/frame_host/navigation_request.cc (right):
> >
> >
>
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
> > content/browser/frame_host/navigation_request.cc:389:
> > navigation_handle_->WasServerRedirect() &&
> > On 2016/10/05 12:47:04, clamy wrote:
> > > Is the redirect part needed? Otherwise, when we create the nav request, we
> > could
> > > just determine that we're trying to navigate to the same url we navigated
> to,
> > > and that this is like a reload. This way, we don't need to do a comparison
> of
> > > the original url.
> >
> > Yeah, I'm not sure why we have the redirect check here. Actually, having a
> > redirect when loading an URL shouldn't cause us to reload the original,
should
> > it?
> >
> >
>
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
> > content/browser/frame_host/navigation_request.cc:393:
> > FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE;
> > I'm a bit afraid of such change. I do want us to do overall, as I'd like to
> > eliminate the SAME_PAGE classification, however in my naive CL for doing
that,
> > there are lots of failures. https://codereview.chromium.org/2246363003/
On 2016/10/06 00:20:19, scottmg wrote:
> On 2016/10/05 22:27:57, scottmg wrote:
> > On 2016/10/05 22:07:09, nasko (slow) wrote:
> > > I'm not sure that the hash based navigation is what is throwing this off.
I
> > > tried rewriting the test a bit to use pushState and it still failed the
same
> > > way.
> > >
> > > --- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc
> > > +++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
> > > @@ -6130,11 +6130,18 @@
> > IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
> > > // Navigate to a simple page and then perform an in-page navigation.
> > > GURL start_url(embedded_test_server()->GetURL("a.com",
"/title1.html"));
> > > EXPECT_TRUE(NavigateToURL(shell(), start_url));
> > > + EXPECT_EQ(1, web_contents->GetController().GetEntryCount());
> > >
> > > GURL same_page_url(
> > > - embedded_test_server()->GetURL("a.com", "/title1.html#foo"));
> > > - EXPECT_TRUE(NavigateToURL(shell(), same_page_url));
> > > - EXPECT_EQ(2, web_contents->GetController().GetEntryCount());
> > > + embedded_test_server()->GetURL("a.com", "/title2.html"));
> > > + {
> > > + TestNavigationObserver observer(web_contents);
> > > + std::string script =
> > > + "history.pushState({}, '', '" + same_page_url.spec() + "')";
> > > + EXPECT_TRUE(ExecuteScript(root, script));
> > > + observer.Wait();
> > > + EXPECT_EQ(2, web_contents->GetController().GetEntryCount());
> > > + }
> > >
> >
> > Yeah, sorry I shouldn't have linked with line numbers. It's not the hash
nav,
> > it's the navigate with type LINK (simulating "press enter on the url" that's
> > problematic.
> >
> > >
> >
>
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
> > > File content/browser/frame_host/navigation_controller_impl_browsertest.cc
> > > (right):
> > >
> > >
> >
>
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
> > > content/browser/frame_host/navigation_controller_impl_browsertest.cc:6054:
> > > ui::PAGE_TRANSITION_RELOAD, std::string());
> > > On 2016/10/05 12:47:04, clamy wrote:
> > > > Are we sure that in the case described above we would get a
> > > > ui::PAGE_TRANSITION_RELOAD? I know that in NavigationController we do
have
> a
> > > > special case for the user wanted to navigate to the same url but didn't
> > > actually
> > > > click on the reload button. I suspect we may not get a value of RELOAD
in
> > that
> > > > case.
> > >
> > > You are correct. Selecting the current URL in the omnibox and hitting
enter
> > does
> > > not result in a RELOAD transition. The usage of LINK is *intentional*.
When
> > the
> > > commit comes back to the browser, it matches the pending navigation entry
> and
> > we
> > > classify it as SAME_PAGE.
> > > The question is why is this not classified as SAME_PAGE in PlzNavigate
here.
Where does this happen? The only reason I could find that it works is that the
renderer tells the browser in FrameHostMsg_DidCommitProvisionalLoad_Params that
did_create_new_entry = false (again because of determineFrameLoadType)
... More debugging required I guess.
> > It
> > > looks like we have a browser test for that -
> > > NavigationTypeClassification_SamePage, which is passing.
> >
> > I'll look into that, thanks.
>
> Sorry, took me a while to get back to this. I remember now -- the reason is
that
> there's no redirect in NavigationTypeClassification_SamePage, so Blink has the
> same url in its history logic, so when the same url comes in,
> determineFrameLoadType() is able to convert the standard load into
> FrameLoadTypeReloadMainResource. (That's what the long CL description was
trying
> to get at. :)
>
> >
> > >
> > >
> >
>
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
> > > File content/browser/frame_host/navigation_request.cc (right):
> > >
> > >
> >
>
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
> > > content/browser/frame_host/navigation_request.cc:389:
> > > navigation_handle_->WasServerRedirect() &&
> > > On 2016/10/05 12:47:04, clamy wrote:
> > > > Is the redirect part needed? Otherwise, when we create the nav request,
we
> > > could
> > > > just determine that we're trying to navigate to the same url we
navigated
> > to,
> > > > and that this is like a reload. This way, we don't need to do a
comparison
> > of
> > > > the original url.
> > >
> > > Yeah, I'm not sure why we have the redirect check here. Actually, having a
> > > redirect when loading an URL shouldn't cause us to reload the original,
> should
> > > it?
> > >
> > >
> >
>
https://codereview.chromium.org/2381503003/diff/80001/content/browser/frame_h...
> > > content/browser/frame_host/navigation_request.cc:393:
> > > FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE;
> > > I'm a bit afraid of such change. I do want us to do overall, as I'd like
to
> > > eliminate the SAME_PAGE classification, however in my naive CL for doing
> that,
> > > there are lots of failures. https://codereview.chromium.org/2246363003/
Patchset #6 (id:100001) has been deleted
Sorry for missing most of this discussion, and I'm finding it a bit hard to catch up with where we stand now. It looks like PS6 is basically the same as Nasko's experiment to remove the SAME_PAGE classification (which had some problems at the time). Is the current plan to do that in the hopes that it fixes PlzNavigate as well? I'm not opposed to that if we can get it working. That said, we should be careful about regressions and possibly loop in toyoshim@ once we have something, since he knows a lot about the various types of reload-- taking a slightly different reload path can lead to very different user experiences (based on how often the cache is consulted). That said, I'd love to see https://crbug.com/536102 fixed.
On 2016/10/07 17:25:03, Charlie Reis (OOO soon) wrote: > Sorry for missing most of this discussion, and I'm finding it a bit hard to > catch up with where we stand now. It looks like PS6 is basically the same as > Nasko's experiment to remove the SAME_PAGE classification (which had some > problems at the time). Is the current plan to do that in the hopes that it > fixes PlzNavigate as well? > > I'm not opposed to that if we can get it working. That said, we should be > careful about regressions and possibly loop in toyoshim@ once we have something, > since he knows a lot about the various types of reload-- taking a slightly > different reload path can lead to very different user experiences (based on how > often the cache is consulted). > > That said, I'd love to see https://crbug.com/536102 fixed. Sorry, have been experimenting with different patchsets to run tryjobs. The summary of the problem is that we currently rely on determineFrameLoadType() in Blink to return FrameLoadTypeReloadMainResource when the same url is being committed. (This causes DidCommitProvisionalLoad did_create_new_entry=false so that ClassifyNavigation can get to the SAME_PAGE case). In this test, we history.replaceState() a url that contains a redirect, followed by a load of that url, and expect it to behave as if it's "reload-ish". But this doesn't work in PlzNavigate because the renderer never sees the un-redirected URL, so it doesn't match the one that's been pushed into the history as far as Blink knows, so it treats it like a regular load.
On 2016/10/07 17:39:24, scottmg wrote: > On 2016/10/07 17:25:03, Charlie Reis (OOO soon) wrote: > > Sorry for missing most of this discussion, and I'm finding it a bit hard to > > catch up with where we stand now. It looks like PS6 is basically the same as > > Nasko's experiment to remove the SAME_PAGE classification (which had some > > problems at the time). Is the current plan to do that in the hopes that it > > fixes PlzNavigate as well? > > > > I'm not opposed to that if we can get it working. That said, we should be > > careful about regressions and possibly loop in toyoshim@ once we have > something, > > since he knows a lot about the various types of reload-- taking a slightly > > different reload path can lead to very different user experiences (based on > how > > often the cache is consulted). > > > > That said, I'd love to see https://crbug.com/536102 fixed. > > Sorry, have been experimenting with different patchsets to run tryjobs. > > The summary of the problem is that we currently rely on determineFrameLoadType() > in Blink to return FrameLoadTypeReloadMainResource when the same url is being > committed. (This causes DidCommitProvisionalLoad did_create_new_entry=false so > that ClassifyNavigation can get to the SAME_PAGE case). > > In this test, we history.replaceState() a url that contains a redirect, followed > by a load of that url, and expect it to behave as if it's "reload-ish". > > But this doesn't work in PlzNavigate because the renderer never sees the > un-redirected URL, so it doesn't match the one that's been pushed into the > history as far as Blink knows, so it treats it like a regular load. (PS#5 was my hokey attempt at catching just the right conditions in the NavigationRequest to make only this case into a reload.)
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/07 17:41:02, scottmg wrote: > On 2016/10/07 17:39:24, scottmg wrote: > > On 2016/10/07 17:25:03, Charlie Reis (OOO soon) wrote: > > > Sorry for missing most of this discussion, and I'm finding it a bit hard to > > > catch up with where we stand now. It looks like PS6 is basically the same > as > > > Nasko's experiment to remove the SAME_PAGE classification (which had some > > > problems at the time). Is the current plan to do that in the hopes that it > > > fixes PlzNavigate as well? > > > > > > I'm not opposed to that if we can get it working. That said, we should be > > > careful about regressions and possibly loop in toyoshim@ once we have > > something, > > > since he knows a lot about the various types of reload-- taking a slightly > > > different reload path can lead to very different user experiences (based on > > how > > > often the cache is consulted). > > > > > > That said, I'd love to see https://crbug.com/536102 fixed. > > > > Sorry, have been experimenting with different patchsets to run tryjobs. > > > > The summary of the problem is that we currently rely on > determineFrameLoadType() > > in Blink to return FrameLoadTypeReloadMainResource when the same url is being > > committed. (This causes DidCommitProvisionalLoad did_create_new_entry=false so > > that ClassifyNavigation can get to the SAME_PAGE case). > > > > In this test, we history.replaceState() a url that contains a redirect, > followed > > by a load of that url, and expect it to behave as if it's "reload-ish". > > > > But this doesn't work in PlzNavigate because the renderer never sees the > > un-redirected URL, so it doesn't match the one that's been pushed into the > > history as far as Blink knows, so it treats it like a regular load. > > (PS#5 was my hokey attempt at catching just the right conditions in the > NavigationRequest to make only this case into a reload.) I wonder how does a reload that ends up with a redirect work today (pre-PlzNavigate). If the behavior of reload with redirect is the same as what we are observing with PlzNavigate and hitting enter in the Omnibox, then it might be fine to standardize on it and not fight fixing it in PlzNavigate. scottmg@ would you be able to help digging into this?
The CQ bit was unchecked by scottmg@chromium.org
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame's last committed url is the original url we navigated to, that we're doing a redirect, and that the page transition is a reload, in which case we tell Blink to do a reload of the main resource, rather than a normal load. R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame node's last committed url is the original url we navigated to there, and that we're not just mucking with history navigation, in which case we convert the normal load to a reload. R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi again, I think this approach is nicer now. We can just switch to a reload in RequestNavigation() when the FTN is doing a load of the same URL. Does this seem reasonable?
Thanks! Lgtm. https://codereview.chromium.org/2381503003/diff/180001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2381503003/diff/180001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:1101: if (navigation_type == FrameMsg_Navigate_Type::NORMAL && nit: add a comment explaining that a normal load of the current url should be treated as a reload?
Please wait another day, as I'd like Charlie to have a chance to look at this and he is out of office today. https://codereview.chromium.org/2381503003/diff/180001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2381503003/diff/180001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:1104: navigation_type = FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE; I think we are best doing this for both current navigation code and PlzNavigate. I'd rather not have the two paths behave differently and the plus of doing it for current navigation is that we can eliminate the SAME_PAGE navigation classification.
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #10 (id:200001) has been deleted
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
https://codereview.chromium.org/2381503003/diff/180001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2381503003/diff/180001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:1101: if (navigation_type == FrameMsg_Navigate_Type::NORMAL && On 2016/10/10 11:48:02, clamy wrote: > nit: add a comment explaining that a normal load of the current url should be > treated as a reload? Done. https://codereview.chromium.org/2381503003/diff/180001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:1104: navigation_type = FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE; On 2016/10/10 22:04:22, nasko (slow) wrote: > I think we are best doing this for both current navigation code and PlzNavigate. > I'd rather not have the two paths behave differently and the plus of doing it > for current navigation is that we can eliminate the SAME_PAGE navigation > classification. I tried this, but Android seems to have some legacy behaviour in webview as tested by LoadDataWithBaseUrlTest#testHistoryUrl that fails after changing this for everywhere. I'm looking into the pain that is Android now, but assuming that's fixable, I won't attempt the removal of SAME_PAGE in this CL but rather defer it to another. (Based on Charlie's calendar it looks like he likely won't get to this until next week.)
On 2016/10/11 19:52:32, scottmg (ooo until 11oct2016) wrote: > https://codereview.chromium.org/2381503003/diff/180001/content/browser/frame_... > File content/browser/frame_host/navigator_impl.cc (right): > > https://codereview.chromium.org/2381503003/diff/180001/content/browser/frame_... > content/browser/frame_host/navigator_impl.cc:1101: if (navigation_type == > FrameMsg_Navigate_Type::NORMAL && > On 2016/10/10 11:48:02, clamy wrote: > > nit: add a comment explaining that a normal load of the current url should be > > treated as a reload? > > Done. > > https://codereview.chromium.org/2381503003/diff/180001/content/browser/frame_... > content/browser/frame_host/navigator_impl.cc:1104: navigation_type = > FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE; > On 2016/10/10 22:04:22, nasko (slow) wrote: > > I think we are best doing this for both current navigation code and > PlzNavigate. > > I'd rather not have the two paths behave differently and the plus of doing it > > for current navigation is that we can eliminate the SAME_PAGE navigation > > classification. > > I tried this, but Android seems to have some legacy behaviour in webview as > tested by LoadDataWithBaseUrlTest#testHistoryUrl that fails after changing this > for everywhere. Aha, Android fails because that test expects that this API https://developer.android.com/reference/android/webkit/WebView.html#loadDataW..., java.lang.String, java.lang.String, java.lang.String, java.lang.String) [ what a silly url that is, you need all the spaces and args after the hash :( ] pretends to be a different URL. The change here modifies NavigatorImpl to compare FrameTreeNodes current url vs. NavigationEntry url, but in this test, the same url is loaded twice with different *virtual* urls. So it expects that both different virtual urls will appear in history. This seems like a reasonable problem, so I'll try to make a non-Android-WebView test to catch this I guess first. And then see about fixing it here. The FrameTreeNode doesn't know the virtual url afaict, which seems like it might be the most straightforward way around this. Does it makes sense to add that knowledge to FrameTreeNode? > > I'm looking into the pain that is Android now, but assuming that's fixable, I > won't attempt the removal of SAME_PAGE in this CL but rather defer it to > another. > > (Based on Charlie's calendar it looks like he likely won't get to this until > next week.)
On 2016/10/11 21:10:12, scottmg (ooo until 11oct2016) wrote: > On 2016/10/11 19:52:32, scottmg (ooo until 11oct2016) wrote: > > > https://codereview.chromium.org/2381503003/diff/180001/content/browser/frame_... > > File content/browser/frame_host/navigator_impl.cc (right): > > > > > https://codereview.chromium.org/2381503003/diff/180001/content/browser/frame_... > > content/browser/frame_host/navigator_impl.cc:1101: if (navigation_type == > > FrameMsg_Navigate_Type::NORMAL && > > On 2016/10/10 11:48:02, clamy wrote: > > > nit: add a comment explaining that a normal load of the current url should > be > > > treated as a reload? > > > > Done. > > > > > https://codereview.chromium.org/2381503003/diff/180001/content/browser/frame_... > > content/browser/frame_host/navigator_impl.cc:1104: navigation_type = > > FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE; > > On 2016/10/10 22:04:22, nasko (slow) wrote: > > > I think we are best doing this for both current navigation code and > > PlzNavigate. > > > I'd rather not have the two paths behave differently and the plus of doing > it > > > for current navigation is that we can eliminate the SAME_PAGE navigation > > > classification. > > > > I tried this, but Android seems to have some legacy behaviour in webview as > > tested by LoadDataWithBaseUrlTest#testHistoryUrl that fails after changing > this > > for everywhere. > > Aha, Android fails because that test expects that this API > > https://developer.android.com/reference/android/webkit/WebView.html#loadDataW..., > java.lang.String, java.lang.String, java.lang.String, java.lang.String) > > [ what a silly url that is, you need all the spaces and args after the hash :( ] > > pretends to be a different URL. > > The change here modifies NavigatorImpl to compare FrameTreeNodes current url vs. > NavigationEntry url, but in this test, the same url is loaded twice with > different *virtual* urls. So it expects that both different virtual urls will > appear in history. > > This seems like a reasonable problem, so I'll try to make a non-Android-WebView > test to catch this I guess first. And then see about fixing it here. The > FrameTreeNode doesn't know the virtual url afaict, which seems like it might be > the most straightforward way around this. Does it makes sense to add that > knowledge to FrameTreeNode? Nah, virtual URL is something that only belongs to NavigationEntry, as it is mostly used for URL display in the omnibox IIUC. Since FrameTreeNode is for all frames and virtual URLs are only for the main frame, it doesn't make sense to put it on FTN. We have a few LoadDataWithBaseURL tests in NavigationControllerBrowserTest. Are those enough coverage? > > I'm looking into the pain that is Android now, but assuming that's fixable, I > > won't attempt the removal of SAME_PAGE in this CL but rather defer it to > > another. Yes, killing SAME_PAGE is definitely deserving of a separate CL, if only to be able to easily bisect. > > (Based on Charlie's calendar it looks like he likely won't get to this until > > next week.)
On 2016/10/11 21:19:03, nasko (slow) wrote: > On 2016/10/11 21:10:12, scottmg (ooo until 11oct2016) wrote: > > On 2016/10/11 19:52:32, scottmg (ooo until 11oct2016) wrote: > > > > > > https://codereview.chromium.org/2381503003/diff/180001/content/browser/frame_... > > > File content/browser/frame_host/navigator_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2381503003/diff/180001/content/browser/frame_... > > > content/browser/frame_host/navigator_impl.cc:1101: if (navigation_type == > > > FrameMsg_Navigate_Type::NORMAL && > > > On 2016/10/10 11:48:02, clamy wrote: > > > > nit: add a comment explaining that a normal load of the current url should > > be > > > > treated as a reload? > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2381503003/diff/180001/content/browser/frame_... > > > content/browser/frame_host/navigator_impl.cc:1104: navigation_type = > > > FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE; > > > On 2016/10/10 22:04:22, nasko (slow) wrote: > > > > I think we are best doing this for both current navigation code and > > > PlzNavigate. > > > > I'd rather not have the two paths behave differently and the plus of doing > > it > > > > for current navigation is that we can eliminate the SAME_PAGE navigation > > > > classification. > > > > > > I tried this, but Android seems to have some legacy behaviour in webview as > > > tested by LoadDataWithBaseUrlTest#testHistoryUrl that fails after changing > > this > > > for everywhere. > > > > Aha, Android fails because that test expects that this API > > > > > https://developer.android.com/reference/android/webkit/WebView.html#loadDataW..., > > java.lang.String, java.lang.String, java.lang.String, java.lang.String) > > > > [ what a silly url that is, you need all the spaces and args after the hash :( > ] > > > > pretends to be a different URL. > > > > The change here modifies NavigatorImpl to compare FrameTreeNodes current url > vs. > > NavigationEntry url, but in this test, the same url is loaded twice with > > different *virtual* urls. So it expects that both different virtual urls will > > appear in history. > > > > This seems like a reasonable problem, so I'll try to make a > non-Android-WebView > > test to catch this I guess first. And then see about fixing it here. The > > FrameTreeNode doesn't know the virtual url afaict, which seems like it might > be > > the most straightforward way around this. Does it makes sense to add that > > knowledge to FrameTreeNode? > > Nah, virtual URL is something that only belongs to NavigationEntry, as it is > mostly used for URL display in the omnibox IIUC. Since FrameTreeNode is for all > frames and virtual URLs are only for the main frame, it doesn't make sense to > put it on FTN. > > We have a few LoadDataWithBaseURL tests in NavigationControllerBrowserTest. Are > those enough coverage? No, they don't fail with this change, the android webview one was the only place that caught it. I added a test near there that demonstrates the problem in ps#11. > > > > I'm looking into the pain that is Android now, but assuming that's fixable, > I > > > won't attempt the removal of SAME_PAGE in this CL but rather defer it to > > > another. > > Yes, killing SAME_PAGE is definitely deserving of a separate CL, if only to be > able to easily bisect. > > > > (Based on Charlie's calendar it looks like he likely won't get to this until > > > next week.)
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #13 (id:280001) has been deleted
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #13 (id:300001) has been deleted
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
On 2016/10/11 21:37:52, scottmg wrote: > On 2016/10/11 21:19:03, nasko (slow) wrote: > > On 2016/10/11 21:10:12, scottmg (ooo until 11oct2016) wrote: > > > On 2016/10/11 19:52:32, scottmg (ooo until 11oct2016) wrote: > > > > > > > > > > https://codereview.chromium.org/2381503003/diff/180001/content/browser/frame_... > > > > File content/browser/frame_host/navigator_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2381503003/diff/180001/content/browser/frame_... > > > > content/browser/frame_host/navigator_impl.cc:1101: if (navigation_type == > > > > FrameMsg_Navigate_Type::NORMAL && > > > > On 2016/10/10 11:48:02, clamy wrote: > > > > > nit: add a comment explaining that a normal load of the current url > should > > > be > > > > > treated as a reload? > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/2381503003/diff/180001/content/browser/frame_... > > > > content/browser/frame_host/navigator_impl.cc:1104: navigation_type = > > > > FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE; > > > > On 2016/10/10 22:04:22, nasko (slow) wrote: > > > > > I think we are best doing this for both current navigation code and > > > > PlzNavigate. > > > > > I'd rather not have the two paths behave differently and the plus of > doing > > > it > > > > > for current navigation is that we can eliminate the SAME_PAGE navigation > > > > > classification. > > > > > > > > I tried this, but Android seems to have some legacy behaviour in webview > as > > > > tested by LoadDataWithBaseUrlTest#testHistoryUrl that fails after changing > > > this > > > > for everywhere. > > > > > > Aha, Android fails because that test expects that this API > > > > > > > > > https://developer.android.com/reference/android/webkit/WebView.html#loadDataW..., > > > java.lang.String, java.lang.String, java.lang.String, java.lang.String) > > > > > > [ what a silly url that is, you need all the spaces and args after the hash > :( > > ] > > > > > > pretends to be a different URL. > > > > > > The change here modifies NavigatorImpl to compare FrameTreeNodes current url > > vs. > > > NavigationEntry url, but in this test, the same url is loaded twice with > > > different *virtual* urls. So it expects that both different virtual urls > will > > > appear in history. > > > > > > This seems like a reasonable problem, so I'll try to make a > > non-Android-WebView > > > test to catch this I guess first. And then see about fixing it here. The > > > FrameTreeNode doesn't know the virtual url afaict, which seems like it might > > be > > > the most straightforward way around this. Does it makes sense to add that > > > knowledge to FrameTreeNode? > > > > Nah, virtual URL is something that only belongs to NavigationEntry, as it is > > mostly used for URL display in the omnibox IIUC. Since FrameTreeNode is for > all > > frames and virtual URLs are only for the main frame, it doesn't make sense to > > put it on FTN. > > > > We have a few LoadDataWithBaseURL tests in NavigationControllerBrowserTest. > Are > > those enough coverage? > > No, they don't fail with this change, the android webview one was the only place > that caught it. I added a test near there that demonstrates the problem in > ps#11. > OK, PTAnotherL. PS#13 fixes the newly found case by checking for a non-empty GetHistoryURLForDataURL(). It was surprising to me that checking for non-emptiness was sufficient, because I would have thought in the non-PlzNav case Blink would have currently converted that to a SAME_PAGE. But I guess it also doesn't know enough to do so. So this feels like sorta-slightly incorrect behaviour, but the change is in the right place now in that it affects both PlzNav and non-PlzNav, and as demonstrated by the new test, I _think_ doesn't change behaviour in either mode. > > > > > > I'm looking into the pain that is Android now, but assuming that's > fixable, > > I > > > > won't attempt the removal of SAME_PAGE in this CL but rather defer it to > > > > another. > > > > Yes, killing SAME_PAGE is definitely deserving of a separate CL, if only to be > > able to easily bisect. > > > > > > (Based on Charlie's calendar it looks like he likely won't get to this > until > > > > next week.)
https://codereview.chromium.org/2381503003/diff/320001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2381503003/diff/320001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:85: if (frame_tree_node->current_url() == entry.GetURL() && Sanity check: this is only called for browser-initiated navigations in PlzNavigate. What currently happens if for whatever reason we have a renderer-initiated navigation to the current url (in PlzNavigate & in the current architecture)? Does it even start? If so, is it classified as same page?
It looks like this version is causing http/tests/navigation/post-goback-same-url.html to fail with --site-per-process. Overall I like the idea, but I'm a bit confused about the new test. Is it the case that loading data URL for the WebView use case is expected to create a new session history item even for the "hit the enter in the omnibox" case? https://codereview.chromium.org/2381503003/diff/320001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2381503003/diff/320001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6194: const GURL data_url = GURL("data:text/html;charset=utf-8," + data); nit: Wouldn't it work the same if you don't use the assignment? https://codereview.chromium.org/2381503003/diff/320001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6244: // All of these cause history entries. Hmm, the whole idea of the hitting Enter with the same URL is to *not* cause a new entry. Or is this specific to the load data URL behavior?
creis@chromium.org changed reviewers: + boliu@chromium.org
[+boliu for LoadDataWithBaseURL behavior question] Putting the extra logic to decide reload behavior in NavigatorImpl (rather than NavigationControllerImpl) seems a bit unfortunate to me, but I can see that it seems to fit in ok there. I could be ok with this approach if we get it to work in --site-per-process. Just clarifying: we would be able to remove NAVIGATION_TYPE_SAME_PAGE after this CL, right? :) https://codereview.chromium.org/2381503003/diff/320001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2381503003/diff/320001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6183: SameURLNavigationWithDifferentVirtualURL) { nit: Can we indicate this test is about LoadDataWithBaseURL? That's only one of many uses of virtual URLs (and probably the most confusing one). It's almost exclusively for Android Webview, though I think Chrome Apps <webview> added a way to use it as well. I'd suggest putting it with the other LoadDataWithBaseURL tests in this file as well, which are up around line 100. https://codereview.chromium.org/2381503003/diff/320001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6222: // Navigate to the same URL with *same* virtual URL. We should be checking whether these are creating new entries or not as we go. I would expect the previous 2 to create new entries, but not this one, so I'm surprised by the result you show below. (Then again, I've learned LoadDataWithBaseURL never does anything I'd expect anyway.) :( https://codereview.chromium.org/2381503003/diff/320001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6244: // All of these cause history entries. On 2016/10/13 00:06:16, nasko (slow) wrote: > Hmm, the whole idea of the hitting Enter with the same URL is to *not* cause a > new entry. Or is this specific to the load data URL behavior? Agreed, seems this like would be 2 rather than 4 if we were navigating normally. Maybe this is just how LoadDataWithBaseURL behaves. boliu@: Is this expected? https://codereview.chromium.org/2381503003/diff/320001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2381503003/diff/320001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:88: return FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE; I'm curious, does this list of conditions correspond to something in Blink that would have converted it to a reload? I'm a bit nervous about regressions if we just came up with it from scratch, but maybe we can convince ourselves it's ok.
https://codereview.chromium.org/2381503003/diff/320001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2381503003/diff/320001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6244: // All of these cause history entries. On 2016/10/15 00:00:49, Charlie Reis wrote: > On 2016/10/13 00:06:16, nasko (slow) wrote: > > Hmm, the whole idea of the hitting Enter with the same URL is to *not* cause a > > new entry. Or is this specific to the load data URL behavior? > > Agreed, seems this like would be 2 rather than 4 if we were navigating normally. > Maybe this is just how LoadDataWithBaseURL behaves. > > boliu@: Is this expected? Hmm, I don't think spec-ed out much, and webview apps probably doesn't care that much either way. If we can refresh LoadDataWithBaseURL loads correctly now, then 2 history entries is probably ok.
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
Patchset #19 (id:440001) has been deleted
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for the really slow turnaround on this change. It's been at the bottom of my pile for a while, and I had to do some experimentation to figure out all the corner cases (e.g. the webview case). The change is now in NavigationControllerImpl, behaviour is maintained in all the cases I could find, and I think the conditions checked there are all fairly logical and correspond to the logic at the end of FrameLoader::determineFrameLoadType() (plus some ensure we're considering the right frame, and for virtual urls). On 2016/10/15 00:00:50, Charlie Reis wrote: > [+boliu for LoadDataWithBaseURL behavior question] > > Putting the extra logic to decide reload behavior in NavigatorImpl (rather than > NavigationControllerImpl) seems a bit unfortunate to me, but I can see that it > seems to fit in ok there. I could be ok with this approach if we get it to work > in --site-per-process. I didn't really investigate putting it in NavigationControllerImpl, but I have now in the latest patch set. > > Just clarifying: we would be able to remove NAVIGATION_TYPE_SAME_PAGE after this > CL, right? :) I haven't fully tested that in all configurations to confirm, but I believe we should be able to. > > https://codereview.chromium.org/2381503003/diff/320001/content/browser/frame_... > File content/browser/frame_host/navigation_controller_impl_browsertest.cc > (right): > > https://codereview.chromium.org/2381503003/diff/320001/content/browser/frame_... > content/browser/frame_host/navigation_controller_impl_browsertest.cc:6183: > SameURLNavigationWithDifferentVirtualURL) { > nit: Can we indicate this test is about LoadDataWithBaseURL? That's only one of > many uses of virtual URLs (and probably the most confusing one). It's almost > exclusively for Android Webview, though I think Chrome Apps <webview> added a > way to use it as well. > > I'd suggest putting it with the other LoadDataWithBaseURL tests in this file as > well, which are up around line 100. > > https://codereview.chromium.org/2381503003/diff/320001/content/browser/frame_... > content/browser/frame_host/navigation_controller_impl_browsertest.cc:6222: // > Navigate to the same URL with *same* virtual URL. > We should be checking whether these are creating new entries or not as we go. I > would expect the previous 2 to create new entries, but not this one, so I'm > surprised by the result you show below. (Then again, I've learned > LoadDataWithBaseURL never does anything I'd expect anyway.) :( > > https://codereview.chromium.org/2381503003/diff/320001/content/browser/frame_... > content/browser/frame_host/navigation_controller_impl_browsertest.cc:6244: // > All of these cause history entries. > On 2016/10/13 00:06:16, nasko (slow) wrote: > > Hmm, the whole idea of the hitting Enter with the same URL is to *not* cause a > > new entry. Or is this specific to the load data URL behavior? > > Agreed, seems this like would be 2 rather than 4 if we were navigating normally. > Maybe this is just how LoadDataWithBaseURL behaves. > > boliu@: Is this expected? > > https://codereview.chromium.org/2381503003/diff/320001/content/browser/frame_... > File content/browser/frame_host/navigator_impl.cc (right): > > https://codereview.chromium.org/2381503003/diff/320001/content/browser/frame_... > content/browser/frame_host/navigator_impl.cc:88: return > FrameMsg_Navigate_Type::RELOAD_MAIN_RESOURCE; > I'm curious, does this list of conditions correspond to something in Blink that > would have converted it to a reload? I'm a bit nervous about regressions if we > just came up with it from scratch, but maybe we can convince ourselves it's ok.
Wow, it looks so simple in retrospect! (Granted, with a long list of conditions to check.) Interesting note: Per https://crbug.com/660388, I half expected this change to cause us to restore scroll position for enter-in-omnibox (even after reverting toyoshim's r416855). I tried undoing toyoshim's change to FrameLoader.cpp and applying this CL, though, and we still don't restore scroll position. That's good-- I'm glad this change doesn't affect that, though I'm not entirely clear why. That means I don't see any downsides to this. Thanks for sticking with it! LGTM with comment nit. https://codereview.chromium.org/2381503003/diff/480001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2381503003/diff/480001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:1792: // Note that we don't want to convert to a reload for history navigations, so nit: It's not obvious what this block is doing at first glance, so I'd recommend starting with something like "Convert enter-in-the-omnibox to a reload, since that's how Blink treats it anyway." https://codereview.chromium.org/2381503003/diff/480001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:1801: pending_entry_->GetVirtualURL()) { Just as a sanity check, I patched this in and tried it on chrome://settings, which seems to do the right thing (both GetURL and GetVirtualURL match between pending and last committed, though not each other).
On 2016/11/01 21:18:39, Charlie Reis wrote: > Wow, it looks so simple in retrospect! (Granted, with a long list of conditions > to check.) Yes, I fear that someone who knew the code better could have arrived here much more quickly. Oh well. :/ > > Interesting note: Per https://crbug.com/660388, I half expected this change to > cause us to restore scroll position for enter-in-omnibox (even after reverting > toyoshim's r416855). I tried undoing toyoshim's change to FrameLoader.cpp and > applying this CL, though, and we still don't restore scroll position. That's > good-- I'm glad this change doesn't affect that, though I'm not entirely clear > why. I will go ahead on landing this then, with fingers crossed. Thanks for patching in to check, and for the suggestions and review. > > That means I don't see any downsides to this. Thanks for sticking with it! > LGTM with comment nit. > > https://codereview.chromium.org/2381503003/diff/480001/content/browser/frame_... > File content/browser/frame_host/navigation_controller_impl.cc (right): > > https://codereview.chromium.org/2381503003/diff/480001/content/browser/frame_... > content/browser/frame_host/navigation_controller_impl.cc:1792: // Note that we > don't want to convert to a reload for history navigations, so > nit: It's not obvious what this block is doing at first glance, so I'd recommend > starting with something like "Convert enter-in-the-omnibox to a reload, since > that's how Blink treats it anyway." > > https://codereview.chromium.org/2381503003/diff/480001/content/browser/frame_... > content/browser/frame_host/navigation_controller_impl.cc:1801: > pending_entry_->GetVirtualURL()) { > Just as a sanity check, I patched this in and tried it on chrome://settings, > which seems to do the right thing (both GetURL and GetVirtualURL match between > pending and last committed, though not each other).
https://codereview.chromium.org/2381503003/diff/480001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2381503003/diff/480001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:1792: // Note that we don't want to convert to a reload for history navigations, so On 2016/11/01 21:18:40, Charlie Reis wrote: > nit: It's not obvious what this block is doing at first glance, so I'd recommend > starting with something like "Convert enter-in-the-omnibox to a reload, since > that's how Blink treats it anyway." Done. https://codereview.chromium.org/2381503003/diff/480001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:1801: pending_entry_->GetVirtualURL()) { On 2016/11/01 21:18:40, Charlie Reis wrote: > Just as a sanity check, I patched this in and tried it on chrome://settings, > which seems to do the right thing (both GetURL and GetVirtualURL match between > pending and last committed, though not each other). Thanks for checking!
The CQ bit was checked by scottmg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2381503003/#ps500001 (title: "update comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Description was changed from ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame node's last committed url is the original url we navigated to there, and that we're not just mucking with history navigation, in which case we convert the normal load to a reload. R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027, 536102 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame node's last committed url is the original url we navigated to there, and that we're not just mucking with history navigation, in which case we convert the normal load to a reload. R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027, 536102 ==========
The CQ bit was checked by scottmg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame node's last committed url is the original url we navigated to there, and that we're not just mucking with history navigation, in which case we convert the normal load to a reload. R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027, 536102 ========== to ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame node's last committed url is the original url we navigated to there, and that we're not just mucking with history navigation, in which case we convert the normal load to a reload. R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027, 536102 ==========
Message was sent while issue was closed.
Committed patchset #21 (id:500001)
Message was sent while issue was closed.
Description was changed from ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame node's last committed url is the original url we navigated to there, and that we're not just mucking with history navigation, in which case we convert the normal load to a reload. R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027, 536102 ========== to ========== PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… This test is failing in PlzNav because https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... is creating a new navigation entry, so the GoBack() goes to the wrong place. It's doing that because FrameHostMsg_DidCommitProvisionalLoad_Params.did_create_new_entry is true. That in turn is because SendDidCommitProvisionalLoad is getting a commit type of WebStandardCommit (not WebHistoryInertCommit like it does in non-PlzNav). *That* is because here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... the url being checked is the already-redirected one in the PlzNavigate case (sub.a.com), but m_documentLoader->urlForHistory() is the pre-redirected one (a.com/server-redirect?sub...). So, this works in non-PlzNav because Blink tells us that it should replace the current entry (i.e. it's like a reload). Instead of relying on Blink to determine that, check if the render frame node's last committed url is the original url we navigated to there, and that we're not just mucking with history navigation, in which case we convert the normal load to a reload. R=nasko@chromium.org,clamy@chromium.org BUG=630103,475027, 536102 Committed: https://crrev.com/28bbbb16194c7337e22c3e0bda752ae6b4b681cf Cr-Commit-Position: refs/heads/master@{#429302} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/28bbbb16194c7337e22c3e0bda752ae6b4b681cf Cr-Commit-Position: refs/heads/master@{#429302}
Message was sent while issue was closed.
A revert of this CL (patchset #21 id:500001) has been created in https://codereview.chromium.org/2495373002/ by scottmg@chromium.org. The reason for reverting is: https://crbug.com/664319.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
