Switch RenderViewContextMenu to use RequestOpenURL

content/browser/web_contents/web_contents_impl.cc

Index: content/browser/web_contents/web_contents_impl.cc
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 6fcb922d3e4c98a0d03e10065ef7d290fafa0c4c..c393403e4d5a73f79f6233a74f5795f5c57a8c66 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -2575,6 +2575,19 @@ WebContents* WebContentsImpl::OpenURL(const OpenURLParams& params) {
     return NULL;
 
   WebContents* new_contents = delegate_->OpenURLFromTab(this, params);
+
+  RenderFrameHost* render_frame_host =
+      RenderFrameHost::FromID(params.render_process_id, params.render_frame_id);
+

[nasko, 2017/02/14 01:20:11]

I realized we discussed one thing I forgot to mention in my previous review.

Since we are now supplying a source RenderFrameHost and a source SiteInstance,
we should ensure that they are matching, otherwise mismatches will likely be
problematic. Let's put a DCHECK or a CHECK that the SiteInstance of the source
RenderFrameHost, if supplied, matches the source SiteInstance, if supplied.

[Patrick Noland, 2017/02/14 21:33:59]

Done.

+  if (new_contents && render_frame_host && new_contents != this) {
+    for (auto& observer : observers_) {
+      observer.DidOpenRequestedURL(new_contents, render_frame_host, params.url,
+                                   params.referrer, params.disposition,
+                                   params.transition,
+                                   params.started_from_context_menu);
+    }
+  }
+
   return new_contents;
 }
 
@@ -3368,25 +3381,6 @@ void WebContentsImpl::DidStartNavigationToPendingEntry(const GURL& url,
     observer.DidStartNavigationToPendingEntry(url, reload_type);
 }
 
-void WebContentsImpl::RequestOpenURL(RenderFrameHostImpl* render_frame_host,
-                                     const OpenURLParams& params) {
-  // OpenURL can blow away the source RFH. Use the process/frame routing ID as a
-  // weak pointer of sorts.
-  const int32_t process_id = render_frame_host->GetProcess()->GetID();
-  const int32_t frame_id = render_frame_host->GetRoutingID();
-
-  WebContents* new_contents = OpenURL(params);
-
-  if (new_contents && RenderFrameHost::FromID(process_id, frame_id)) {
-    // Notify observers.
-    for (auto& observer : observers_) {
-      observer.DidOpenRequestedURL(new_contents, render_frame_host, params.url,
-                                   params.referrer, params.disposition,
-                                   params.transition);
-    }
-  }
-}
-
 bool WebContentsImpl::ShouldTransferNavigation(bool is_main_frame_navigation) {
   if (!delegate_)
     return true;