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

content/renderer/render_frame_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 
 #include "base/command_line.h"
@@ skipping 117 matching lines @@
 
   return item.httpBody().identifier();
 }
 
 WebURLResponseExtraDataImpl* GetExtraDataFromResponse(
     const WebURLResponse& response) {
   return static_cast<WebURLResponseExtraDataImpl*>(
       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;
   ds->redirectChain(urls);
   result->reserve(urls.size());
   for (size_t i = 0; i < urls.size(); ++i) {
     if (urls[i] != GURL(kSwappedOutURL))
       result->push_back(urls[i]);
     else
       result->push_back(blank_url);
@@ skipping 1950 matching lines @@
   FOR_EACH_OBSERVER(RenderFrameObserver, observers_, OnStop());
 }
 
 // Tell the embedding application that the URL of the active page has changed.
 void RenderFrameImpl::UpdateURL(blink::WebFrame* frame) {
   DCHECK(!frame_ || frame_ == frame);
   WebDataSource* ds = frame->dataSource();
   DCHECK(ds);
 
   const WebURLRequest& request = ds->request();
-  const WebURLRequest& original_request = ds->originalRequest();
   const WebURLResponse& response = ds->response();
 
   DocumentState* document_state = DocumentState::FromDataSource(ds);
   NavigationState* navigation_state = document_state->navigation_state();
   InternalDocumentStateData* internal_data =
       InternalDocumentStateData::FromDocumentState(document_state);
 
   FrameHostMsg_DidCommitProvisionalLoad_Params params;
   params.http_status_code = response.httpStatusCode();
   params.is_post = false;
@@ skipping 100 matching lines @@
       params.is_post = true;
       params.post_id = ExtractPostId(item);
     }
 
     // Send the user agent override back.
     params.is_overriding_user_agent = internal_data->is_overriding_user_agent();
 
     // 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();
 
     // Save some histogram data so we can compute the average memory used per
     // page load of the glyphs.
     UMA_HISTOGRAM_COUNTS_10000("Memory.GlyphPagesPerLoad",
                                blink::WebGlyphCache::pageCount());
 
     // This message needs to be sent before any of allowScripts(),
@@ skipping 30 matching lines @@
 }
 
 void RenderFrameImpl::didStartLoading() {
   Send(new FrameHostMsg_DidStartLoading(routing_id_));
 }
 
 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