Provide default implementation for WebContentsDelegate::OpenURLFromTab.

content/public/browser/web_contents_delegate.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/public/browser/web_contents_delegate.h"
 
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/memory/singleton.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/security_style_explanations.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/bindings_policy.h"
 #include "content/public/common/security_style.h"
 #include "content/public/common/url_constants.h"
 #include "ui/gfx/geometry/rect.h"
 
 namespace content {
 
 WebContentsDelegate::WebContentsDelegate() {
 }
 
 WebContents* WebContentsDelegate::OpenURLFromTab(WebContents* source,
                                                  const OpenURLParams& params) {
-  return nullptr;
+  // CURRENT_TAB is the only one we implement for now.

[Charlie Reis, 2015/11/25 00:05:28]

// This default implementation only handles CURRENT_TAB.

[alexmos, 2015/11/25 19:15:39]

Done.

+  if (params.disposition != CURRENT_TAB)
+      return nullptr;
+
+  NavigationController::LoadURLParams load_url_params(params.url);
+  load_url_params.source_site_instance = params.source_site_instance;

[Charlie Reis, 2015/11/25 00:05:28]

Let's make sure we cover all the parameters from OpenURLParams (which have
matches in LoadURLParams, like redirect_chain), and in the same order as they're
declared.

[alexmos, 2015/11/25 19:15:39]

I've added redirect_chain and rearranged them.

I've also added the browser_initiated_post_data translation, but I'm not quite
sure of the cases when it needs to be copied.  I ended up mirroring the
condition from LoadURLInContents in browser_navigator.cc , and also setting
load_type accordingly for posts -- let me know if that makes sense.

I've left out the user_gesture -> has_user_gesture translation. The
LoadURLParams flag is only declared and used for Android, so I'm not sure it's
needed here, but let me know if you think it's better to include it.

[Charlie Reis, 2015/11/25 19:34:25]

Seems right.
That would only affect WebContentsDelegates on Android that don't override
OpenURLFromTab.  Interestingly, WebContentsDelegateAndroid already overrides
OpenURLFromTab and doesn't copy over user_gesture.  Not sure if that's a bug on
their part, but at least it's fail closed, and it probably means we don't need
to worry about it here.

+  load_url_params.referrer = params.referrer;
+  load_url_params.frame_tree_node_id = params.frame_tree_node_id;
+  load_url_params.transition_type = params.transition;
+  load_url_params.extra_headers = params.extra_headers;
+  load_url_params.should_replace_current_entry =
+      params.should_replace_current_entry;
+
+  if (params.transferred_global_request_id != GlobalRequestID()) {
+    load_url_params.is_renderer_initiated = params.is_renderer_initiated;
+    load_url_params.transferred_global_request_id =
+        params.transferred_global_request_id;
+  } else if (params.is_renderer_initiated) {
+    load_url_params.is_renderer_initiated = true;

[Charlie Reis, 2015/11/25 00:05:28]

I can't figure out why shell.cc had this obscure approach to setting
is_renderer_initiated (added in https://codereview.chromium.org/22904002).  It's
false by default, so it seems like it will always match
params.is_renderer_initiated by the end of this method.  Can we just set it
directly as in the cases above, independent of whether there's a
transferred_global_request_id?

[alexmos, 2015/11/25 19:15:39]

Done.  Same seems to be true for transferred_global_request_id (the default is
GlobalRequestID(), so no reason to only set it if it's not GlobalRequestID(). 
I'm setting both directly now.

I think the source of this may be just that it was copied over from
LoadURLInContents in browser_navigator.cc, and there this logic was added in
https://chromiumcodereview.appspot.com/10830144 (which you reviewed :) ). 

[Charlie Reis, 2015/11/25 19:34:25]

Ah, yes, nice find.  Specifically:
https://codereview.chromium.org/10830144/diff/2003/chrome/browser/ui/browser_...
That seems to have gone through contortions due to the old variations on LoadURL
/ LoadURLFromRenderer / TransferURL.  So we could have simplified that logic
further at the time.

+  }
+
+  source->GetController().LoadURLWithParams(load_url_params);
+  return source;
 }
 
 bool WebContentsDelegate::IsPopupOrPanel(const WebContents* source) const {
   return false;
 }
 
 bool WebContentsDelegate::CanOverscrollContent() const { return false; }
 
 gfx::Rect WebContentsDelegate::GetRootWindowResizerRect() const {
   return gfx::Rect();
@@ skipping 212 matching lines @@
     SecurityStyleExplanations* security_style_explanations) {
   return content::SECURITY_STYLE_UNKNOWN;
 }
 
 void WebContentsDelegate::ShowCertificateViewerInDevTools(
     WebContents* web_contents,
     int cert_id) {
 }
 
 }  // namespace content