Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1304373007:
Browser test for LoadDataWithBaseURL reload (Closed)
|
Created:
5 years, 3 months ago by boliu Modified:
5 years ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, creis+watch_chromium.org, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix LoadDataWithBaseURL reload
On a LoadDataWithBaseURL, renderer reports the history URL
as the URL of the navigation, and is stored in the
NavigationEntry::GetURL field, overwriting the data URL
originally stored there. This causes a crash on a reload
since the data URL has been lost.
Fix this by saving the data URL DocumentState so that
renderer can return it as the URL of the load. This means
that GetURL and GetOriginalURL for a LoadDataWithBaseURL
will now return the data URL, not the history URL.
Also fix WebFrame::LoadData to not create new navigation
for a reload.
BUG=522567
Committed: https://crrev.com/15890e4b345bfef3854566899a0a462f7c5939a9
Cr-Commit-Position: refs/heads/master@{#361481}
Patch Set 1 #Patch Set 2 : check renderer alive #Patch Set 3 : potential fix #Patch Set 4 : original_data_url #Patch Set 5 : no reachableURL, check with base in DidNavigate #Patch Set 6 : renderer side #Patch Set 7 : store in DocumentState #Patch Set 8 : did navigate virtual url #Patch Set 9 : limit DidNavigate virtual url to data load #Patch Set 10 : rebase #Patch Set 11 : rebase, new test, fix blink reload, clean ups #Patch Set 12 : more clean ups #
Total comments: 23
Patch Set 13 : upload for tests, do not review #Patch Set 14 : address most review comments, try virtual URL from DidCommitProvisionalLoad #Patch Set 15 : reverts #
Total comments: 15
Patch Set 16 : new test #
Total comments: 7
Patch Set 17 : review #
Messages
Total messages: 32 (5 generated)
boliu@chromium.org changed reviewers: + creis@chromium.org
Added one workaround for the LoadDataWithBaseURL crash, but looks like it's still not quite right, and one webview test is definitely failing now, but I haven't looked into it yet. Just feels like discussion in bug isn't going anywhere, and code is a bit more concrete
On 2015/08/28 16:45:00, boliu wrote: > Added one workaround for the LoadDataWithBaseURL crash, but looks like it's > still not quite right, and one webview test is definitely failing now, but I > haven't looked into it yet. Ok, so the problem is skipping SetURL could break places where the virtual url is expected. In that test, it breaks detection for fragment navigation. Maybe the data url should be saved in say SetOriginalRequestURL, then at least reload will pick that up as the url to load I think. Not sure if it has other implications. Trying it out now.. > > Just feels like discussion in bug isn't going anywhere, and code is a bit more > concrete
On 2015/08/28 20:30:53, boliu wrote: > On 2015/08/28 16:45:00, boliu wrote: > > Added one workaround for the LoadDataWithBaseURL crash, but looks like it's > > still not quite right, and one webview test is definitely failing now, but I > > haven't looked into it yet. > > Ok, so the problem is skipping SetURL could break places where the virtual url > is expected. In that test, it breaks detection for fragment navigation. > > Maybe the data url should be saved in say SetOriginalRequestURL, then at least > reload will pick that up as the url to load I think. Not sure if it has other > implications. Trying it out now.. So OriginalRequestURL only works for RELOAD_ORIGINAL_REQUEST_URL. But I think the proper fix is something very similar, something like adding an "original data url" that should be used for reloads when it's a data url load type. WDYT? > > > > > Just feels like discussion in bug isn't going anywhere, and code is a bit more > > concrete
Hmm. I'm not thrilled with adding yet another URL that might disagree with other state on the entry and might need to be serialized for session restore, etc. I haven't had time to understand the change you're making, but I do appreciate that you're trying it out as a CL. That should make it easy for me to patch it in locally to understand where things are going wrong. I'll try to do that next week.
Friendly ping on high level review here :)
On 2015/09/08 23:57:44, boliu wrote: > Friendly ping on high level review here :) Sorry for the delay-- it's a busy week. I'm working my way through this, but I'm currently stuck trying to understand why Blink is reporting the base URL as the committed URL. Hypothetically, if it reported the data URL instead, how much would that simplify the rest of this CL? (I'll try to dig into why it's the case.)
On 2015/09/10 23:15:36, Charlie Reis (slow till 9-14) wrote: > On 2015/09/08 23:57:44, boliu wrote: > > Friendly ping on high level review here :) > > Sorry for the delay-- it's a busy week. > > I'm working my way through this, but I'm currently stuck trying to understand > why Blink is reporting the base URL as the committed URL. Hypothetically, if it > reported the data URL instead, how much would that simplify the rest of this CL? So this assumes the virtual URL is not passed to blink. That has a separate problem that two LoadDataWithBaseURL with only the virtual URL changing will be treated incorrectly by blink as a reload, because to blink nothing changed. I don't really have a good way to resolve this. But ignoring that question, if blink passed up the data url, then there is no need to save the data url separately in DocumentState. So all of the renderer side changes are not needed. Will still need the browser side NavigatorImpl change. > > (I'll try to dig into why it's the case.)
On 2015/09/10 23:26:22, boliu wrote: > On 2015/09/10 23:15:36, Charlie Reis (slow till 9-14) wrote: > > On 2015/09/08 23:57:44, boliu wrote: > > > Friendly ping on high level review here :) > > > > Sorry for the delay-- it's a busy week. > > > > I'm working my way through this, but I'm currently stuck trying to understand > > why Blink is reporting the base URL as the committed URL. Hypothetically, if > it > > reported the data URL instead, how much would that simplify the rest of this > CL? > > So this assumes the virtual URL is not passed to blink. That has a separate > problem that two LoadDataWithBaseURL with only the virtual URL changing will be > treated incorrectly by blink as a reload, because to blink nothing changed. I > don't really have a good way to resolve this. Err, let me rewrite this to avoid pronoun games... Your base url concern only matters if we no longer pass in virtual URL as the unreachableURL. But not passing in the virtual URL as the unreachableURL has another problem: two consecutive LoadDataWithBaseURL that differ only in the virtual URL will be treated incorrectly by blink as a reload, because to blink, nothing changed. I don't really have a good way to resolve this. > > But ignoring that question, if blink passed up the data url, then there is no s/that question/problem above/ > need to save the data url separately in DocumentState. So all of the renderer > side changes are not needed. Will still need the browser side NavigatorImpl > change. > > > > > (I'll try to dig into why it's the case.)
Ping on this. Quite late in m46, so mind if I merge the same crash workaround to m46? Got sidetrcked a bit during perf and had a rather different thought on potential fix. The android webview API is Load*Data*WithBaseURL, not LoadData*URL*WithBaseURL. Assuming <webview> tag API works similarly, then there is no reason why the data need to be encoded into a data: URL and saved in the url field. Instead the data should be just a different data field in NavigationEntry (like browser_initiated_post_data_). Or keep going with this fix..
On 2015/09/24 16:58:48, boliu wrote: > Ping on this. Quite late in m46, so mind if I merge the same crash workaround to > m46? Yes, sorry, I similarly fell into a perf black hole. I'm ok with merging the same workaround to M46 while we figure out the right long term behavior. > Got sidetrcked a bit during perf and had a rather different thought on potential > fix. The android webview API is Load*Data*WithBaseURL, not > LoadData*URL*WithBaseURL. Assuming <webview> tag API works similarly, then there > is no reason why the data need to be encoded into a data: URL and saved in the > url field. Instead the data should be just a different data field in > NavigationEntry (like browser_initiated_post_data_). Intriguing. What would go in the URL field in that proposal? Maybe you can write up some notes on how it would work? (I'm always hesitant to add new fields to NavigationEntry that might need to be persisted, but I'd be on board if there's a chance to clean up this feature.) > Or keep going with this fix.. Sounds like there were some concerns about navigating between two virtual URLs with the same underlying URL. (Ah, the fun cases that arise when we lie to our own code...) Let's consider your new idea a bit before diving into the previous one even further.
On 2015/09/24 17:09:15, Charlie Reis wrote: > On 2015/09/24 16:58:48, boliu wrote: > > Ping on this. Quite late in m46, so mind if I merge the same crash workaround > to > > m46? > > Yes, sorry, I similarly fell into a perf black hole. I'm ok with merging the > same workaround to M46 while we figure out the right long term behavior. > > > Got sidetrcked a bit during perf and had a rather different thought on > potential > > fix. The android webview API is Load*Data*WithBaseURL, not > > LoadData*URL*WithBaseURL. Assuming <webview> tag API works similarly, then > there > > is no reason why the data need to be encoded into a data: URL and saved in the > > url field. Instead the data should be just a different data field in > > NavigationEntry (like browser_initiated_post_data_). > > Intriguing. What would go in the URL field in that proposal? Umm, I think it would be the virtual URL goes into URL, and virtual will be empty. And base URL stays where it is. > Maybe you can > write up some notes on how it would work? (I'm always hesitant to add new > fields to NavigationEntry that might need to be persisted, but I'd be on board > if there's a chance to clean up this feature.) Sure (or I might find someone else on the webview team to investigate..) > > > Or keep going with this fix.. > > Sounds like there were some concerns about navigating between two virtual URLs > with the same underlying URL. (Ah, the fun cases that arise when we lie to our > own code...) Let's consider your new idea a bit before diving into the previous > one even further.
I'm revisiting this one (patch set 10), since we both have some concerns about the new proposal in https://codereview.chromium.org/1449793002/. This one might actually be quite close to something we can land. I like that it's keeping the various URL values where they were, and that it makes the renderer process report the data URL in params.url. No changes to serialization will be required, and we can verify that the NavigationEntry fields stay correct (as in the expanded version of the test below). My test shows there's still one main issue we need to solve-- a reload of the data URL is being classified as a new navigation, so it leads to some problems (it creates a new NavigationEntry, loses the various base_url_for_data_url_ state, we can't reload again, etc.). I'm trying to track down why this is the case in a debugger. We're kind of doing the right thing in RenderFrameImpl::NavigateInternal during the reload (i.e., calling LoadDataURL again), but Blink considers that a new navigation rather than a reload. There's probably a way to get Blink to realize it's a reload. One other question-- what's the reason that we need to change the original request URL from the history URL to the data URL? I'm not sure I understand the need for that change yet. I would suggest using a version of the code below as the new test, since it's much more explicit about what we expect in each stage. It's helping me track down the bug I mentioned above. (We may also want to add a navigation to another page and then test a back navigation, since that will have similar concerns as reload.) // Verifies that the base, history, and data URLs for LoadDataWithBaseURL end up // in the expected parts of the NavigationEntry in each stage of navigation, and // that we don't kill the renderer on reload. See https://crbug.com/522567. IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, LoadDataWithBaseURL) { const GURL base_url("http://baseurl"); const GURL history_url("http://historyurl"); const std::string data = "<html><body>foo</body></html>"; const GURL data_url = GURL("data:text/html;charset=utf-8," + data); const NavigationControllerImpl& controller = static_cast<const NavigationControllerImpl&>( shell()->web_contents()->GetController()); // Load data, but don't commit yet. TestNavigationObserver same_tab_observer(shell()->web_contents(), 1); shell()->LoadDataWithBaseURL(history_url, data, base_url); // Verify the pending NavigationEntry. NavigationEntryImpl* pending_entry = controller.GetPendingEntry(); EXPECT_EQ(base_url, pending_entry->GetBaseURLForDataURL()); EXPECT_EQ(history_url, pending_entry->GetVirtualURL()); EXPECT_EQ(history_url, pending_entry->GetHistoryURLForDataURL()); EXPECT_EQ(data_url, pending_entry->GetURL()); // Let the navigation commit. same_tab_observer.Wait(); // Verify the last committed NavigationEntry. NavigationEntryImpl* entry = controller.GetLastCommittedEntry(); EXPECT_EQ(base_url, entry->GetBaseURLForDataURL()); EXPECT_EQ(history_url, entry->GetVirtualURL()); EXPECT_EQ(history_url, entry->GetHistoryURLForDataURL()); EXPECT_EQ(data_url, entry->GetURL()); // We should use history_url instead of the base_url as the original url of // this navigation entry, because base_url is only used for resolving relative // paths in the data, or enforcing same origin policy. EXPECT_EQ(history_url, entry->GetOriginalRequestURL()); // Now reload and make sure the renderer isn't killed. ReloadBlockUntilNavigationsComplete(shell(), 1); EXPECT_TRUE(shell()->web_contents()->GetMainFrame()->IsRenderFrameLive()); // Verify the last committed NavigationEntry hasn't changed. NavigationEntryImpl* reload_entry = controller.GetLastCommittedEntry(); EXPECT_EQ(entry, reload_entry); EXPECT_EQ(base_url, reload_entry->GetBaseURLForDataURL()); EXPECT_EQ(history_url, reload_entry->GetVirtualURL()); EXPECT_EQ(history_url, reload_entry->GetHistoryURLForDataURL()); EXPECT_EQ(history_url, reload_entry->GetOriginalRequestURL()); EXPECT_EQ(data_url, reload_entry->GetURL()); }
Thanks for the detailed reply! On 2015/11/19 20:08:36, Charlie Reis wrote: > I'm revisiting this one (patch set 10), since we both have some concerns about > the new proposal in https://codereview.chromium.org/1449793002/. > > This one might actually be quite close to something we can land. I like that > it's keeping the various URL values where they were, and that it makes the > renderer process report the data URL in params.url. No changes to serialization > will be required, and we can verify that the NavigationEntry fields stay correct > (as in the expanded version of the test below). > > My test shows there's still one main issue we need to solve-- a reload of the > data URL is being classified as a new navigation, so it leads to some problems > (it creates a new NavigationEntry, loses the various base_url_for_data_url_ > state, we can't reload again, etc.). I'm trying to track down why this is the > case in a debugger. We're kind of doing the right thing in > RenderFrameImpl::NavigateInternal during the reload (i.e., calling LoadDataURL > again), but Blink considers that a new navigation rather than a reload. There's > probably a way to get Blink to realize it's a reload. This might possibly be an independent blink issue. I'll look into it. > > One other question-- what's the reason that we need to change the original > request URL from the history URL to the data URL? I'm not sure I understand the > need for that change yet. That chunk was added in PS5. I tried to look back if this was required to not break tests, but all the bot links don't work anymore. So not sure if that's required or not. Conceptually, the original URL not matching GetURL, even though there were no redirects, feels weird to me. Also if original URL is not changed, then the exact same crash would happen for a RELOAD_ORIGINAL_REQUEST_URL. > > I would suggest using a version of the code below as the new test, since it's > much more explicit about what we expect in each stage. It's helping me track > down the bug I mentioned above. (We may also want to add a navigation to > another page and then test a back navigation, since that will have similar > concerns as reload.) > > // Verifies that the base, history, and data URLs for LoadDataWithBaseURL end up > // in the expected parts of the NavigationEntry in each stage of navigation, and > // that we don't kill the renderer on reload. See https://crbug.com/522567. > IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, LoadDataWithBaseURL) { > const GURL base_url("http://baseurl"); > const GURL history_url("http://historyurl"); > const std::string data = "<html><body>foo</body></html>"; > const GURL data_url = GURL("data:text/html;charset=utf-8," + data); > > const NavigationControllerImpl& controller = > static_cast<const NavigationControllerImpl&>( > shell()->web_contents()->GetController()); > > // Load data, but don't commit yet. > TestNavigationObserver same_tab_observer(shell()->web_contents(), 1); > shell()->LoadDataWithBaseURL(history_url, data, base_url); > > // Verify the pending NavigationEntry. > NavigationEntryImpl* pending_entry = controller.GetPendingEntry(); > EXPECT_EQ(base_url, pending_entry->GetBaseURLForDataURL()); > EXPECT_EQ(history_url, pending_entry->GetVirtualURL()); > EXPECT_EQ(history_url, pending_entry->GetHistoryURLForDataURL()); > EXPECT_EQ(data_url, pending_entry->GetURL()); > > // Let the navigation commit. > same_tab_observer.Wait(); > > // Verify the last committed NavigationEntry. > NavigationEntryImpl* entry = controller.GetLastCommittedEntry(); > EXPECT_EQ(base_url, entry->GetBaseURLForDataURL()); > EXPECT_EQ(history_url, entry->GetVirtualURL()); > EXPECT_EQ(history_url, entry->GetHistoryURLForDataURL()); > EXPECT_EQ(data_url, entry->GetURL()); > > // We should use history_url instead of the base_url as the original url of > // this navigation entry, because base_url is only used for resolving relative > // paths in the data, or enforcing same origin policy. > EXPECT_EQ(history_url, entry->GetOriginalRequestURL()); > > // Now reload and make sure the renderer isn't killed. > ReloadBlockUntilNavigationsComplete(shell(), 1); > EXPECT_TRUE(shell()->web_contents()->GetMainFrame()->IsRenderFrameLive()); > > // Verify the last committed NavigationEntry hasn't changed. > NavigationEntryImpl* reload_entry = controller.GetLastCommittedEntry(); > EXPECT_EQ(entry, reload_entry); > EXPECT_EQ(base_url, reload_entry->GetBaseURLForDataURL()); > EXPECT_EQ(history_url, reload_entry->GetVirtualURL()); > EXPECT_EQ(history_url, reload_entry->GetHistoryURLForDataURL()); > EXPECT_EQ(history_url, reload_entry->GetOriginalRequestURL()); > EXPECT_EQ(data_url, reload_entry->GetURL()); > } Thanks for the test :D
On 2015/11/19 20:42:14, boliu wrote: > Thanks for the detailed reply! > > On 2015/11/19 20:08:36, Charlie Reis wrote: > > I'm revisiting this one (patch set 10), since we both have some concerns about > > the new proposal in https://codereview.chromium.org/1449793002/. > > > > This one might actually be quite close to something we can land. I like that > > it's keeping the various URL values where they were, and that it makes the > > renderer process report the data URL in params.url. No changes to > serialization > > will be required, and we can verify that the NavigationEntry fields stay > correct > > (as in the expanded version of the test below). > > > > My test shows there's still one main issue we need to solve-- a reload of the > > data URL is being classified as a new navigation, so it leads to some problems > > (it creates a new NavigationEntry, loses the various base_url_for_data_url_ > > state, we can't reload again, etc.). I'm trying to track down why this is the > > case in a debugger. We're kind of doing the right thing in > > RenderFrameImpl::NavigateInternal during the reload (i.e., calling LoadDataURL > > again), but Blink considers that a new navigation rather than a reload. > There's > > probably a way to get Blink to realize it's a reload. > > This might possibly be an independent blink issue. I'll look into it. Great, thanks. I'll take a break and write up a different fix in the meantime. > > > > > One other question-- what's the reason that we need to change the original > > request URL from the history URL to the data URL? I'm not sure I understand > the > > need for that change yet. > > That chunk was added in PS5. I tried to look back if this was required to not > break tests, but all the bot links don't work anymore. So not sure if that's > required or not. > > Conceptually, the original URL not matching GetURL, even though there were no > redirects, feels weird to me. Also if original URL is not changed, then the > exact same crash would happen for a RELOAD_ORIGINAL_REQUEST_URL. That's a fair point. If nothing else was depending on the history URL being there, then feel free to update the test expectation to be the data URL rather than the history URL (and include a comment in RenderFrameImpl about it). > > > > > I would suggest using a version of the code below as the new test, since it's > > much more explicit about what we expect in each stage. It's helping me track > > down the bug I mentioned above. (We may also want to add a navigation to > > another page and then test a back navigation, since that will have similar > > concerns as reload.) > > > > // Verifies that the base, history, and data URLs for LoadDataWithBaseURL end > up > > // in the expected parts of the NavigationEntry in each stage of navigation, > and > > // that we don't kill the renderer on reload. See https://crbug.com/522567. > > IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, LoadDataWithBaseURL) { > > const GURL base_url("http://baseurl"); > > const GURL history_url("http://historyurl"); > > const std::string data = "<html><body>foo</body></html>"; > > const GURL data_url = GURL("data:text/html;charset=utf-8," + data); > > > > const NavigationControllerImpl& controller = > > static_cast<const NavigationControllerImpl&>( > > shell()->web_contents()->GetController()); > > > > // Load data, but don't commit yet. > > TestNavigationObserver same_tab_observer(shell()->web_contents(), 1); > > shell()->LoadDataWithBaseURL(history_url, data, base_url); > > > > // Verify the pending NavigationEntry. > > NavigationEntryImpl* pending_entry = controller.GetPendingEntry(); > > EXPECT_EQ(base_url, pending_entry->GetBaseURLForDataURL()); > > EXPECT_EQ(history_url, pending_entry->GetVirtualURL()); > > EXPECT_EQ(history_url, pending_entry->GetHistoryURLForDataURL()); > > EXPECT_EQ(data_url, pending_entry->GetURL()); > > > > // Let the navigation commit. > > same_tab_observer.Wait(); > > > > // Verify the last committed NavigationEntry. > > NavigationEntryImpl* entry = controller.GetLastCommittedEntry(); > > EXPECT_EQ(base_url, entry->GetBaseURLForDataURL()); > > EXPECT_EQ(history_url, entry->GetVirtualURL()); > > EXPECT_EQ(history_url, entry->GetHistoryURLForDataURL()); > > EXPECT_EQ(data_url, entry->GetURL()); > > > > // We should use history_url instead of the base_url as the original url of > > // this navigation entry, because base_url is only used for resolving > relative > > // paths in the data, or enforcing same origin policy. > > EXPECT_EQ(history_url, entry->GetOriginalRequestURL()); > > > > // Now reload and make sure the renderer isn't killed. > > ReloadBlockUntilNavigationsComplete(shell(), 1); > > EXPECT_TRUE(shell()->web_contents()->GetMainFrame()->IsRenderFrameLive()); > > > > // Verify the last committed NavigationEntry hasn't changed. > > NavigationEntryImpl* reload_entry = controller.GetLastCommittedEntry(); > > EXPECT_EQ(entry, reload_entry); > > EXPECT_EQ(base_url, reload_entry->GetBaseURLForDataURL()); > > EXPECT_EQ(history_url, reload_entry->GetVirtualURL()); > > EXPECT_EQ(history_url, reload_entry->GetHistoryURLForDataURL()); > > EXPECT_EQ(history_url, reload_entry->GetOriginalRequestURL()); > > EXPECT_EQ(data_url, reload_entry->GetURL()); > > } > > Thanks for the test :D
PTAL Fixed the new nav entry issue. Rebased and cleaned up everything a bit. On 2015/11/19 20:51:49, Charlie Reis wrote: > On 2015/11/19 20:42:14, boliu wrote: > > Thanks for the detailed reply! > > > > On 2015/11/19 20:08:36, Charlie Reis wrote: > > > I'm revisiting this one (patch set 10), since we both have some concerns > about > > > the new proposal in https://codereview.chromium.org/1449793002/. > > > > > > This one might actually be quite close to something we can land. I like > that > > > it's keeping the various URL values where they were, and that it makes the > > > renderer process report the data URL in params.url. No changes to > > serialization > > > will be required, and we can verify that the NavigationEntry fields stay > > correct > > > (as in the expanded version of the test below). > > > > > > My test shows there's still one main issue we need to solve-- a reload of > the > > > data URL is being classified as a new navigation, so it leads to some > problems > > > (it creates a new NavigationEntry, loses the various base_url_for_data_url_ > > > state, we can't reload again, etc.). I'm trying to track down why this is > the > > > case in a debugger. We're kind of doing the right thing in > > > RenderFrameImpl::NavigateInternal during the reload (i.e., calling > LoadDataURL > > > again), but Blink considers that a new navigation rather than a reload. > > There's > > > probably a way to get Blink to realize it's a reload. > > > > This might possibly be an independent blink issue. I'll look into it. > > Great, thanks. I'll take a break and write up a different fix in the meantime. Was pretty easy. WebFrame::LoadData takes a bool replace argument that was hardcoded to false. Setting it to true on reloads solved this. > > > > > > > > > One other question-- what's the reason that we need to change the original > > > request URL from the history URL to the data URL? I'm not sure I understand > > the > > > need for that change yet. > > > > That chunk was added in PS5. I tried to look back if this was required to not > > break tests, but all the bot links don't work anymore. So not sure if that's > > required or not. > > > > Conceptually, the original URL not matching GetURL, even though there were no > > redirects, feels weird to me. Also if original URL is not changed, then the > > exact same crash would happen for a RELOAD_ORIGINAL_REQUEST_URL. > > That's a fair point. If nothing else was depending on the history URL being > there, then feel free to update the test expectation to be the data URL rather > than the history URL (and include a comment in RenderFrameImpl about it). Previous cq runs were happy with the change. Here's hoping test coverage is great.. > > > > > > > > > > I would suggest using a version of the code below as the new test, since > it's > > > much more explicit about what we expect in each stage. It's helping me > track > > > down the bug I mentioned above. (We may also want to add a navigation to > > > another page and then test a back navigation, since that will have similar > > > concerns as reload.) > > > > > > // Verifies that the base, history, and data URLs for LoadDataWithBaseURL > end > > up > > > // in the expected parts of the NavigationEntry in each stage of navigation, > > and > > > // that we don't kill the renderer on reload. See https://crbug.com/522567. > > > IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, LoadDataWithBaseURL) > { > > > const GURL base_url("http://baseurl"); > > > const GURL history_url("http://historyurl"); > > > const std::string data = "<html><body>foo</body></html>"; > > > const GURL data_url = GURL("data:text/html;charset=utf-8," + data); > > > > > > const NavigationControllerImpl& controller = > > > static_cast<const NavigationControllerImpl&>( > > > shell()->web_contents()->GetController()); > > > > > > // Load data, but don't commit yet. > > > TestNavigationObserver same_tab_observer(shell()->web_contents(), 1); > > > shell()->LoadDataWithBaseURL(history_url, data, base_url); > > > > > > // Verify the pending NavigationEntry. > > > NavigationEntryImpl* pending_entry = controller.GetPendingEntry(); > > > EXPECT_EQ(base_url, pending_entry->GetBaseURLForDataURL()); > > > EXPECT_EQ(history_url, pending_entry->GetVirtualURL()); > > > EXPECT_EQ(history_url, pending_entry->GetHistoryURLForDataURL()); > > > EXPECT_EQ(data_url, pending_entry->GetURL()); > > > > > > // Let the navigation commit. > > > same_tab_observer.Wait(); > > > > > > // Verify the last committed NavigationEntry. > > > NavigationEntryImpl* entry = controller.GetLastCommittedEntry(); > > > EXPECT_EQ(base_url, entry->GetBaseURLForDataURL()); > > > EXPECT_EQ(history_url, entry->GetVirtualURL()); > > > EXPECT_EQ(history_url, entry->GetHistoryURLForDataURL()); > > > EXPECT_EQ(data_url, entry->GetURL()); > > > > > > // We should use history_url instead of the base_url as the original url > of > > > // this navigation entry, because base_url is only used for resolving > > relative > > > // paths in the data, or enforcing same origin policy. > > > EXPECT_EQ(history_url, entry->GetOriginalRequestURL()); > > > > > > // Now reload and make sure the renderer isn't killed. > > > ReloadBlockUntilNavigationsComplete(shell(), 1); > > > EXPECT_TRUE(shell()->web_contents()->GetMainFrame()->IsRenderFrameLive()); > > > > > > // Verify the last committed NavigationEntry hasn't changed. > > > NavigationEntryImpl* reload_entry = controller.GetLastCommittedEntry(); > > > EXPECT_EQ(entry, reload_entry); > > > EXPECT_EQ(base_url, reload_entry->GetBaseURLForDataURL()); > > > EXPECT_EQ(history_url, reload_entry->GetVirtualURL()); > > > EXPECT_EQ(history_url, reload_entry->GetHistoryURLForDataURL()); > > > EXPECT_EQ(history_url, reload_entry->GetOriginalRequestURL()); > > > EXPECT_EQ(data_url, reload_entry->GetURL()); > > > } > > > > Thanks for the test :D
Description was changed from ========== Browser test for LoadDataWithBaseURL reload BUG=522567 ========== to ========== Fix LoadDataWithBaseURL reload On a LoadDataWithBaseURL, renderer reports the history URL as the URL of the navigation, and is stored in the NavigationEntry::GetURL field, overwriting the data URL originally stored there. This causes a crash on a reload since the data URL has been lost. Fix this by saving the data URL DocumentState so that renderer can return it as the URL of the load. This means that GetURL and GetOriginalURL for a LoadDataWithBaseURL will now return the data URL, not the history URL. Also fix WebFrame::LoadData to not create new navigation for a reload. BUG=522567 ==========
Very glad to see it passing the new test! I'm wondering if we can get by without the DocumentState changes. Is the data URL stored in the NavigationState's CommonNavigationParams or in ds->originalRequest().url()? Some related questions and various nits below. https://codereview.chromium.org/1304373007/diff/220001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1304373007/diff/220001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:496: NavigationEntryImpl* entry = controller_->GetLastCommittedEntry(); I'm a little concerned about this particular change because other similar APIs (like DidNavigateMainFramePostCommit below) also expose the data URL (via params). This causes us to have inconsistent behavior across APIs. Is there any way that we could update the delegates instead, if they were expecting the virtual URL? (At the least, we'll need a comment indicating why this is necessary, but it would be good to avoid this block if we can.) https://codereview.chromium.org/1304373007/diff/220001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:497: bool use_virtual_url = entry->GetVirtualURL().is_valid() && This condition is kind of a no-op, because GetVirtualURL() will return GetURL() if there is no virtual URL. It would be hard to get back an invalid URL here. https://codereview.chromium.org/1304373007/diff/220001/content/public/rendere... File content/public/renderer/document_state.h (right): https://codereview.chromium.org/1304373007/diff/220001/content/public/rendere... content/public/renderer/document_state.h:185: void set_was_load_data_url_with_base_url_request(bool value) { nit: s/data_url/data/ (Since the API is LoadDataWithBaseURL.) https://codereview.chromium.org/1304373007/diff/220001/content/public/rendere... content/public/renderer/document_state.h:188: bool was_load_data_url_with_base_url_request() const { Same. https://codereview.chromium.org/1304373007/diff/220001/content/public/rendere... content/public/renderer/document_state.h:232: bool was_load_data_url_with_base_url_request_; Same. Also, these seem like they belong on NavigationState instead of DocumentState. And that already has CommonParams with url, base_url_for_data_url, and history_url_for_data_url, so maybe we don't need to store anything extra after all? https://codereview.chromium.org/1304373007/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1304373007/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:306: // Return URLs that should override the default getter for this data source. nit: Please rephrase. This method returns a bool, not a URL. https://codereview.chromium.org/1304373007/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:312: // URL of the data source. nit: ..., not the history URL. Sanity check: I know the history URL is stored in unreachableURL(), but what's in ds->originalRequest().url()? I thought that was the data URL at this point, which means we wouldn't need to separately store it in DocumentState. Maybe I'm wrong? https://codereview.chromium.org/1304373007/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:2634: // Carry over the user agent override flag, if it exists. nit: Update comment. https://codereview.chromium.org/1304373007/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:5114: // Needed to not identify only changing history url as reload. nit: Rephrase. Needed so that history-url-only changes don't become reloads. https://codereview.chromium.org/1304373007/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:5182: if (MaybeGetURLOverride(ds, &overriden_url)) { nit: No braces. https://codereview.chromium.org/1304373007/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:5258: document_state->set_data_url(common_params.url); We should only do this if |load_data| is true. It will be confusing for non-load-data cases to have a non-data URL in data_url_.
Please don't review new patch sets yet. But a comment about getting the data URL from common params instead of DocumentState. It doesn't work when there is an in-page navigation on the data url. In that case, didNavigateWithinPage calls UpdateNavigationState throws away the existing NavigationStateImpl. Suggestions? Webview actually has a test that's testing exactly this. Took awhile to figure out why it was failing..
Other than the DocumentState vs NavigationState comment, PS15 should have addressed everything else. PTAL https://codereview.chromium.org/1304373007/diff/220001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1304373007/diff/220001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:496: NavigationEntryImpl* entry = controller_->GetLastCommittedEntry(); On 2015/11/20 21:30:12, Charlie Reis wrote: > I'm a little concerned about this particular change because other similar APIs > (like DidNavigateMainFramePostCommit below) also expose the data URL (via > params). This causes us to have inconsistent behavior across APIs. > > Is there any way that we could update the delegates instead, if they were > expecting the virtual URL? > > (At the least, we'll need a comment indicating why this is necessary, but it > would be good to avoid this block if we can.) Moved this change to extensions/browser/guest_view/web_view/web_view_guest.cc https://codereview.chromium.org/1304373007/diff/220001/content/public/rendere... File content/public/renderer/document_state.h (right): https://codereview.chromium.org/1304373007/diff/220001/content/public/rendere... content/public/renderer/document_state.h:185: void set_was_load_data_url_with_base_url_request(bool value) { On 2015/11/20 21:30:12, Charlie Reis wrote: > nit: s/data_url/data/ > > (Since the API is LoadDataWithBaseURL.) Done. https://codereview.chromium.org/1304373007/diff/220001/content/public/rendere... content/public/renderer/document_state.h:188: bool was_load_data_url_with_base_url_request() const { On 2015/11/20 21:30:12, Charlie Reis wrote: > Same. Done. https://codereview.chromium.org/1304373007/diff/220001/content/public/rendere... content/public/renderer/document_state.h:232: bool was_load_data_url_with_base_url_request_; On 2015/11/20 21:30:12, Charlie Reis wrote: > Same. > > Also, these seem like they belong on NavigationState instead of DocumentState. > And that already has CommonParams with url, base_url_for_data_url, and > history_url_for_data_url, so maybe we don't need to store anything extra after > all? Did not make this change. As noted, this breaks in-page navigations, where NavigationState is completely recreated from scratch. https://codereview.chromium.org/1304373007/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1304373007/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:306: // Return URLs that should override the default getter for this data source. On 2015/11/20 21:30:12, Charlie Reis wrote: > nit: Please rephrase. This method returns a bool, not a URL. Done. https://codereview.chromium.org/1304373007/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:312: // URL of the data source. On 2015/11/20 21:30:12, Charlie Reis wrote: > nit: ..., not the history URL. Done > > Sanity check: I know the history URL is stored in unreachableURL(), but what's > in ds->originalRequest().url()? I thought that was the data URL at this point, > which means we wouldn't need to separately store it in DocumentState. Maybe I'm > wrong? It's the base URL. Blink never sees the data part as an URL. The data URL is parsed into a buffer in LoadDataURL and blink only ever sees that buffer. https://codereview.chromium.org/1304373007/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:2634: // Carry over the user agent override flag, if it exists. On 2015/11/20 21:30:12, Charlie Reis wrote: > nit: Update comment. Done https://codereview.chromium.org/1304373007/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:5114: // Needed to not identify only changing history url as reload. On 2015/11/20 21:30:12, Charlie Reis wrote: > nit: Rephrase. > > Needed so that history-url-only changes don't become reloads. Done. https://codereview.chromium.org/1304373007/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:5182: if (MaybeGetURLOverride(ds, &overriden_url)) { On 2015/11/20 21:30:12, Charlie Reis wrote: > nit: No braces. Done. https://codereview.chromium.org/1304373007/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:5258: document_state->set_data_url(common_params.url); On 2015/11/20 21:30:12, Charlie Reis wrote: > We should only do this if |load_data| is true. It will be confusing for > non-load-data cases to have a non-data URL in data_url_. Done
This is great. I almost approved but noticed one possible bug in didCreateDataSource. Since that seems to be related to the in-page navigation discussion as well, would you be willing to add a second NavigationControllerBrowserTest covering LoadDataWithBaseURL + in-page navigation (plus a script navigation to a non-data URL)? I think it'll be very helpful to have that test in content. Once we get that case nailed down, I'm very happy with this. A few other nits below. On 2015/11/23 16:15:59, boliu wrote: > Please don't review new patch sets yet. But a comment about getting the data URL > from common params instead of DocumentState. It doesn't work when there is an > in-page navigation on the data url. In that case, didNavigateWithinPage calls > UpdateNavigationState throws away the existing NavigationStateImpl. Suggestions? > > Webview actually has a test that's testing exactly this. Took awhile to figure > out why it was failing.. Nice. On the one hand, I can see the rationale of clearing the NavigationState for in-page navigations and keeping around the DocumentState, so I could be ok with this approach. On the other hand, I noticed a possible bug below in didCreateDataSource that might be related. See discussion there. https://codereview.chromium.org/1304373007/diff/220001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1304373007/diff/220001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:496: NavigationEntryImpl* entry = controller_->GetLastCommittedEntry(); On 2015/11/23 22:32:25, boliu wrote: > On 2015/11/20 21:30:12, Charlie Reis wrote: > > I'm a little concerned about this particular change because other similar APIs > > (like DidNavigateMainFramePostCommit below) also expose the data URL (via > > params). This causes us to have inconsistent behavior across APIs. > > > > Is there any way that we could update the delegates instead, if they were > > expecting the virtual URL? > > > > (At the least, we'll need a comment indicating why this is necessary, but it > > would be good to avoid this block if we can.) > > Moved this change to extensions/browser/guest_view/web_view/web_view_guest.cc Much better-- thanks! https://codereview.chromium.org/1304373007/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1304373007/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:312: // URL of the data source. On 2015/11/23 22:32:25, boliu wrote: > On 2015/11/20 21:30:12, Charlie Reis wrote: > > nit: ..., not the history URL. > > Done > > > > > Sanity check: I know the history URL is stored in unreachableURL(), but what's > > in ds->originalRequest().url()? I thought that was the data URL at this > point, > > which means we wouldn't need to separately store it in DocumentState. Maybe > I'm > > wrong? > > It's the base URL. Blink never sees the data part as an URL. The data URL is > parsed into a buffer in LoadDataURL and blink only ever sees that buffer. Acknowledged. https://codereview.chromium.org/1304373007/diff/280001/content/public/rendere... File content/public/renderer/document_state.h (right): https://codereview.chromium.org/1304373007/diff/280001/content/public/rendere... content/public/renderer/document_state.h:185: void set_was_load_data_with_base_url_request(bool value) { nit: Comment on this block mentioning LoadDataWithBaseURL (making it easier to search for) and that data_url() is only valid if was_load_data_with_base_url_request() is true. https://codereview.chromium.org/1304373007/diff/280001/content/public/rendere... content/public/renderer/document_state.h:232: bool was_load_data_with_base_url_request_; nit: Place these two in their own block with a comment. (They're not related to prefetcher.) https://codereview.chromium.org/1304373007/diff/280001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1304373007/diff/280001/content/renderer/rende... content/renderer/render_frame_impl.cc:310: // Gets URL that should override the default getter for this data source. nit: Add: "(if any), storing it in |output|." https://codereview.chromium.org/1304373007/diff/280001/content/renderer/rende... content/renderer/render_frame_impl.cc:311: // Return true if there is an override URL. nit: Returns (to be consistent with "Gets") https://codereview.chromium.org/1304373007/diff/280001/content/renderer/rende... content/renderer/render_frame_impl.cc:312: bool MaybeGetURLOverride(WebDataSource* ds, GURL* output) { nit: MaybeGetOverriddenURL https://codereview.chromium.org/1304373007/diff/280001/content/renderer/rende... content/renderer/render_frame_impl.cc:2657: document_state->set_was_load_data_with_base_url_request( Looking closer, I'm skeptical about this call. Won't this apply to link clicks to different (non-data) pages? It makes sense to carry over the user agent override in that case, but we would no longer be on a data URL. What's the case this was needed for? We should set it correctly for browser-initiated navigations in UpdateNavigationState (given your change below). Is this for the in-page renderer-initiated case? If so, maybe we should copy the URL over in didNavigateWithinPage instead? (That might also clear the way for using NavigateState instead of DocumentState?) https://codereview.chromium.org/1304373007/diff/280001/extensions/browser/gue... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/1304373007/diff/280001/extensions/browser/gue... extensions/browser/guest_view/web_view/web_view_guest.cc:792: src_ = web_contents()->GetURL(); nit: Add comment (so that this doesn't regress in a future cleanup CL).
Added new test but not all checks pass. So turns out in-page navigation is semi-broken for LoadDataWithBaseURL as well. Out of scope for this CL though IMO. I glossed over your comment about NavigateState and saving data URL in NavigateState yesterday. I can explore that a bit today. Also meta: Not sure what your vacation plans are, but today is my last day at work before a 1.5-long vacation. If our schedules don't line up again, this might very well get pushed out to 2016 :( https://codereview.chromium.org/1304373007/diff/280001/content/public/rendere... File content/public/renderer/document_state.h (right): https://codereview.chromium.org/1304373007/diff/280001/content/public/rendere... content/public/renderer/document_state.h:185: void set_was_load_data_with_base_url_request(bool value) { On 2015/11/24 00:22:59, Charlie Reis wrote: > nit: Comment on this block mentioning LoadDataWithBaseURL (making it easier to > search for) and that data_url() is only valid if > was_load_data_with_base_url_request() is true. Done. https://codereview.chromium.org/1304373007/diff/280001/content/public/rendere... content/public/renderer/document_state.h:232: bool was_load_data_with_base_url_request_; On 2015/11/24 00:22:59, Charlie Reis wrote: > nit: Place these two in their own block with a comment. (They're not related to > prefetcher.) Done. https://codereview.chromium.org/1304373007/diff/280001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1304373007/diff/280001/content/renderer/rende... content/renderer/render_frame_impl.cc:310: // Gets URL that should override the default getter for this data source. On 2015/11/24 00:22:59, Charlie Reis wrote: > nit: Add: "(if any), storing it in |output|." Done. https://codereview.chromium.org/1304373007/diff/280001/content/renderer/rende... content/renderer/render_frame_impl.cc:311: // Return true if there is an override URL. On 2015/11/24 00:22:59, Charlie Reis wrote: > nit: Returns > (to be consistent with "Gets") Done. https://codereview.chromium.org/1304373007/diff/280001/content/renderer/rende... content/renderer/render_frame_impl.cc:312: bool MaybeGetURLOverride(WebDataSource* ds, GURL* output) { On 2015/11/24 00:22:59, Charlie Reis wrote: > nit: MaybeGetOverriddenURL Done. https://codereview.chromium.org/1304373007/diff/280001/content/renderer/rende... content/renderer/render_frame_impl.cc:2657: document_state->set_was_load_data_with_base_url_request( On 2015/11/24 00:22:59, Charlie Reis wrote: > Looking closer, I'm skeptical about this call. Won't this apply to link clicks > to different (non-data) pages? It makes sense to carry over the user agent > override in that case, but we would no longer be on a data URL. > > What's the case this was needed for? We should set it correctly for > browser-initiated navigations in UpdateNavigationState (given your change > below). Is this for the in-page renderer-initiated case? > > If so, maybe we should copy the URL over in didNavigateWithinPage instead? > (That might also clear the way for using NavigateState instead of > DocumentState?) Err, this is not needed afaict. in-page navigations don't create new DocumentState. I think I was copying originally copying the set_is_overriding_user_agent pattern, and didn't really know what I was doing :p https://codereview.chromium.org/1304373007/diff/280001/extensions/browser/gue... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/1304373007/diff/280001/extensions/browser/gue... extensions/browser/guest_view/web_view/web_view_guest.cc:792: src_ = web_contents()->GetURL(); On 2015/11/24 00:22:59, Charlie Reis wrote: > nit: Add comment (so that this doesn't regress in a future cleanup CL). Done.
I think I'm happy with where it is now, with the plan to fix in-page as a followup. Patch 16 LGTM with the nits below. (Thanks for working through all the issues here!) On 2015/11/24 19:03:25, boliu wrote: > Added new test but not all checks pass. So turns out in-page navigation is > semi-broken for LoadDataWithBaseURL as well. Out of scope for this CL though > IMO. Agreed about fixing in-page later. > I glossed over your comment about NavigateState and saving data URL in > NavigateState yesterday. I can explore that a bit today. Don't worry about it, now that the block in didCreateDataSource is gone. Maybe later we can move it from DocumentState to NavigationState, but I'm ok with it in DocumentState for now. > Also meta: Not sure what your vacation plans are, but today is my last day at > work before a 1.5-long vacation. If our schedules don't line up again, this > might very well get pushed out to 2016 :( I'll be around most of December (until the 18th), but let's try to get it landed before you go if possible. https://codereview.chromium.org/1304373007/diff/280001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1304373007/diff/280001/content/renderer/rende... content/renderer/render_frame_impl.cc:2657: document_state->set_was_load_data_with_base_url_request( On 2015/11/24 19:03:25, boliu wrote: > On 2015/11/24 00:22:59, Charlie Reis wrote: > > Looking closer, I'm skeptical about this call. Won't this apply to link > clicks > > to different (non-data) pages? It makes sense to carry over the user agent > > override in that case, but we would no longer be on a data URL. > > > > What's the case this was needed for? We should set it correctly for > > browser-initiated navigations in UpdateNavigationState (given your change > > below). Is this for the in-page renderer-initiated case? > > > > If so, maybe we should copy the URL over in didNavigateWithinPage instead? > > (That might also clear the way for using NavigateState instead of > > DocumentState?) > > Err, this is not needed afaict. in-page navigations don't create new > DocumentState. > > I think I was copying originally copying the set_is_overriding_user_agent > pattern, and didn't really know what I was doing :p Great, I'm glad it's not needed. https://codereview.chromium.org/1304373007/diff/300001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1304373007/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:128: " <p id=\"frag\"><a id=\"fraglink\" href=\"#frag\">in-page nav</a></p>" Let's leave the fragment out of the test for now. (See below.) I'm not actually sure if this URL is valid-- Chrome seems to treat it as ending at 'href="', with '#frag">in-page nav</a></p>' as the fragment. https://codereview.chromium.org/1304373007/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:136: // Load data and commit nit: End with period. https://codereview.chromium.org/1304373007/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:150: std::string script = "document.getElementById('fraglink').click()"; Let's leave this whole block out for now and just have a TODO to test in-page navigations. (I remembered that we're broken on data URLs + fragments anyway, per https://crbug.com/528454.) Can you file a bug for in-page + LoadDataWithBaseURL and include its number in the TODO? (Note, if you want to try one last thing before we give up, pushState might be an easier way to test in-page without dealing with the brokenness around fragments.) https://codereview.chromium.org/1304373007/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:165: embedded_test_server()->base_url().spec() + "';"; nit: Store this URL in a local variable that we can use in the expectations below. https://codereview.chromium.org/1304373007/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:171: EXPECT_EQ(GURL(), entry->GetBaseURLForDataURL()); nit: EXPECT_TRUE / .is_empty() https://codereview.chromium.org/1304373007/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:172: EXPECT_EQ(GURL(), entry->GetHistoryURLForDataURL()); nit: EXPECT_TRUE / .is_empty()
https://codereview.chromium.org/1304373007/diff/300001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1304373007/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:150: std::string script = "document.getElementById('fraglink').click()"; On 2015/11/24 19:32:38, Charlie Reis wrote: > Let's leave this whole block out for now and just have a TODO to test in-page > navigations. (I remembered that we're broken on data URLs + fragments anyway, > per https://crbug.com/528454.) > > Can you file a bug for in-page + LoadDataWithBaseURL and include its number in > the TODO? > > (Note, if you want to try one last thing before we give up, pushState might be > an easier way to test in-page without dealing with the brokenness around > fragments.) Actually, don't bother with pushState. pushState isn't allowed in data URLs.
boliu@chromium.org changed reviewers: + fsamuel@chromium.org
Thanks. All comments on test addressed. +fsamuel for web_view_guest.cc
web_view lgtm
The CQ bit was checked by boliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/1304373007/#ps320001 (title: "review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1304373007/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1304373007/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/15890e4b345bfef3854566899a0a462f7c5939a9 Cr-Commit-Position: refs/heads/master@{#361481} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
