Transfer user agent override info between browser and renderer

content/browser/web_contents/navigation_controller_impl.cc

 // Copyright (c) 2012 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/browser/web_contents/navigation_controller_impl.h"
 
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/string_number_conversions.h"  // Temporary
 #include "base/string_util.h"
@@ skipping 556 matching lines @@
   entry->set_transferred_global_request_id(transferred_global_request_id);
 
   LoadEntry(entry);
 }
 
 void NavigationControllerImpl::LoadURL(
     const GURL& url,
     const content::Referrer& referrer,
     content::PageTransition transition,
     const std::string& extra_headers) {
+  bool override = false;
+
+  // Carry over the user agent override if we're doing a regular navigation.
+  bool carry_over = transition == content::PAGE_TRANSITION_TYPED ||
+      transition == content::PAGE_TRANSITION_AUTO_BOOKMARK ||
+      transition == content::PAGE_TRANSITION_GENERATED;

[gone, 2012/05/24 18:26:21]

These conditions were the only ones we encountered on Android, save for the new
tab page.  This will likely have to be generalized, but I don't know the right
ones to use.

[jochen (gone - plz use gerrit), 2012/05/31 14:09:16]

What is the intention? When should the UA override be dropped?

[gone, 2012/05/31 21:43:25]

We intend to carry the override around until a call is made to explicitly turn
it off.  I'm not sure if this should include subframe navigations and the like,
though.

[Charlie Reis, 2012/06/01 00:36:08]

As noted before, I don't understand why it's per-RenderView.  What happens if
you open a link in a new tab?  What happens if you navigate cross-site to a
different process?

It feels odd to me to preserve the setting for a single tab, since we don't do
that for other similar settings like zoom level.

+  if (GetLastCommittedEntry() && carry_over)
+    override = GetLastCommittedEntry()->GetIsOverridingUserAgent();
+
+  LoadURLWithUserAgentOverride(url, referrer, transition, extra_headers,
+      override);
+}
+
+void NavigationControllerImpl::LoadURLWithUserAgentOverride(
+    const GURL& url,
+    const content::Referrer& referrer,
+    content::PageTransition transition,
+    const std::string& extra_headers,
+    bool is_overriding_user_agent) {
   if (content::HandleDebugURL(url, transition))
     return;
 
   // The user initiated a load, we don't need to reload anymore.
   needs_reload_ = false;
 
   NavigationEntryImpl* entry = NavigationEntryImpl::FromNavigationEntry(
       CreateNavigationEntry(
           url, referrer, transition, false, extra_headers, browser_context_));
+  entry->SetIsOverridingUserAgent(is_overriding_user_agent);
 
   LoadEntry(entry);
 }
 
 void NavigationControllerImpl::LoadURLFromRenderer(

[Charlie Reis, 2012/06/01 00:36:08]

What about this case?  Do we only care about user agent overrides for
browser-initiated navigations?

[gone, 2012/06/01 01:04:06]

We catch renderer-initiated navigations when it sends a message back through
UpdateURL, though that might not be the right way to go.

[Charlie Reis, 2012/06/01 18:04:09]

UpdateURL is called after the navigation commits in WebKit.  Isn't that too late
for setting the user agent?  Or is WebKit grabbing RenderView's state when doing
the navigation itself?

Hmm, I see how this is tricky.  I'm still concerned about races.

[gone, 2012/06/14 00:46:39]

I've added this case in.
If we force a reload whenever replacing a user agent override that is actively
being used, it should avoid issues with races.

     const GURL& url,
     const content::Referrer& referrer,
     content::PageTransition transition,
     const std::string& extra_headers) {
   // The user initiated a load, we don't need to reload anymore.
   needs_reload_ = false;
 
   NavigationEntryImpl* entry = NavigationEntryImpl::FromNavigationEntry(
       CreateNavigationEntry(
           url, referrer, transition, true, extra_headers, browser_context_));
@@ skipping 260 matching lines @@
   new_entry->SetURL(params.url);
   if (update_virtual_url)
     UpdateVirtualURLToURL(new_entry, params.url);
   new_entry->SetReferrer(params.referrer);
   new_entry->SetPageID(params.page_id);
   new_entry->SetTransitionType(params.transition);
   new_entry->set_site_instance(
       static_cast<SiteInstanceImpl*>(web_contents_->GetSiteInstance()));
   new_entry->SetHasPostData(params.is_post);
   new_entry->SetPostID(params.post_id);
-
-  if (params.redirects.size() > 0)
-    new_entry->SetOriginalRequestURL(params.redirects[0]);
-  else
-    new_entry->SetOriginalRequestURL(params.url);
+  new_entry->SetOriginalRequestURL(params.original_request_url);

[tony, 2012/05/31 17:25:00]

How are original_request_url and url/redirects[0] different? Is this change
intentional?

[gone, 2012/05/31 21:43:25]

It's intentional; the redirect chain doesn't always match the original request
URL: http://b/issue?id=6213026&#ISSUE_HistoryHeader24

[tony, 2012/05/31 21:59:02]

I don't fully understand the issue, but is this bug specific to overriding user
agents? If not, it might be good to split this out into a separate change with a
test.

[gone, 2012/06/01 01:04:06]

Will do.

+  new_entry->SetIsOverridingUserAgent(params.is_overriding_user_agent);
 
   InsertOrReplaceEntry(new_entry, *did_replace_entry);
 }
 
 void NavigationControllerImpl::RendererDidNavigateToExistingPage(
     const ViewHostMsg_FrameNavigate_Params& params) {
   // We should only get here for main frame navigations.
   DCHECK(content::PageTransitionIsMainFrame(params.transition));
 
   // This is a back/forward navigation. The existing page for the ID is
@@ skipping 546 matching lines @@
   for (int i = 0; i < max_index; i++) {
     // When cloning a tab, copy all entries except interstitial pages
     if (source.entries_[i].get()->GetPageType() !=
         content::PAGE_TYPE_INTERSTITIAL) {
       entries_.insert(entries_.begin() + insert_index++,
                       linked_ptr<NavigationEntryImpl>(
                           new NavigationEntryImpl(*source.entries_[i])));
     }
   }
 }