Switch RenderViewContextMenu to use RequestOpenURL

content/public/browser/page_navigator.h

Index: content/public/browser/page_navigator.h
diff --git a/content/public/browser/page_navigator.h b/content/public/browser/page_navigator.h
index 945c96ec763622ccb5d52ba036cd723e8867deec..b7fd9f96e0dd7ff8f3ce19a91d838cfc7698b993 100644
--- a/content/public/browser/page_navigator.h
+++ b/content/public/browser/page_navigator.h
@@ -14,6 +14,7 @@
 #include "base/memory/ref_counted.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/global_request_id.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/site_instance.h"
 #include "content/public/common/referrer.h"
 #include "content/public/common/resource_request_body.h"
@@ -99,6 +100,10 @@ class PageNavigator {
  public:
   virtual ~PageNavigator() {}
 
+  // Opens a URL with the given parameters.
+  virtual void RequestOpenURL(RenderFrameHost* render_frame_host,

[Charlie Reis, 2017/02/02 23:34:22]

I think we'll need to find a way to improve this part, for a few reasons.

One, it makes this PageNavigator interface confusing.  Callers don't have a good
way know when to use OpenURL vs RequestOpenURL, which have different return
types but only differ in whether they fire DidOpenRequestedURL afterward.  I
feel like we shouldn't make them face this choice, though I suppose better
documentation might be one option.

Two, it introduces a confusion over |render_frame_host|, which can differ from
the frame corresponding to OpenURLParams::frame_tree_node_id.  If I understand
correctly, |render_frame_host| should really be called
|source_render_frame_host|, and frame_tree_node_id is the destination frame (or
-1 for the main frame).

I see that we need this to give context menu code (which is in components) a way
to generate DidOpenRequestedURL with the correct source RFH.  No one else should
need to do this, though, so maybe we can move this out of PageNavigator and onto
WebContents itself, with an explanation of the difference and a disclaimer that
most people should use PageNavigator::OpenURL instead?

(This still kind of feels like exposing an internal helper method as a public
interface, which is unfortunate.  I don't see a way around that given how the
context menu code is in components, though.)

+                              const OpenURLParams& params) {}
+
   // Opens a URL with the given disposition.  The transition specifies how this
   // navigation should be recorded in the history system (for example, typed).
   // Returns the WebContents the URL is opened in, or nullptr if the URL wasn't