Switch RenderViewContextMenu to use RequestOpenURL

content/public/browser/page_navigator.h

 // 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.
 
 // PageNavigator defines an interface that can be used to express the user's
 // intention to navigate to a particular URL.  The implementing class should
 // perform the navigation.
 
 #ifndef CONTENT_PUBLIC_BROWSER_PAGE_NAVIGATOR_H_
 #define CONTENT_PUBLIC_BROWSER_PAGE_NAVIGATOR_H_
 
 #include <string>
 
 #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"
 #include "ui/base/page_transition_types.h"
 #include "ui/base/window_open_disposition.h"
 #include "url/gurl.h"
 
 namespace content {
 
 class WebContents;
@@ skipping 65 matching lines @@
   bool started_from_context_menu;
 
  private:
   OpenURLParams();
 };
 
 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
   // opened immediately.
   virtual WebContents* OpenURL(const OpenURLParams& params) = 0;
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_BROWSER_PAGE_NAVIGATOR_H_