Set the original url correctly if the frame is loaded via loadData

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 209b20bd42ea3ed1cf75a673d3d1c1ef659e9f85..9680817bca827e9b6571da58717822efb7689bd5 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -135,7 +135,10 @@ WebURLResponseExtraDataImpl* GetExtraDataFromResponse(
       response.extraData());
 }
 
-void GetRedirectChain(WebDataSource* ds, std::vector<GURL>* result) {
+void GetRedirectChain(const WebDataSource* ds, std::vector<GURL>* result) {

[Charlie Reis, 2014/03/03 18:21:11]

Please don't change this signature.  (Why make the pointer const?)

[hush (inactive), 2014/03/03 23:12:52]

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Use_of...
I want to make WebDataSource* const because GetRedirectChain is only supposed to
read from WebDataSource and not change its states. Making it const ensures that
we don't accidentally change WebDataSource in later code. And the readers of the
code can easily reason about what the function does by looking at the signature,
without worrying the state of WebDataSource being changed.

However I'm new to the code base and if this is inconsistent with current
chromium code styles I will remove the "const".

[Charlie Reis, 2014/03/04 20:32:03]

Yes, we generally don't put const on pointer parameters.

[hush (inactive), 2014/03/04 23:22:33]

Done.

+  if (!result || !ds) {

[Charlie Reis, 2014/03/03 18:21:11]

style nit: No braces for a one-line body.

In what circumstances do we expect these to be null?  I'm not sure this is
needed.

[hush (inactive), 2014/03/03 23:12:52]

I was just doing this defensively in case that code in the future uses the
function incorrectly to introduce a null pointer exception. There won't be any
null pointers for the code right now.
I will remove the null checks if you are okay with it.

[Charlie Reis, 2014/03/04 20:32:03]

Yes, please remove them.  Better to crash early if this is being used in
unexpected ways.

[hush (inactive), 2014/03/04 23:22:33]

Done.

+    return;
+  }
   // Replace any occurrences of swappedout:// with about:blank.
   const WebURL& blank_url = GURL(kAboutBlankURL);
   WebVector<WebURL> urls;
@@ -2106,7 +2109,6 @@ void RenderFrameImpl::UpdateURL(blink::WebFrame* frame) {
   DCHECK(ds);
 
   const WebURLRequest& request = ds->request();
-  const WebURLRequest& original_request = ds->originalRequest();
   const WebURLResponse& response = ds->response();
 
   DocumentState* document_state = DocumentState::FromDataSource(ds);
@@ -2227,10 +2229,7 @@ void RenderFrameImpl::UpdateURL(blink::WebFrame* frame) {
     // Track the URL of the original request.  We use the first entry of the
     // redirect chain if it exists because the chain may have started in another
     // process.
-    if (params.redirects.size() > 0)
-      params.original_request_url = params.redirects.at(0);
-    else
-      params.original_request_url = original_request.url();
+    params.original_request_url = GetOriginalRequestUrl(ds);
 
     params.history_list_was_cleared =
         navigation_state->history_list_was_cleared();
@@ -2281,4 +2280,29 @@ void RenderFrameImpl::didStopLoading() {
   Send(new FrameHostMsg_DidStopLoading(routing_id_));
 }
 
+const GURL RenderFrameImpl::GetOriginalRequestUrl(const WebDataSource* ds) {

[Charlie Reis, 2014/03/03 18:21:11]

No const in either part.

[hush (inactive), 2014/03/03 23:12:52]

Please see the previous reply about "const"

+  if (!ds) {

[Charlie Reis, 2014/03/03 18:21:11]

This check seems unnecessary.

[hush (inactive), 2014/03/03 23:12:52]

Please the previous reply about null check.

+    return GURL();
+  }
+
+  std::vector<GURL> redirects;
+  GetRedirectChain(ds, &redirects);
+
+  if (ds->hasUnreachableURL()) {
+    // The frame is loaded via WebFrame::loadData, where we pass the
+    // history_url_for_data_url as unreachableURL in blink. The

[Charlie Reis, 2014/03/03 18:21:11]

I'm not sure this comment is accurate.  That's not the only time
hasUnreachableURL is true, is it?  We use it for error pages as well.

@darin or @rdsmith might be able to comment more, since I think they know a bit
more about the error pages than I do.

[hush (inactive), 2014/03/03 23:12:52]

You are right, error page uses unreachableURL as well. (I'm currently not aware
of a 3rd case where unreachableURL is used.)
If I try to visit "http://non.existent.url" in a tab, the error page is
displayed by calling loadData with the parameters: baseUrl = chrome error page,
historyUrl = "http://non.existent.url"
That means the original url for the tab is the chrome error page. My change will
make the original url for the tab to "http://non.existent.url".
But I think this is reasonable because there is in fact no redirect from chrome
error page to "http://non.existent.url"

[Charlie Reis, 2014/03/04 20:32:03]

That's a behavior change that I'm not sure how to evaluate.  I defer to @darin
or @rdsmith there.

+    // base_url_for_data_url will be in the redirect chain, but we
+    // never actually visited base_url_for_data_url. So we need to set
+    // history_url_for_data_url as the original_request_url.
+    return ds->unreachableURL();
+  }
+
+  if (redirects.size() > 0) {

[Charlie Reis, 2014/03/03 18:21:11]

No braces for a one-line body.

[hush (inactive), 2014/03/03 23:12:52]

Okay

+    return redirects.at(0);
+  }
+  else {

[Charlie Reis, 2014/03/03 18:21:11]

No need for an else block if your if block returns.

[hush (inactive), 2014/03/03 23:12:52]

okay

+    return ds->originalRequest().url();
+  }
+}
+
 }  // namespace content