PlzNavigate: Support data urls

PlzNavigate: Support data urls

This CL adds support for navigating to data urls in browser-side navigation mode. This is a prerequisite to showing error pages when browser-side navigation is enabled.

BUG=459033
Committed: https://crrev.com/4cc9b820607cfd1a4c41ee34050b81f4e24a99a4
Cr-Commit-Position: refs/heads/master@{#318686}

[clamy, 2015-02-16 16:45:34]

Patchset #2 (id:20001) has been deleted

[clamy, 2015-02-16 17:57:39]

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

[clamy, 2015-02-16 18:00:44]

@creis, carlosk: PTAL
@fdegans: FYI

This CL enables loading data urls with PlzNavigate enabled. This is a
prerequisite for showing error pages (since we get them as data urls).

https://codereview.chromium.org/906283003/diff/40001/content/common/navigatio...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/906283003/diff/40001/content/common/navigatio...
content/common/navigation_params.h:29: bool
ShouldMakeNetworkRequestForNavigation(const GURL& url);
This function is used both in the renderer and the browser, so needs to be in
content/common. I thought it may be too much to create some navigation_utils
file in content/common just for it so put it there. If you have a better
suggestion for where it should be located, I'm happy to here it.

[carlosk, 2015-02-17 12:35:09]

Looks good; just a few minor comments.

And the CR extension seems to be reporting that content/common/frame_messages.h
still needs a reviewer. Do you really need to add someone else beyond creis@?

https://codereview.chromium.org/906283003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/906283003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_request.cc:182:
frame_tree_node_->navigator()->CommitNavigation(
I find it weird that we're calling into Navigator from a class named
NavitationRequest (it sounds like a static thing) but as we're already doing so
in 2 different instances this doesn't change anything. It's maybe something we'd
want to figure out later...

https://codereview.chromium.org/906283003/diff/40001/content/common/navigatio...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/906283003/diff/40001/content/common/navigatio...
content/common/navigation_params.cc:15: return true;
Suggestion: return !url.SchemeIs(url::kDataScheme);

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

https://codereview.chromium.org/906283003/diff/40001/content/renderer/render_...
content/renderer/render_frame_impl.cc:3873:
common_params.url.SchemeIs(url::kDataScheme)) {
Shouldn't this be a call to ShouldMakeNetworkRequestForNavigation?

[clamy, 2015-02-17 12:54:29]

Thanks! Yes I will need a security review for frame_messages (from Nasko I guess
but I did not add him yet since he's ooo).

https://codereview.chromium.org/906283003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/906283003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_request.cc:182:
frame_tree_node_->navigator()->CommitNavigation(
On 2015/02/17 12:35:09, carlosk wrote:
> I find it weird that we're calling into Navigator from a class named
> NavitationRequest (it sounds like a static thing) but as we're already doing
so
> in 2 different instances this doesn't change anything. It's maybe something
we'd
> want to figure out later...

I am not quite sure what you mean there. There are several instances of
Navigator, so we need the right one to commit the navigation.

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

https://codereview.chromium.org/906283003/diff/40001/content/renderer/render_...
content/renderer/render_frame_impl.cc:3873:
common_params.url.SchemeIs(url::kDataScheme)) {
On 2015/02/17 12:35:09, carlosk wrote:
> Shouldn't this be a call to ShouldMakeNetworkRequestForNavigation?

The goal is for ShouldMakeNetworkRequestForNavigation to be extended to also
cover the same document navigation case (where we should not be making a network
request). Here we specifically only handle data urls.

[carlosk, 2015-02-17 14:11:23]

LGTM.

https://codereview.chromium.org/906283003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/906283003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_request.cc:182:
frame_tree_node_->navigator()->CommitNavigation(
On 2015/02/17 12:54:28, clamy wrote:
> On 2015/02/17 12:35:09, carlosk wrote:
> > I find it weird that we're calling into Navigator from a class named
> > NavitationRequest (it sounds like a static thing) but as we're already doing
> so
> > in 2 different instances this doesn't change anything. It's maybe something
> we'd
> > want to figure out later...
> 
> I am not quite sure what you mean there. There are several instances of
> Navigator, so we need the right one to commit the navigation.

Yes, one navigator per node. I's just an architectural concern that maybe a new
delegate would solve.

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

https://codereview.chromium.org/906283003/diff/40001/content/renderer/render_...
content/renderer/render_frame_impl.cc:3873:
common_params.url.SchemeIs(url::kDataScheme)) {
On 2015/02/17 12:54:28, clamy wrote:
> On 2015/02/17 12:35:09, carlosk wrote:
> > Shouldn't this be a call to ShouldMakeNetworkRequestForNavigation?
> 
> The goal is for ShouldMakeNetworkRequestForNavigation to be extended to also
> cover the same document navigation case (where we should not be making a
network
> request). Here we specifically only handle data urls.

Acknowledged.

[Charlie Reis, 2015-02-18 00:43:25]

[+lfg to CC, since he worked on cross-process data URL navigations]

It's not clear to me that we can skip the browser process for data URLs. 
Consider a page on site A with two iframes (sites B and C respectively).  If the
B iframe tells the C iframe to go to a data URL, then we want it to end up in
B's process, since B has control over the data URL's contents.

This is managed using source_site_instance and we track it using the browser
process's cross-process navigation logic.  See https://crbug.com/433014.

https://codereview.chromium.org/906283003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/906283003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_request.cc:89:
entry.GetBaseURLForDataURL().is_empty()? GURL() : entry.GetVirtualURL();
nit: Space before ?

https://codereview.chromium.org/906283003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_request.cc:180: // the renderer should
already be trying to commit it.
I'm not sure I agree with the second sentence.  Data URLs don't commit
synchronously, and it's not clear to me whether the renderer process should
directly commit them.  Why would they not go through the browser process?

https://codereview.chromium.org/906283003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_request.cc:182:
frame_tree_node_->navigator()->CommitNavigation(
On 2015/02/17 14:11:23, carlosk wrote:
> On 2015/02/17 12:54:28, clamy wrote:
> > On 2015/02/17 12:35:09, carlosk wrote:
> > > I find it weird that we're calling into Navigator from a class named
> > > NavitationRequest (it sounds like a static thing) but as we're already
doing
> > so
> > > in 2 different instances this doesn't change anything. It's maybe
something
> > we'd
> > > want to figure out later...
> > 
> > I am not quite sure what you mean there. There are several instances of
> > Navigator, so we need the right one to commit the navigation.
> 
> Yes, one navigator per node. I's just an architectural concern that maybe a
new
> delegate would solve.

To be clear, there is not one Navigator per FrameTreeNode.  In most cases, all
FrameTreeNodes of a WebContents share the same Navigator.  We made it possible
for a FTN to have a different Navigator to support things like the <webview> tag
in apps.

https://codereview.chromium.org/906283003/diff/40001/content/common/navigatio...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/906283003/diff/40001/content/common/navigatio...
content/common/navigation_params.h:29: bool
ShouldMakeNetworkRequestForNavigation(const GURL& url);
On 2015/02/16 18:00:44, clamy wrote:
> This function is used both in the renderer and the browser, so needs to be in
> content/common. I thought it may be too much to create some navigation_utils
> file in content/common just for it so put it there. If you have a better
> suggestion for where it should be located, I'm happy to here it.

I think the premise on this one might be a bit flawed.  There's lots of
navigations that don't go to the network (e.g., fragments, pushState, file://,
chrome://, javascript:, etc).  Exposing this as a global helper function in
content/ is likely to lead to confusion about what it means.

Is there a way we can avoid it?

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

https://codereview.chromium.org/906283003/diff/40001/content/renderer/render_...
content/renderer/render_frame_impl.cc:4130: return
blink::WebNavigationPolicyIgnore;
What if we're navigating a RemoteFrame and it needs to be pulled back in
process?  I'm not sure we can skip the browser process on all data URL
navigations.

[carlosk, 2015-02-18 15:45:08]

https://codereview.chromium.org/906283003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/906283003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_request.cc:182:
frame_tree_node_->navigator()->CommitNavigation(
On 2015/02/18 00:43:25, Charlie Reis wrote:
> On 2015/02/17 14:11:23, carlosk wrote:
> > On 2015/02/17 12:54:28, clamy wrote:
> > > On 2015/02/17 12:35:09, carlosk wrote:
> > > > I find it weird that we're calling into Navigator from a class named
> > > > NavitationRequest (it sounds like a static thing) but as we're already
> doing
> > > so
> > > > in 2 different instances this doesn't change anything. It's maybe
> something
> > > we'd
> > > > want to figure out later...
> > > 
> > > I am not quite sure what you mean there. There are several instances of
> > > Navigator, so we need the right one to commit the navigation.
> > 
> > Yes, one navigator per node. I's just an architectural concern that maybe a
> new
> > delegate would solve.
> 
> To be clear, there is not one Navigator per FrameTreeNode.  In most cases, all
> FrameTreeNodes of a WebContents share the same Navigator.  We made it possible
> for a FTN to have a different Navigator to support things like the <webview>
tag
> in apps.

Acknowledged.

[clamy, 2015-02-20 13:39:59]

New patchsets have been uploaded after l-g-t-m from carlosk@chromium.org

[clamy, 2015-02-20 17:05:59]

Thanks! I uploaded a new patchset where we do go to the browser for data urls.
Note that this require a blink side patch for the data url to be properly loaded
(https://codereview.chromium.org/927653005/).

https://codereview.chromium.org/906283003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/906283003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_request.cc:89:
entry.GetBaseURLForDataURL().is_empty()? GURL() : entry.GetVirtualURL();
On 2015/02/18 00:43:25, Charlie Reis wrote:
> nit: Space before ?

Done.

https://codereview.chromium.org/906283003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_request.cc:180: // the renderer should
already be trying to commit it.
On 2015/02/18 00:43:25, Charlie Reis wrote:
> I'm not sure I agree with the second sentence.  Data URLs don't commit
> synchronously, and it's not clear to me whether the renderer process should
> directly commit them.  Why would they not go through the browser process?

Changed the behavior so that now we always go through the commit step. I guess
the only renderer-initiated navigations that the renderer should pursue by
itself are the same-document ones.

https://codereview.chromium.org/906283003/diff/40001/content/common/navigatio...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/906283003/diff/40001/content/common/navigatio...
content/common/navigation_params.cc:15: return true;
On 2015/02/17 12:35:09, carlosk wrote:
> Suggestion: return !url.SchemeIs(url::kDataScheme);

Done.

https://codereview.chromium.org/906283003/diff/40001/content/common/navigatio...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/906283003/diff/40001/content/common/navigatio...
content/common/navigation_params.h:29: bool
ShouldMakeNetworkRequestForNavigation(const GURL& url);
On 2015/02/18 00:43:25, Charlie Reis wrote:
> On 2015/02/16 18:00:44, clamy wrote:
> > This function is used both in the renderer and the browser, so needs to be
in
> > content/common. I thought it may be too much to create some navigation_utils
> > file in content/common just for it so put it there. If you have a better
> > suggestion for where it should be located, I'm happy to here it.
> 
> I think the premise on this one might be a bit flawed.  There's lots of
> navigations that don't go to the network (e.g., fragments, pushState, file://,
> chrome://, javascript:, etc).  Exposing this as a global helper function in
> content/ is likely to lead to confusion about what it means.
> 
> Is there a way we can avoid it?

I moved it to NavigationRequest where the context should make sense.

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

https://codereview.chromium.org/906283003/diff/40001/content/renderer/render_...
content/renderer/render_frame_impl.cc:4130: return
blink::WebNavigationPolicyIgnore;
On 2015/02/18 00:43:25, Charlie Reis wrote:
> What if we're navigating a RemoteFrame and it needs to be pulled back in
> process?  I'm not sure we can skip the browser process on all data URL
> navigations.

Done.

[Charlie Reis, 2015-02-20 22:11:50]

Thanks, that looks better to me.  Just a few more comments.

https://codereview.chromium.org/906283003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/906283003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigation_request.cc:55: // TODO(clamy): same
document navigations should nto make netwrok requests.
nit: not
nit: network

https://codereview.chromium.org/906283003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/906283003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:95:
entry.GetBaseURLForDataURL().is_empty()? GURL() : entry.GetVirtualURL();
nit: Wrong indent.

Also, maybe it makes sense to have a
NavigationEntryImpl::GetHistoryURLForDataURL() if we're going to have this code
in multiple places?  Seems like it would be easy for one of the copies to get
stale.

https://codereview.chromium.org/906283003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/906283003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl_unittest.cc:944: // PlzNavigate:
Verify that data urls are properly handled.
Can you add a second test for base_url_for_data_url?

https://codereview.chromium.org/906283003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl_unittest.cc:968:
ASSERT_TRUE(speculative_rfh);
Why is there a speculative RFH?  NavigatorImpl::BeginNavigation doesn't call
RFHM::BeginNavigation for data URLs.

https://codereview.chromium.org/906283003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl_unittest.cc:975: // Do a
renderer-initiated navigation to a data url. The request should be
should not?

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

https://codereview.chromium.org/906283003/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:4377: const GURL base_url =
params.base_url_for_data_url.is_empty() ?
We didn't have this case before (i.e., passing a data: URL without having
base_url_for_data_url), so I'm not sure whether the loadData method expects
params.url or an empty URL.  I assume you've verified this is the right thing to
pass?

[carlosk, 2015-02-23 10:50:29]

https://codereview.chromium.org/906283003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/906283003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl_unittest.cc:968:
ASSERT_TRUE(speculative_rfh);
On 2015/02/20 22:11:50, Charlie Reis wrote:
> Why is there a speculative RFH?  NavigatorImpl::BeginNavigation doesn't call
> RFHM::BeginNavigation for data URLs.

Line 960 causes it to be called.

[clamy, 2015-02-26 15:28:35]

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

[clamy, 2015-02-26 15:28:36]

Thanks! The latest patch set should have addressed most comments, except the
test one as I'd like to precise what needs to be tested there.

+nasko for frame_messages review.

https://codereview.chromium.org/906283003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/906283003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigation_request.cc:55: // TODO(clamy): same
document navigations should nto make netwrok requests.
On 2015/02/20 22:11:50, Charlie Reis wrote:
> nit: not
> nit: network

Done.

https://codereview.chromium.org/906283003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/906283003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:95:
entry.GetBaseURLForDataURL().is_empty()? GURL() : entry.GetVirtualURL();
On 2015/02/20 22:11:50, Charlie Reis wrote:
> nit: Wrong indent.
> 
> Also, maybe it makes sense to have a
> NavigationEntryImpl::GetHistoryURLForDataURL() if we're going to have this
code
> in multiple places?  Seems like it would be easy for one of the copies to get
> stale.

Done.

https://codereview.chromium.org/906283003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/906283003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl_unittest.cc:944: // PlzNavigate:
Verify that data urls are properly handled.
On 2015/02/20 22:11:50, Charlie Reis wrote:
> Can you add a second test for base_url_for_data_url?

What exactly would you want to test in that test? That we properly have the
base_url_for_data_url and support receiving the DidCommitProvisionalLoad IPC for
the base_url_for_data_url? Speaking of which, should the url used to select a
renderer to display the request be the data url itself or the
base_url_for_data_url?

https://codereview.chromium.org/906283003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl_unittest.cc:968:
ASSERT_TRUE(speculative_rfh);
On 2015/02/23 10:50:28, carlosk wrote:
> On 2015/02/20 22:11:50, Charlie Reis wrote:
> > Why is there a speculative RFH?  NavigatorImpl::BeginNavigation doesn't call
> > RFHM::BeginNavigation for data URLs.
> 
> Line 960 causes it to be called.

No, in that case NavigationRequest::BeginNavigation will call
NavigatorImpl::CommitNavigation, which in turn calls
RFHM::GetFrameHostForNavigation which creates it.

https://codereview.chromium.org/906283003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl_unittest.cc:975: // Do a
renderer-initiated navigation to a data url. The request should be
On 2015/02/20 22:11:50, Charlie Reis wrote:
> should not?

Done.

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

https://codereview.chromium.org/906283003/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:4377: const GURL base_url =
params.base_url_for_data_url.is_empty() ?
On 2015/02/20 22:11:50, Charlie Reis wrote:
> We didn't have this case before (i.e., passing a data: URL without having
> base_url_for_data_url), so I'm not sure whether the loadData method expects
> params.url or an empty URL.  I assume you've verified this is the right thing
to
> pass?

We cannot pass an empty URL here as this will cause the subsequent
ResourceRequest in Blink to have an empty url (and fail checks). Since it seems
that the base url is used for security purposes when the data mime type is html,
I added a CHECK for that.

[nasko, 2015-02-26 17:35:24]

The IPC changes look good. I've added a few comments of mine and I'll stamp the
CL once the rest of it is good to go.

https://codereview.chromium.org/906283003/diff/120001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/906283003/diff/120001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:355: return
GetBaseURLForDataURL().is_empty()? GURL() : GetVirtualURL();
nit: space still needed before "?"

https://codereview.chromium.org/906283003/diff/120001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/906283003/diff/120001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:181: state_ = RESPONSE_STARTED;
nit: I'd move this state assignment after the comment, as it applies equally
well to it.

https://codereview.chromium.org/906283003/diff/120001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/906283003/diff/120001/content/browser/frame_h...
content/browser/frame_host/navigator_impl_unittest.cc:947: const GURL
kUrl2("data:test");
nit: this isn't a valid data URL. It needs a comma that separates the mime type
from the content.

https://codereview.chromium.org/906283003/diff/120001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/906283003/diff/120001/content/common/navigati...
content/common/navigation_params.h:72: // Base URL for use in blink's
SubstituteData.
nit: s/blink/Blink/? You've used the uppercased version in other parts of this
CL.

[Charlie Reis, 2015-02-26 20:49:01]

Thanks Nasko.  A few questions below, and it looks like there's a compile error
in navigator_impl_unittest.cc.

https://codereview.chromium.org/906283003/diff/120001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/906283003/diff/120001/content/browser/frame_h...
content/browser/frame_host/navigator_impl_unittest.cc:947: const GURL
kUrl2("data:test");
On 2015/02/26 17:35:24, nasko wrote:
> nit: this isn't a valid data URL. It needs a comma that separates the mime
type
> from the content.

Yes, it seems like it should be data:text/html,test.  Is this related to the
mime type changes in render_frame_impl.cc?

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

https://codereview.chromium.org/906283003/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:3880:
common_params.url.SchemeIs(url::kDataScheme)) {
This case passes data URLs without base_url_data_urls to LoadDataURL, which
would fail the new CHECK.

https://codereview.chromium.org/906283003/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:4386:
!params.base_url_for_data_url.is_empty());
I don't understand why this would be true.  It's possible to navigate to data
URLs without specifying base_url_for_data_url.

If this function doesn't handle that case, can you elaborate in the comment
above to say where data URLs without base URLs are handled?

[clamy, 2015-02-27 12:54:00]

Thanks!

https://codereview.chromium.org/906283003/diff/120001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/906283003/diff/120001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:355: return
GetBaseURLForDataURL().is_empty()? GURL() : GetVirtualURL();
On 2015/02/26 17:35:24, nasko wrote:
> nit: space still needed before "?"

Done.

https://codereview.chromium.org/906283003/diff/120001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/906283003/diff/120001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:181: state_ = RESPONSE_STARTED;
On 2015/02/26 17:35:24, nasko wrote:
> nit: I'd move this state assignment after the comment, as it applies equally
> well to it.

Done.

https://codereview.chromium.org/906283003/diff/120001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/906283003/diff/120001/content/browser/frame_h...
content/browser/frame_host/navigator_impl_unittest.cc:947: const GURL
kUrl2("data:test");
On 2015/02/26 20:49:01, Charlie Reis wrote:
> On 2015/02/26 17:35:24, nasko wrote:
> > nit: this isn't a valid data URL. It needs a comma that separates the mime
> type
> > from the content.
> 
> Yes, it seems like it should be data:text/html,test.  Is this related to the
> mime type changes in render_frame_impl.cc?

Done. No it was there from the start.

https://codereview.chromium.org/906283003/diff/120001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/906283003/diff/120001/content/common/navigati...
content/common/navigation_params.h:72: // Base URL for use in blink's
SubstituteData.
On 2015/02/26 17:35:24, nasko wrote:
> nit: s/blink/Blink/? You've used the uppercased version in other parts of this
> CL.

Done.

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

https://codereview.chromium.org/906283003/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:3880:
common_params.url.SchemeIs(url::kDataScheme)) {
On 2015/02/26 20:49:01, Charlie Reis wrote:
> This case passes data URLs without base_url_data_urls to LoadDataURL, which
> would fail the new CHECK.

I removed the CHECK.

https://codereview.chromium.org/906283003/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:4386:
!params.base_url_for_data_url.is_empty());
On 2015/02/26 20:49:01, Charlie Reis wrote:
> I don't understand why this would be true.  It's possible to navigate to data
> URLs without specifying base_url_for_data_url.
> 
> If this function doesn't handle that case, can you elaborate in the comment
> above to say where data URLs without base URLs are handled?

I removed the CHECK. It was introduced because the current logic only calls
LoadDataURL when there is a base_url_for_data_url, and the last one may be
important for html data. Note that trying to do a browser-initiated navigation
to a data url currently does not seem to work in content shell.

[Charlie Reis, 2015-02-27 23:46:56]

Thanks, LGTM.

[nasko, 2015-02-28 00:15:21]

LGTM

[clamy, 2015-03-02 09:20:28]

The CQ bit was checked by clamy@chromium.org

[clamy, 2015-03-02 09:20:29]

The patchset sent to the CQ was uploaded after l-g-t-m from carlosk@chromium.org
Link to the patchset: https://codereview.chromium.org/906283003/#ps160001
(title: "Addressed comments")

[commit-bot: I haz the power, 2015-03-02 09:20:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/906283003/160001

[commit-bot: I haz the power, 2015-03-02 11:04:47]

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

[commit-bot: I haz the power, 2015-03-02 11:04:48]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[clamy, 2015-03-02 12:57:58]

The CQ bit was checked by clamy@chromium.org

[clamy, 2015-03-02 12:57:59]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
nasko@chromium.org, carlosk@chromium.org
Link to the patchset: https://codereview.chromium.org/906283003/#ps180001
(title: "Rebase + change to unit test")

[commit-bot: I haz the power, 2015-03-02 12:58:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/906283003/180001

[commit-bot: I haz the power, 2015-03-02 13:51:45]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2015-03-02 13:52:32]

Patchset 9 (id:??) landed as
https://crrev.com/4cc9b820607cfd1a4c41ee34050b81f4e24a99a4
Cr-Commit-Position: refs/heads/master@{#318686}