content/browser/frame_host/navigation_controller_impl_browsertest.cc - Issue 2381503003: PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga…

Side by Side Diff: content/browser/frame_host/navigation_controller_impl_browsertest.cc

Issue 2381503003: PlzNav: Fix NavigationControllerBrowserTest.EnsureSamePageNavigationUpdatesFrameNaviga… (Closed)

Patch Set: improve test (3) Created 4 years, 2 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

OLD	NEW
1 // Copyright 2014 The Chromium Authors. All rights reserved.	1 // Copyright 2014 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 #include "content/browser/frame_host/navigation_controller_impl.h"	5 #include "content/browser/frame_host/navigation_controller_impl.h"

6	6

7 #include <stdint.h>	7 #include <stdint.h>

8 #include <utility>	8 #include <utility>

9	9

10 #include "base/bind.h"	10 #include "base/bind.h"

(...skipping 6139 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
6150	6150

6151 // Simulate the user hitting Enter in the omnibox without changing the URL.	6151 // Simulate the user hitting Enter in the omnibox without changing the URL.

6152 {	6152 {

6153 TestNavigationObserver observer(web_contents);	6153 TestNavigationObserver observer(web_contents);

6154 web_contents->GetController().LoadURL(web_contents->GetLastCommittedURL(),	6154 web_contents->GetController().LoadURL(web_contents->GetLastCommittedURL(),

6155 Referrer(), ui::PAGE_TRANSITION_LINK,	6155 Referrer(), ui::PAGE_TRANSITION_LINK,

6156 std::string());	6156 std::string());

6157 observer.Wait();	6157 observer.Wait();

6158 }	6158 }

6159	6159

	6160 EXPECT_EQ(2, web_contents->GetController().GetEntryCount());

	6161

6160 // Prior to fixing the issue, the above omnibox navigation (which is	6162 // Prior to fixing the issue, the above omnibox navigation (which is

6161 // classified as SAME_PAGE) was leaving the FrameNavigationEntry with the	6163 // classified as SAME_PAGE) was leaving the FrameNavigationEntry with the

6162 // same document sequence number as the previous entry but updates the URL.	6164 // same document sequence number as the previous entry but updates the URL.

6163 // Doing a back session history navigation now will cause the browser to	6165 // Doing a back session history navigation now will cause the browser to

6164 // consider it as in-page because of this matching document sequence number	6166 // consider it as in-page because of this matching document sequence number

6165 // and lead to a mismatch of origin and URL in the renderer process.	6167 // and lead to a mismatch of origin and URL in the renderer process.

6166 {	6168 {

6167 TestNavigationObserver observer(web_contents);	6169 TestNavigationObserver observer(web_contents);

6168 web_contents->GetController().GoBack();	6170 web_contents->GetController().GoBack();

6169 observer.Wait();	6171 observer.Wait();

6170 }	6172 }

6171	6173

6172 // Verify the expected origin through JavaScript. It also has the additional	6174 // Verify the expected origin through JavaScript. It also has the additional

6173 // verification of the process also being still alive.	6175 // verification of the process also being still alive.

6174 std::string origin;	6176 std::string origin;

6175 EXPECT_TRUE(ExecuteScriptAndExtractString(	6177 EXPECT_TRUE(ExecuteScriptAndExtractString(

6176 web_contents, "domAutomationController.send(document.origin)", &origin));	6178 web_contents, "domAutomationController.send(document.origin)", &origin));

6177 EXPECT_EQ(start_url.GetOrigin().spec(), origin + "/");	6179 EXPECT_EQ(start_url.GetOrigin().spec(), origin + "/");

6178 }	6180 }

6179	6181

	6182 IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,

	6183 SameURLNavigationWithDifferentVirtualURL) {
	Charlie Reis 2016/10/15 00:00:49 nit: Can we indicate this test is about LoadDataWi 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.
	6184 WebContentsImpl* web_contents =

	6185 static_cast<WebContentsImpl*>(shell()->web_contents());

	6186 const NavigationControllerImpl& controller =

	6187 static_cast<const NavigationControllerImpl&>(

	6188 web_contents->GetController());

	6189

	6190 const GURL base_url("http://baseurl");

	6191 const GURL virtual_url1("http://virtual1");

	6192 const GURL virtual_url2("http://virtual2");

	6193 const std::string data = "<html><body>foo</body></html>";

	6194 const GURL data_url = GURL("data:text/html;charset=utf-8," + data);
	nasko 2016/10/13 00:06:16 nit: Wouldn't it work the same if you don't use th nit: Wouldn't it work the same if you don't use the assignment?
	6195

	6196 // Navigate to a data URL with a virtual URL.

	6197 {

	6198 TestNavigationObserver observer(web_contents);

	6199 shell()->LoadDataWithBaseURL(virtual_url1, data, base_url);

	6200 observer.Wait();

	6201

	6202 NavigationEntryImpl* entry = controller.GetLastCommittedEntry();

	6203 EXPECT_EQ(base_url, entry->GetBaseURLForDataURL());

	6204 EXPECT_EQ(virtual_url1, entry->GetVirtualURL());

	6205 EXPECT_EQ(virtual_url1, entry->GetHistoryURLForDataURL());

	6206 EXPECT_EQ(data_url, entry->GetURL());

	6207 }

	6208

	6209 // Navigate to the same URL with different virtual URL.

	6210 {

	6211 TestNavigationObserver observer(web_contents);

	6212 shell()->LoadDataWithBaseURL(virtual_url2, data, base_url);

	6213 observer.Wait();

	6214

	6215 NavigationEntryImpl* entry = controller.GetLastCommittedEntry();

	6216 EXPECT_EQ(base_url, entry->GetBaseURLForDataURL());

	6217 EXPECT_EQ(virtual_url2, entry->GetVirtualURL());

	6218 EXPECT_EQ(virtual_url2, entry->GetHistoryURLForDataURL());

	6219 EXPECT_EQ(data_url, entry->GetURL());

	6220 }

	6221

	6222 // Navigate to the same URL with same virtual URL.
	Charlie Reis 2016/10/15 00:00:49 We should be checking whether these are creating n 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.) :(
	6223 {

	6224 TestNavigationObserver observer(web_contents);

	6225 shell()->LoadDataWithBaseURL(virtual_url2, data, base_url);

	6226 observer.Wait();

	6227

	6228 NavigationEntryImpl* entry = controller.GetLastCommittedEntry();

	6229 EXPECT_EQ(base_url, entry->GetBaseURLForDataURL());

	6230 EXPECT_EQ(virtual_url2, entry->GetVirtualURL());

	6231 EXPECT_EQ(virtual_url2, entry->GetHistoryURLForDataURL());

	6232 EXPECT_EQ(data_url, entry->GetURL());

	6233 }

	6234

	6235 // Simulate the user hitting Enter in the omnibox without changing the URL.

	6236 {

	6237 TestNavigationObserver observer(web_contents);

	6238 web_contents->GetController().LoadURL(web_contents->GetLastCommittedURL(),

	6239 Referrer(), ui::PAGE_TRANSITION_LINK,

	6240 std::string());

	6241 observer.Wait();

	6242 }

	6243

	6244 // All of these cause history entries.
	nasko 2016/10/13 00:06:16 Hmm, the whole idea of the hitting Enter with the 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? Charlie Reis 2016/10/15 00:00:49 Agreed, seems this like would be 2 rather than 4 i Show quoted text 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? boliu 2016/10/17 17:42:43 Hmm, I don't think spec-ed out much, and webview a Show quoted text 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.
	6245 EXPECT_EQ(4, web_contents->GetController().GetEntryCount());

	6246 }

	6247

6180 // A BrowserMessageFilter that delays FrameHostMsg_DidCommitProvisionalLoad IPC	6248 // A BrowserMessageFilter that delays FrameHostMsg_DidCommitProvisionalLoad IPC

6181 // message for a specified URL, navigates the WebContents back and then	6249 // message for a specified URL, navigates the WebContents back and then

6182 // processes the commit message.	6250 // processes the commit message.

6183 class GoBackAndCommitFilter : public BrowserMessageFilter {	6251 class GoBackAndCommitFilter : public BrowserMessageFilter {

6184 public:	6252 public:

6185 GoBackAndCommitFilter(const GURL& url, WebContentsImpl* web_contents)	6253 GoBackAndCommitFilter(const GURL& url, WebContentsImpl* web_contents)

6186 : BrowserMessageFilter(FrameMsgStart),	6254 : BrowserMessageFilter(FrameMsgStart),

6187 url_(url),	6255 url_(url),

6188 web_contents_(web_contents) {}	6256 web_contents_(web_contents) {}

6189	6257

(...skipping 360 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
6550 histogram.ExpectTotalCount(kReloadToReloadMetricName, 4);	6618 histogram.ExpectTotalCount(kReloadToReloadMetricName, 4);

6551 histogram.ExpectTotalCount(kReloadMainResourceToReloadMetricName, 3);	6619 histogram.ExpectTotalCount(kReloadMainResourceToReloadMetricName, 3);

6552	6620

6553 controller.ReloadToRefreshContent(false);	6621 controller.ReloadToRefreshContent(false);

6554 EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));	6622 EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));

6555 histogram.ExpectTotalCount(kReloadToReloadMetricName, 4);	6623 histogram.ExpectTotalCount(kReloadToReloadMetricName, 4);

6556 histogram.ExpectTotalCount(kReloadMainResourceToReloadMetricName, 3);	6624 histogram.ExpectTotalCount(kReloadMainResourceToReloadMetricName, 3);

6557 }	6625 }

6558	6626

6559 } // namespace content	6627 } // namespace content

OLD	NEW

« no previous file with comments | « no previous file | content/browser/frame_host/navigator_impl.cc » ('j') | content/browser/frame_host/navigator_impl.cc » ('J')