On Mac: Hide any popup menus on the active view before switching to another.

On Mac: Hide any popup menus on the active view before switching to another.

This to prevent click-jacking using an option popup list that becomes persistent.

BUG=448008
Committed: https://crrev.com/a20c13f307387b7c250d45cbc9dc4286ac1435ee
Cr-Commit-Position: refs/heads/master@{#311500}

[haraldh, 2015-01-06 14:47:29]

haraldh@opera.com changed reviewers:
+ avi@chromium.org, nasko@chromium.org

[haraldh, 2015-01-06 15:22:20]

@nasko, @Avi, added you as reviewers based recent activity in this file.

[Avi (use Gerrit), 2015-01-06 15:46:39]

https://codereview.chromium.org/839573002/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/839573002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:1080: #endif
Android also uses external popup menus; do we want to do this for them?

Also, I'm very suspicious of this code being in this file.
ShowPopupMenu/HidePopupMenu calls are made in RenderFrameHostImpl. Why should
this code go here?

[Avi (use Gerrit), 2015-01-06 16:08:39]

On 2015/01/06 15:46:39, Avi wrote:
>
https://codereview.chromium.org/839573002/diff/1/content/browser/web_contents...
> File content/browser/web_contents/web_contents_impl.cc (right):
> 
>
https://codereview.chromium.org/839573002/diff/1/content/browser/web_contents...
> content/browser/web_contents/web_contents_impl.cc:1080: #endif
> Android also uses external popup menus; do we want to do this for them?
> 
> Also, I'm very suspicious of this code being in this file.
> ShowPopupMenu/HidePopupMenu calls are made in RenderFrameHostImpl. Why should
> this code go here?

Also, you need to link a BUG=.

[haraldh, 2015-01-06 16:19:10]

On 2015/01/06 15:46:39, Avi wrote:
>
https://codereview.chromium.org/839573002/diff/1/content/browser/web_contents...
> File content/browser/web_contents/web_contents_impl.cc (right):
> 
>
https://codereview.chromium.org/839573002/diff/1/content/browser/web_contents...
> content/browser/web_contents/web_contents_impl.cc:1080: #endif
> Android also uses external popup menus; do we want to do this for them?

Android is not affected by the potential click-jacking I believe.

> 
> Also, I'm very suspicious of this code being in this file.
> ShowPopupMenu/HidePopupMenu calls are made in RenderFrameHostImpl. Why should
> this code go here?

The location is based on the patch https://codereview.chromium.org/228333002/
that did similar things, but was eventually dropped when it was not necessary
for Windows anymore.

[Avi (use Gerrit), 2015-01-06 17:28:16]

On 2015/01/06 16:19:10, haraldh wrote:
> On 2015/01/06 15:46:39, Avi wrote:
> >
>
https://codereview.chromium.org/839573002/diff/1/content/browser/web_contents...
> > File content/browser/web_contents/web_contents_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/839573002/diff/1/content/browser/web_contents...
> > content/browser/web_contents/web_contents_impl.cc:1080: #endif
> > Android also uses external popup menus; do we want to do this for them?
> 
> Android is not affected by the potential click-jacking I believe.
> 
> > 
> > Also, I'm very suspicious of this code being in this file.
> > ShowPopupMenu/HidePopupMenu calls are made in RenderFrameHostImpl. Why
should
> > this code go here?
> 
> The location is based on the patch https://codereview.chromium.org/228333002/
> that did similar things, but was eventually dropped when it was not necessary
> for Windows anymore.

I'm not thrilled with that change either. My question remains: why does this
belong here, when all the other external popup code lives in
RenderFrameHostImpl?

[haraldh, 2015-01-07 19:21:17]

On 2015/01/06 17:28:16, Avi wrote:
> On 2015/01/06 16:19:10, haraldh wrote:
> > On 2015/01/06 15:46:39, Avi wrote:
> > >
> >
>
https://codereview.chromium.org/839573002/diff/1/content/browser/web_contents...
> > > File content/browser/web_contents/web_contents_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/839573002/diff/1/content/browser/web_contents...
> > > content/browser/web_contents/web_contents_impl.cc:1080: #endif
> > > Android also uses external popup menus; do we want to do this for them?
> > 
> > Android is not affected by the potential click-jacking I believe.
> > 
> > > 
> > > Also, I'm very suspicious of this code being in this file.
> > > ShowPopupMenu/HidePopupMenu calls are made in RenderFrameHostImpl. Why
> should
> > > this code go here?
> > 
> > The location is based on the patch
https://codereview.chromium.org/228333002/
> > that did similar things, but was eventually dropped when it was not
necessary
> > for Windows anymore.
> 
> I'm not thrilled with that change either. My question remains: why does this
> belong here, when all the other external popup code lives in
> RenderFrameHostImpl?

Well, it needs to trigger when the web contents changes (goes away in this
case). Have not seen anywhere in RenderFrameHostImpl where it would fit.

[Avi (use Gerrit), 2015-01-07 20:45:39]

Put this in in the PopupMenuHelper directly. In its constructor, it already
listens for NOTIFICATION_RENDER_WIDGET_HOST_DESTROYED. Make it listen to
NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED and have it close itself.

Have you verified that it is safe for HidePopupMenu to be called multiple times?
(This close + the existing close message.)

Again, you need a BUG=.

[haraldh, 2015-01-08 08:16:41]

On 2015/01/07 20:45:39, Avi wrote:
> Put this in in the PopupMenuHelper directly. In its constructor, it already
> listens for NOTIFICATION_RENDER_WIDGET_HOST_DESTROYED. Make it listen to
> NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED and have it close itself.
> 
> Have you verified that it is safe for HidePopupMenu to be called multiple
times?
> (This close + the existing close message.)
> 
> Again, you need a BUG=.

Allright, will look into it next week (probably), I'm down with a virus at the
moment...

[haraldh, 2015-01-13 09:12:48]

On 2015/01/07 20:45:39, Avi wrote:
> Put this in in the PopupMenuHelper directly. In its constructor, it already
> listens for NOTIFICATION_RENDER_WIDGET_HOST_DESTROYED. Make it listen to
> NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED and have it close itself.
> 
> Have you verified that it is safe for HidePopupMenu to be called multiple
times?
> (This close + the existing close message.)
> 
> Again, you need a BUG=.

Bug created (https://code.google.com/p/chromium/issues/detail?id=448008) and fix
rewritten using the suggested notification in PopupMenuHelper.

[Avi (use Gerrit), 2015-01-13 15:35:13]

On 2015/01/13 09:12:48, haraldh wrote:
> On 2015/01/07 20:45:39, Avi wrote:
> > Put this in in the PopupMenuHelper directly. In its constructor, it already
> > listens for NOTIFICATION_RENDER_WIDGET_HOST_DESTROYED. Make it listen to
> > NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED and have it close itself.
> > 
> > Have you verified that it is safe for HidePopupMenu to be called multiple
> times?
> > (This close + the existing close message.)
> > 
> > Again, you need a BUG=.
> 
> Bug created (https://code.google.com/p/chromium/issues/detail?id=448008) and
fix
> rewritten using the suggested notification in PopupMenuHelper.

Have you verified that it behaves properly when it gets multiple hide messages?
If so, LGTM.

[haraldh, 2015-01-13 16:07:40]

On 2015/01/13 15:35:13, Avi wrote:
> On 2015/01/13 09:12:48, haraldh wrote:
> > On 2015/01/07 20:45:39, Avi wrote:
> > > Put this in in the PopupMenuHelper directly. In its constructor, it
already
> > > listens for NOTIFICATION_RENDER_WIDGET_HOST_DESTROYED. Make it listen to
> > > NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED and have it close itself.
> > > 
> > > Have you verified that it is safe for HidePopupMenu to be called multiple
> > times?
> > > (This close + the existing close message.)
> > > 
> > > Again, you need a BUG=.
> > 
> > Bug created (https://code.google.com/p/chromium/issues/detail?id=448008) and
> fix
> > rewritten using the suggested notification in PopupMenuHelper.
> 
> Have you verified that it behaves properly when it gets multiple hide
messages?
> If so, LGTM.

Yes, this has no ill effect.

[haraldh, 2015-01-14 16:35:43]

The CQ bit was checked by haraldh@opera.com

[commit-bot: I haz the power, 2015-01-14 16:36:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/839573002/20001

[commit-bot: I haz the power, 2015-01-14 17:30:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-14 17:31:57]

Patchset 2 (id:??) landed as
https://crrev.com/a20c13f307387b7c250d45cbc9dc4286ac1435ee
Cr-Commit-Position: refs/heads/master@{#311500}