Move content shell context menu to ShellWebContentsViewDelegate in Views

Move content shell context menu to ShellWebContentsViewDelegate in Views

Allows content shell to run context menu handling code in
WebContentsViewAura, including touch selection code. This is needed to
be able to properly test long press touch selection behavior.

BUG=594101
Committed: https://crrev.com/6eb57fb790e8c319cc80d9d6a9d5ff5f6d042e32
Cr-Commit-Position: refs/heads/master@{#407008}

[mohsen, 2016-07-12 05:04:28]

Description was changed from

==========
[WIP] Remove content shell custom context menu

BUG=594101
==========

to

==========
Move content shell context menu to ShellWebContentsViewDelegate in Aura

Allows content shell to run context menu handling code in
WebContentsViewAura, including touch selection code. This is needed to
be able to properly test long press touch selection behavior.

BUG=594101
==========

[mohsen, 2016-07-13 18:14:13]

mohsen@chromium.org changed reviewers:
+ sadrul@chromium.org

[mohsen, 2016-07-13 18:14:13]

sadrul@: Can you please take a first look before I send it to OWNERS?

[mohsen, 2016-07-18 17:17:08]

sadrul@: ping!

[sadrul, 2016-07-19 22:22:11]

Cool cleanup! lgtm

[mohsen, 2016-07-20 15:21:11]

mohsen@chromium.org changed reviewers:
+ peter@chromium.org

[mohsen, 2016-07-20 15:21:12]

peter@: Please take a look...

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

lgtm % question

https://codereview.chromium.org/2132983002/diff/100001/content/shell/browser/...
File content/shell/browser/shell.cc (left):

https://codereview.chromium.org/2132983002/diff/100001/content/shell/browser/...
content/shell/browser/shell.cc:293: InnerShowDevTools();
nit: please inline InnerShowDevTools() in Shell::ShowDevTools() now that there's
only a single caller left.

https://codereview.chromium.org/2132983002/diff/100001/content/shell/browser/...
File content/shell/browser/shell_web_contents_view_delegate_aura.cc (right):

https://codereview.chromium.org/2132983002/diff/100001/content/shell/browser/...
content/shell/browser/shell_web_contents_view_delegate_aura.cc:25: const
content::ContextMenuParams& params)
micro nit: drop content::

https://codereview.chromium.org/2132983002/diff/100001/content/shell/browser/...
content/shell/browser/shell_web_contents_view_delegate_aura.cc:103: return;
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:10:04]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 22:10:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

sadrul@ noticed that the WebContentsViewDelegate I'm adding is actually for
Views not Aura. So, I renamed it and updated gyp/gn files to include it in codes
using views rather than aura. Please take another look...

https://codereview.chromium.org/2132983002/diff/100001/content/shell/browser/...
File content/shell/browser/shell.cc (left):

https://codereview.chromium.org/2132983002/diff/100001/content/shell/browser/...
content/shell/browser/shell.cc:293: InnerShowDevTools();
On 2016/07/20 at 16:40:14, Peter Beverloo wrote:
> nit: please inline InnerShowDevTools() in Shell::ShowDevTools() now that
there's only a single caller left.

Done.

https://codereview.chromium.org/2132983002/diff/100001/content/shell/browser/...
File content/shell/browser/shell_web_contents_view_delegate_aura.cc (right):

https://codereview.chromium.org/2132983002/diff/100001/content/shell/browser/...
content/shell/browser/shell_web_contents_view_delegate_aura.cc:25: const
content::ContextMenuParams& params)
On 2016/07/20 at 16:40:14, Peter Beverloo wrote:
> micro nit: drop content::

Done.

https://codereview.chromium.org/2132983002/diff/100001/content/shell/browser/...
content/shell/browser/shell_web_contents_view_delegate_aura.cc:103: return;
On 2016/07/20 at 16:40:14, Peter Beverloo wrote:
> 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?

(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?

[commit-bot: I haz the power, 2016-07-20 22:18:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 22:19:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Peter Beverloo, 2016-07-20 22:25:03]

On 2016/07/20 22:16:31, mohsen wrote:
> sadrul@ noticed that the WebContentsViewDelegate I'm adding is actually for
> Views not Aura. So, I renamed it and updated gyp/gn files to include it in
codes
> using views rather than aura. Please take another look...
> 
>
https://codereview.chromium.org/2132983002/diff/100001/content/shell/browser/...
> File content/shell/browser/shell.cc (left):
> 
>
https://codereview.chromium.org/2132983002/diff/100001/content/shell/browser/...
> content/shell/browser/shell.cc:293: InnerShowDevTools();
> On 2016/07/20 at 16:40:14, Peter Beverloo wrote:
> > nit: please inline InnerShowDevTools() in Shell::ShowDevTools() now that
> there's only a single caller left.
> 
> Done.
> 
>
https://codereview.chromium.org/2132983002/diff/100001/content/shell/browser/...
> File content/shell/browser/shell_web_contents_view_delegate_aura.cc (right):
> 
>
https://codereview.chromium.org/2132983002/diff/100001/content/shell/browser/...
> content/shell/browser/shell_web_contents_view_delegate_aura.cc:25: const
> content::ContextMenuParams& params)
> On 2016/07/20 at 16:40:14, Peter Beverloo wrote:
> > micro nit: drop content::
> 
> Done.
> 
>
https://codereview.chromium.org/2132983002/diff/100001/content/shell/browser/...
> content/shell/browser/shell_web_contents_view_delegate_aura.cc:103: return;
> On 2016/07/20 at 16:40:14, Peter Beverloo wrote:
> > 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?
> 
> (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?

Thank you for the updates, as well as the additional detail.

Before the menu went on-heap, I _think_ there may have been cases where
references to the ContextMenuModel would remain, so we'd rely on the fact that
RunMenuAt() would indicate that the menu has been deleted. That could be
DCHECK'ed as:

views::MenuRunner::RunResult result = context_menu_model...;
DCHECK_EQ(views::MenuRunner::MENU_DELETED, result);

But this is less of an issue now that it lives on the stack.

[mohsen, 2016-07-21 01:43:58]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 01:44:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mohsen, 2016-07-21 01:44:55]

Description was changed from

==========
Move content shell context menu to ShellWebContentsViewDelegate in Aura

Allows content shell to run context menu handling code in
WebContentsViewAura, including touch selection code. This is needed to
be able to properly test long press touch selection behavior.

BUG=594101
==========

to

==========
Move content shell context menu to ShellWebContentsViewDelegate in Views

Allows content shell to run context menu handling code in
WebContentsViewAura, including touch selection code. This is needed to
be able to properly test long press touch selection behavior.

BUG=594101
==========

[commit-bot: I haz the power, 2016-07-21 02:03:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 02:03:25]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[mohsen, 2016-07-21 02:20:33]

Patchset #8 (id:140001) has been deleted

[mohsen, 2016-07-21 02:23:07]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 02:23:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 02:46:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 02:46:47]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[mohsen, 2016-07-21 20:06:12]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 20:06:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 20:20:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 20:20:04]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[mohsen, 2016-07-21 20:33:06]

Patchset #9 (id:180001) has been deleted

[mohsen, 2016-07-21 20:38:40]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 20:39:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 21:09:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 21:09:49]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[mohsen, 2016-07-21 21:31:59]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 21:32:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 23:55:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 23:55:20]

Dry run: This issue passed the CQ dry run.

[mohsen, 2016-07-22 01:18:54]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2016-07-22 01:18:54]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2132983002/#ps220001
(title: "Fixed cast_shell_linux compile error")

[commit-bot: I haz the power, 2016-07-22 01:19:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 03:14:23]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2016-07-22 03:17:38]

Description was changed from

==========
Move content shell context menu to ShellWebContentsViewDelegate in Views

Allows content shell to run context menu handling code in
WebContentsViewAura, including touch selection code. This is needed to
be able to properly test long press touch selection behavior.

BUG=594101
==========

to

==========
Move content shell context menu to ShellWebContentsViewDelegate in Views

Allows content shell to run context menu handling code in
WebContentsViewAura, including touch selection code. This is needed to
be able to properly test long press touch selection behavior.

BUG=594101

Committed: https://crrev.com/6eb57fb790e8c319cc80d9d6a9d5ff5f6d042e32
Cr-Commit-Position: refs/heads/master@{#407008}
==========

[commit-bot: I haz the power, 2016-07-22 03:17:39]

Patchset 10 (id:??) landed as
https://crrev.com/6eb57fb790e8c319cc80d9d6a9d5ff5f6d042e32
Cr-Commit-Position: refs/heads/master@{#407008}