Move content shell context menu to ShellWebContentsViewDelegate in Views

content/shell/browser/shell_web_contents_view_delegate_aura.cc

Index: content/shell/browser/shell_web_contents_view_delegate_aura.cc
diff --git a/content/shell/browser/shell_web_contents_view_delegate_aura.cc b/content/shell/browser/shell_web_contents_view_delegate_aura.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b06df3f5fdcb4d4871f5d070653b1b2cc32d2332
--- /dev/null
+++ b/content/shell/browser/shell_web_contents_view_delegate_aura.cc
@@ -0,0 +1,107 @@
+// Copyright 2016 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/shell/browser/shell_web_contents_view_delegate.h"
+
+#include "base/strings/utf_string_conversions.h"
+#include "content/public/browser/web_contents.h"
+#include "content/shell/browser/shell_devtools_frontend.h"
+#include "content/shell/common/shell_switches.h"
+#include "ui/aura/client/screen_position_client.h"
+#include "ui/aura/window.h"
+#include "ui/base/models/simple_menu_model.h"
+#include "ui/views/controls/menu/menu_runner.h"
+#include "ui/views/widget/widget.h"
+
+namespace content {
+namespace {
+
+// Model for the "Debug" menu
+class ContextMenuModel : public ui::SimpleMenuModel,
+                         public ui::SimpleMenuModel::Delegate {
+ public:
+  ContextMenuModel(WebContents* web_contents,
+                   const content::ContextMenuParams& params)

[Peter Beverloo, 2016/07/20 16:40:14]

micro nit: drop content::

[mohsen, 2016/07/20 22:16:31]

Done.

+      : ui::SimpleMenuModel(this),
+        web_contents_(web_contents),
+        params_(params) {
+    AddItem(COMMAND_OPEN_DEVTOOLS, base::ASCIIToUTF16("Inspect Element"));
+  }
+  ~ContextMenuModel() override {}
+
+  // ui::SimpleMenuModel::Delegate:
+  bool IsCommandIdChecked(int command_id) const override { return false; }
+  bool IsCommandIdEnabled(int command_id) const override { return true; }
+  bool GetAcceleratorForCommandId(int command_id,
+                                  ui::Accelerator* accelerator) const override {
+    return false;
+  }
+  void ExecuteCommand(int command_id, int event_flags) override {
+    switch (command_id) {
+      case COMMAND_OPEN_DEVTOOLS:
+        ShellDevToolsFrontend* devtools_frontend =
+            ShellDevToolsFrontend::Show(web_contents_);
+        devtools_frontend->Activate();
+        devtools_frontend->Focus();
+        devtools_frontend->InspectElementAt(params_.x, params_.y);
+        break;
+    };
+  }
+
+ private:
+  enum CommandID { COMMAND_OPEN_DEVTOOLS };
+
+  WebContents* web_contents_;
+  ContextMenuParams params_;
+
+  DISALLOW_COPY_AND_ASSIGN(ContextMenuModel);
+};
+
+}  // namespace
+
+WebContentsViewDelegate* CreateShellWebContentsViewDelegate(
+    WebContents* web_contents) {
+  return new ShellWebContentsViewDelegate(web_contents);
+}
+
+ShellWebContentsViewDelegate::ShellWebContentsViewDelegate(
+    WebContents* web_contents)
+    : web_contents_(web_contents) {}
+
+ShellWebContentsViewDelegate::~ShellWebContentsViewDelegate() {}
+
+void ShellWebContentsViewDelegate::ShowContextMenu(
+    RenderFrameHost* render_frame_host,
+    const ContextMenuParams& params) {
+  if (switches::IsRunLayoutTestSwitchPresent())
+    return;
+
+  gfx::Point screen_point(params.x, params.y);
+
+  // Convert from content coordinates to window coordinates.
+  // This code copied from chrome_web_contents_view_delegate_views.cc
+  aura::Window* web_contents_window = web_contents_->GetNativeView();
+  aura::Window* root_window = web_contents_window->GetRootWindow();
+  aura::client::ScreenPositionClient* screen_position_client =
+      aura::client::GetScreenPositionClient(root_window);
+  if (screen_position_client) {
+    screen_position_client->ConvertPointToScreen(web_contents_window,
+                                                 &screen_point);
+  }
+
+  ContextMenuModel context_menu_model(web_contents_, params);
+  views::MenuRunner context_menu_runner(&context_menu_model,
+                                        views::MenuRunner::CONTEXT_MENU);
+
+  views::Widget* widget = views::Widget::GetWidgetForNativeView(
+      web_contents_->GetTopLevelNativeWindow());
+  if (context_menu_runner.RunMenuAt(
+          widget, nullptr, gfx::Rect(screen_point, gfx::Size()),
+          views::MENU_ANCHOR_TOPRIGHT,
+          ui::MENU_SOURCE_NONE) == views::MenuRunner::MENU_DELETED) {
+    return;

[Peter Beverloo, 2016/07/20 16:40:14]

I'll defer to Sadrul for the details here, but a few thoughts about part
of the change:

  (1) Both the ContextMenuModel and the views::MenuRunner used to be heap-
      allocated, but are now stack-allocated. The class-level comment of
      views::MenuRunner explicitly warns against this because the |this|
      may go away before this object goes off-stack. Can that happen here?

  (2) The RunMenuAt return value indicates whether it was deleted, for
      which the defensive return is in place. This leads to a really odd
      pattern in the code (which has already been duplicated 12 times)
      where there's a "return" statement immediately before ending the
      function's control flow. I get why it's there, but:

      (a) This makes it feel like RunMenuAt() should really be annotated
          with WARN_UNUSED_RESULT;
      (b) Since content shell is our test runner, would it be sensible to
          DCHECK_EQ so that we understand when this goes wrong?

[mohsen, 2016/07/20 22:16:31]

(1) I think that might be possible. I allocated them on stack to avoid adding
#ifdef'd aura-only fields to the ShellWebContentsViewDelegate class. But, I
think that might be problematic. Changed to heap-allocated.

(2.a) This function used to be annotated with WARN_UNUSED_RESULT. Since we added
the ability to have async menus, that was removed as the return value is not
used for async menus (see
https://codereview.chromium.org/1647003002/diff/40001/chrome/browser/ui/views...).
There is also a plan to turn all existing menus into async in which case this
pattern will go away completely.

(2.b) Can you explain more? Where do you suggest adding DCHECK to?

+  }
+}
+
+}  // namespace content