Move ContextMenu show/hide state tracking to WebContents

Move ContextMenu show/hide state tracking to WebContents

Currently, RenderWidgetHostView of the page tracks the visibility state of context
menus. However, the RWHV might go away during navigations (cross origin). On the
other hand, a context menu might outlive the RWHV and stay visible during the page
navigation which leads to a mismatch between the stored visibility state in the
new RWHV and the actual visibility of the context menu.

This CL will move this state tracking logic to WebContents which stays the same
during page navigations.

BUG=708182
Review-Url: https://codereview.chromium.org/2890143003
Cr-Commit-Position: refs/heads/master@{#477346}
Committed: https://chromium.googlesource.com/chromium/src/+/f6750aae23ef4c4fd8e500f092bff269ca277abc

[EhsanK, 2017-05-18 15:30:17]

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

[commit-bot: I haz the power, 2017-05-18 15:30:44]

Dry run: CQ is trying da patch.

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

[EhsanK, 2017-05-18 15:38:30]

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

[commit-bot: I haz the power, 2017-05-18 15:39:27]

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, 2017-05-18 15:53:54]

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

[commit-bot: I haz the power, 2017-05-18 15:53:55]

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...)
  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_...)

[EhsanK, 2017-05-18 17:31:24]

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

[commit-bot: I haz the power, 2017-05-18 17:33:18]

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, 2017-05-18 17:46:36]

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

[commit-bot: I haz the power, 2017-05-18 17:46:38]

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

[EhsanK, 2017-05-23 14:58:38]

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

[commit-bot: I haz the power, 2017-05-23 14:59:03]

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, 2017-05-23 15:02:57]

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

[commit-bot: I haz the power, 2017-05-23 15:02:58]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[EhsanK, 2017-05-23 17:18:10]

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

[commit-bot: I haz the power, 2017-05-23 17:18:39]

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, 2017-05-23 17:37:19]

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

[commit-bot: I haz the power, 2017-05-23 17:37:20]

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...)

[EhsanK, 2017-05-24 14:40:43]

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

[commit-bot: I haz the power, 2017-05-24 14:40:57]

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, 2017-05-24 15:45:02]

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

[commit-bot: I haz the power, 2017-05-24 15:45:03]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2017-05-24 16:02:10]

ekaramad@chromium.org changed reviewers:
+ kenrb@chromium.org

[EhsanK, 2017-05-24 16:02:11]

Hello Ken. Could you please take a look? I think this is about what it takes to
keep the DCEHCK but fix hitting it.

[kenrb, 2017-06-05 17:39:16]

lgtm with nits. It seems reasonable to move that bit of state to the
WebContents.

https://codereview.chromium.org/2890143003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2890143003/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:397: // Called by
WebContensImpl to notify the view about a change in visibility of
nit: WebContentsImpl

https://codereview.chromium.org/2890143003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_event_handler.cc
(right):

https://codereview.chromium.org/2890143003/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_event_handler.cc:39:
#include "content/browser/renderer_host/render_widget_host_view_aura.h"
It doesn't look like this is needed.

https://codereview.chromium.org/2890143003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2890143003/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:739: void
RenderWidgetHostViewMac::SetShowingContextMenu(bool showing) {
Is this change intentional? It looks like you just moved this method
implementation within the file.

[EhsanK, 2017-06-05 21:40:11]

Patchset #8 (id:140001) has been deleted

[EhsanK, 2017-06-05 21:47:24]

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

[commit-bot: I haz the power, 2017-06-05 21:47:57]

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, 2017-06-06 00:05:03]

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

[commit-bot: I haz the power, 2017-06-06 00:05:04]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2017-06-06 12:34:45]

ekaramad@chromium.org changed reviewers:
+ creis@chromium.org

[EhsanK, 2017-06-06 12:34:47]

Thanks Ken!

Adding creis@ for content/ changes.

Charlie, PTAL.

https://codereview.chromium.org/2890143003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2890143003/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:397: // Called by
WebContensImpl to notify the view about a change in visibility of
On 2017/06/05 17:39:16, kenrb wrote:
> nit: WebContentsImpl

Done.

https://codereview.chromium.org/2890143003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_event_handler.cc
(right):

https://codereview.chromium.org/2890143003/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_event_handler.cc:39:
#include "content/browser/renderer_host/render_widget_host_view_aura.h"
On 2017/06/05 17:39:16, kenrb wrote:
> It doesn't look like this is needed.

Uh thanks for finding this. Left over from WIP phase.

https://codereview.chromium.org/2890143003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2890143003/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:739: void
RenderWidgetHostViewMac::SetShowingContextMenu(bool showing) {
On 2017/06/05 17:39:16, kenrb wrote:
> Is this change intentional? It looks like you just moved this method
> implementation within the file.

Thanks! Moved it back where it used to be. The only change is removing:
RWHVBase::SetShowingContextMenu(showing);

[EhsanK, 2017-06-06 12:36:33]

ekaramad@chromium.org changed reviewers:
+ avi@chromium.org

[EhsanK, 2017-06-06 12:36:34]

avi@chromium.org: Please review changes in chrome/ components/ content/.

Thanks!

[Avi (use Gerrit), 2017-06-06 14:18:31]

I do like the bit living on WebContents.

https://codereview.chromium.org/2890143003/diff/170014/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2890143003/diff/170014/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4528: showing);
This concerns me. The function |SetShowingContextMenu| on
|RenderWidgetHostViewBase| is marked as "Called by WebContentsImpl..." with no
indication that it's called only on certain platforms. If we really only want to
call it on the Mac, then #if MACOSX it, otherwise remove the #if here. Let's
have those two match.

[EhsanK, 2017-06-06 14:40:09]

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

[commit-bot: I haz the power, 2017-06-06 14:40:25]

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, 2017-06-06 15:51:58]

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

[commit-bot: I haz the power, 2017-06-06 15:51:59]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2017-06-06 16:17:38]

Thanks Avi! PTAL.

https://codereview.chromium.org/2890143003/diff/170014/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2890143003/diff/170014/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4528: showing);
On 2017/06/06 14:18:30, Avi (ping after 24h) wrote:
> This concerns me. The function |SetShowingContextMenu| on
> |RenderWidgetHostViewBase| is marked as "Called by WebContentsImpl..." with no
> indication that it's called only on certain platforms. If we really only want
to
> call it on the Mac, then #if MACOSX it, otherwise remove the #if here. Let's
> have those two match.

Thanks! I agree the mismatch does not look nice. But here is the problem I have:
inside RWHVBase we do not have any platform specific code (I guess we should
not). We also do not refer to any RWHVX's here in WebContentsImpl. So the only
option seems to be just to call this on all RWHVBase's which is a NOOP except
for Mac.

[Avi (use Gerrit), 2017-06-06 16:21:14]

On 2017/06/06 16:17:38, EhsanK wrote:
> Thanks Avi! PTAL.
> 
>
https://codereview.chromium.org/2890143003/diff/170014/content/browser/web_co...
> File content/browser/web_contents/web_contents_impl.cc (right):
> 
>
https://codereview.chromium.org/2890143003/diff/170014/content/browser/web_co...
> content/browser/web_contents/web_contents_impl.cc:4528: showing);
> On 2017/06/06 14:18:30, Avi (ping after 24h) wrote:
> > This concerns me. The function |SetShowingContextMenu| on
> > |RenderWidgetHostViewBase| is marked as "Called by WebContentsImpl..." with
no
> > indication that it's called only on certain platforms. If we really only
want
> to
> > call it on the Mac, then #if MACOSX it, otherwise remove the #if here. Let's
> > have those two match.
> 
> Thanks! I agree the mismatch does not look nice. But here is the problem I
have:
> inside RWHVBase we do not have any platform specific code (I guess we should
> not). We also do not refer to any RWHVX's here in WebContentsImpl. So the only
> option seems to be just to call this on all RWHVBase's which is a NOOP except
> for Mac.

LGTM with that change. It's less surprising that way.

[EhsanK, 2017-06-06 18:24:07]

Thanks Avi! I will try to CQ soon.

[EhsanK, 2017-06-06 18:24:25]

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

[EhsanK, 2017-06-06 18:24:27]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2017-06-06 18:24:30]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2017-06-06 18:24:31]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2017-06-06 18:24:31]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org
Link to the patchset: https://codereview.chromium.org/2890143003/#ps190001
(title: "Remove #if def")

[commit-bot: I haz the power, 2017-06-06 18:25:04]

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, 2017-06-06 18:29:45]

CQ is committing da patch.

Bot data: {"patchset_id": 190001, "attempt_start_ts": 1496773471289270,
"parent_rev": "9837cd2419f7d43f2bb34b83d1434353efd01c39", "commit_rev":
"f6750aae23ef4c4fd8e500f092bff269ca277abc"}

[commit-bot: I haz the power, 2017-06-06 18:29:59]

Description was changed from

==========
Move ContextMenu show/hide state tracking to WebContents

Currently, RenderWidgetHostView of the page tracks the visibility state of
context
menus. However, the RWHV might go away during navigations (cross origin). On the
other hand, a context menu might outlive the RWHV and stay visible during the
page
navigation which leads to a mismatch between the stored visibility state in the
new RWHV and the actual visibility of the context menu.

This CL will move this state tracking logic to WebContents which stays the same
during page navigations.

BUG=708182
==========

to

==========
Move ContextMenu show/hide state tracking to WebContents

Currently, RenderWidgetHostView of the page tracks the visibility state of
context
menus. However, the RWHV might go away during navigations (cross origin). On the
other hand, a context menu might outlive the RWHV and stay visible during the
page
navigation which leads to a mismatch between the stored visibility state in the
new RWHV and the actual visibility of the context menu.

This CL will move this state tracking logic to WebContents which stays the same
during page navigations.

BUG=708182

Review-Url: https://codereview.chromium.org/2890143003
Cr-Commit-Position: refs/heads/master@{#477346}
Committed:
https://chromium.googlesource.com/chromium/src/+/f6750aae23ef4c4fd8e500f092bf...
==========

[commit-bot: I haz the power, 2017-06-06 18:30:00]

Committed patchset #10 (id:190001) as
https://chromium.googlesource.com/chromium/src/+/f6750aae23ef4c4fd8e500f092bf...