Fixing the Position of Context Menu for BrowserPlugin (<webview>)

Fixing the Position of Context Menu for BrowserPlugin (<webview>)

With current implementation of the BrowserPlugin, all WebMouseEvents are
routed, throught the browser (RenderWidgetHostViewGuest), to the
renderer process of the guest. The guest uses the coordinates, as passed, for creating the ContextMenu event.
Therefore, the guest renderer process is oblivious to any CSS transforms
applied on the BrowserPlugin and as a result, the reported position of
the context menu is potentially incorrect.

This patch applies a hacky solution to resolve this issue. The work
around is to store the location of the context menu from a MouseDown
event. The position is measured form the global coordinates of the mouse
event and the boundaries of the owner's view.

BUG=470087
Committed: https://crrev.com/b205e53f3aa9a741f4dffa5726992976c517f6ee
Cr-Commit-Position: refs/heads/master@{#347195}

[Ehsaan, 2015-08-15 05:00:09]

ekaramad@google.com changed reviewers:
+ fsamuel@chromium.org, lazyboy@chromium.org, paulmeyer@chromium.org,
wjmaclean@chromium.org

[Ehsaan, 2015-08-15 05:00:10]

paulmeyer@chromium.org: Please review changes in 

wjmaclean@chromium.org: Please review changes in 

lazyboy@chromium.org: Please review changes in 

fsamuel@chromium.org: Please review changes in

[wjmaclean, 2015-08-17 13:05:23]

https://codereview.chromium.org/1293963002/diff/1/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/1293963002/diff/1/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.h:446: gfx::Point
context_menu_position_;
Why do we have this? It doesn't seem to be used (or did I miss something?)

https://codereview.chromium.org/1293963002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/1293963002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_widget_host_view_guest.cc:630: // ContextMenu
from the guest renderer become potentially incorrect. As a
-> "... renderer to potentially be incorrect."

https://codereview.chromium.org/1293963002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_widget_host_view_guest.cc:639:
guest_->SetContextMenuPosition(
Will the transformation ever be more than just a translation? Will scale and/or
rotation ever be an issue?

[Ehsaan, 2015-08-17 14:09:13]

https://codereview.chromium.org/1293963002/diff/1/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/1293963002/diff/1/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.h:446: gfx::Point
context_menu_position_;
My mistake! I was initially setting this value here, but then moved it to the
ChromeWebViewGuestDelegate. I will remove this.

https://codereview.chromium.org/1293963002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/1293963002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_widget_host_view_guest.cc:630: // ContextMenu
from the guest renderer become potentially incorrect. As a
On 2015/08/17 13:05:23, wjmaclean wrote:
> -> "... renderer to potentially be incorrect."

Done.

https://codereview.chromium.org/1293963002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_widget_host_view_guest.cc:639:
guest_->SetContextMenuPosition(
On 2015/08/17 13:05:23, wjmaclean wrote:
> Will the transformation ever be more than just a translation? Will scale
and/or
> rotation ever be an issue?

This translation is based on globalX/Y which is supposed to be untouched, plus
the corner point of the view. This should become the value where the context
menu was supposed to be shown. I am doing this on the browser side to avoid the
CSS transformations. The way it normally work is for the renderer to undo the
transforms is here (I guess):
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Which is not done (and should not be I believe) in the case of BrowserPlugin.
This even is supposed to be carried to the guest renderer and changing the
coordinates at the renderer side will break functionality.

[wjmaclean, 2015-08-18 14:33:40]

On 2015/08/17 14:09:13, Ehsaan wrote:
>
https://codereview.chromium.org/1293963002/diff/1/content/browser/browser_plu...
> File content/browser/browser_plugin/browser_plugin_guest.h (right):
> 
>
https://codereview.chromium.org/1293963002/diff/1/content/browser/browser_plu...
> content/browser/browser_plugin/browser_plugin_guest.h:446: gfx::Point
> context_menu_position_;
> My mistake! I was initially setting this value here, but then moved it to the
> ChromeWebViewGuestDelegate. I will remove this.
> 
>
https://codereview.chromium.org/1293963002/diff/1/content/browser/frame_host/...
> File content/browser/frame_host/render_widget_host_view_guest.cc (right):
> 
>
https://codereview.chromium.org/1293963002/diff/1/content/browser/frame_host/...
> content/browser/frame_host/render_widget_host_view_guest.cc:630: //
ContextMenu
> from the guest renderer become potentially incorrect. As a
> On 2015/08/17 13:05:23, wjmaclean wrote:
> > -> "... renderer to potentially be incorrect."
> 
> Done.
> 
>
https://codereview.chromium.org/1293963002/diff/1/content/browser/frame_host/...
> content/browser/frame_host/render_widget_host_view_guest.cc:639:
> guest_->SetContextMenuPosition(
> On 2015/08/17 13:05:23, wjmaclean wrote:
> > Will the transformation ever be more than just a translation? Will scale
> and/or
> > rotation ever be an issue?
> 
> This translation is based on globalX/Y which is supposed to be untouched, plus
> the corner point of the view. This should become the value where the context
> menu was supposed to be shown. I am doing this on the browser side to avoid
the
> CSS transformations. The way it normally work is for the renderer to undo the
> transforms is here (I guess):
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Which is not done (and should not be I believe) in the case of BrowserPlugin.
> This even is supposed to be carried to the guest renderer and changing the
> coordinates at the renderer side will break functionality.

I'm ok with the change as it currently stands; it might be nice to add a comment
about the class of transformations it can handle, and why we expect this to be
sufficient.

lgtm

[Ehsaan, 2015-08-25 16:36:21]

Thanks wjmaclean@!

@lazyboy|paulmeyer|fsamuel:
PTAL.

[Ehsaan, 2015-08-25 16:37:14]

ekaramad@google.com changed reviewers:
+ sievers@chromium.org

[Ehsaan, 2015-08-25 16:37:14]

sievers@chromium.org: Please review changes in

[Ehsaan, 2015-08-25 16:37:58]

On 2015/08/25 16:37:14, Ehsaan wrote:
> mailto:sievers@chromium.org: Please review changes in

+sievers@ for the missing permissions "RenderWidgetHostViewGuest". Thanks!

[no sievers, 2015-08-25 22:06:54]

On 2015/08/25 16:37:58, Ehsaan wrote:
> On 2015/08/25 16:37:14, Ehsaan wrote:
> > mailto:sievers@chromium.org: Please review changes in
> 
> +sievers@ for the missing permissions "RenderWidgetHostViewGuest". Thanks!

lgtm. since it's a hack (and it says that too in the code), should it have a
TODO to rethink this more broadly?

[Ehsaan, 2015-08-25 22:15:38]

On 2015/08/25 22:06:54, sievers wrote:
> On 2015/08/25 16:37:58, Ehsaan wrote:
> > On 2015/08/25 16:37:14, Ehsaan wrote:
> > > mailto:sievers@chromium.org: Please review changes in
> > 
> > +sievers@ for the missing permissions "RenderWidgetHostViewGuest". Thanks!
> 
> lgtm. since it's a hack (and it says that too in the code), should it have a
> TODO to rethink this more broadly?

Thanks!.

[Ehsaan, 2015-08-25 22:15:46]

PTAL.

[no sievers, 2015-08-25 22:36:07]

On 2015/08/25 22:15:46, Ehsaan wrote:
> PTAL.

lgtm, though a bug id would be even better :)

[no sievers, 2015-08-25 22:36:08]

On 2015/08/25 22:15:46, Ehsaan wrote:
> PTAL.

lgtm, though a bug id would be even better :)

[Ehsaan, 2015-08-26 14:48:14]

On 2015/08/25 22:36:08, sievers wrote:
> On 2015/08/25 22:15:46, Ehsaan wrote:
> > PTAL.
> 
> lgtm, though a bug id would be even better :)

Thanks! Added a (BUG=ID) for both comments on render_widget_...cc and
browser_plugin_guest.h.

[Ehsaan, 2015-08-26 14:48:30]

PTAL.

[no sievers, 2015-08-26 18:39:14]

still lgtm

[Ehsaan, 2015-09-01 19:13:42]

ekaramad@google.com changed reviewers:
- fsamuel@chromium.org, paulmeyer@chromium.org, sievers@chromium.org,
wjmaclean@chromium.org

[Ehsaan, 2015-09-01 19:13:43]

PTAL.
+lazyboy for the missing permissions.

[lazyboy, 2015-09-01 19:45:47]

Some comments for the test, mostly nits, thanks for the test!

https://codereview.chromium.org/1293963002/diff/120001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right):

https://codereview.chromium.org/1293963002/diff/120001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:292:
mouse_event.globalX = x + rwh->GetView()->GetViewBounds().x();
You want to use rect.x() and rect.y() here.

https://codereview.chromium.org/1293963002/diff/120001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:863: if
(embedder_web_contents_ == nullptr)
nit: if (!embedder_web_contents_)

https://codereview.chromium.org/1293963002/diff/120001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:874: //
RenderWidgetHostViewGuest for the embedder so that the
embedder shouldn't have RWHVGuest, you mean just RenderWidgetHostView?
(RWHVAura/Mac).

https://codereview.chromium.org/1293963002/diff/120001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:882:
scoped_ptr<ExtensionTestMessageListener> transform_set_listener(
scoped_ptr seems unnecessary here, you can just use a local variable.

https://codereview.chromium.org/1293963002/diff/120001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:891: int
windowX = 150, windowY = 150;
nit: window_x, window_y, better set it as point gfx::Point window_point(150,
150); then you can do a more readable construct:

window_point += guest_view_bounds.origin() - embedder_view_bounds.origin();

https://codereview.chromium.org/1293963002/diff/120001/chrome/test/data/exten...
File
chrome/test/data/extensions/platform_apps/web_view/context_menus/coordinates_with_transforms/embedder.js
(right):

https://codereview.chromium.org/1293963002/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/web_view/context_menus/coordinates_with_transforms/embedder.js:12:
webview.style.transform = (tr !== "NONE") ? tr : "";
nit: 'NONE'

https://codereview.chromium.org/1293963002/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/web_view/context_menus/coordinates_with_transforms/embedder.js:17:
webview.setAttribute('src', url);
Instead of reading the url from .src and setting, you probably just want
webview.reload()

[Ehsaan, 2015-09-02 15:18:20]

PTAL.

https://codereview.chromium.org/1293963002/diff/120001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right):

https://codereview.chromium.org/1293963002/diff/120001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:292:
mouse_event.globalX = x + rwh->GetView()->GetViewBounds().x();
On 2015/09/01 19:45:46, lazyboy wrote:
> You want to use rect.x() and rect.y() here.

Done.

https://codereview.chromium.org/1293963002/diff/120001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:863: if
(embedder_web_contents_ == nullptr)
On 2015/09/01 19:45:47, lazyboy wrote:
> nit: if (!embedder_web_contents_)

Done.

https://codereview.chromium.org/1293963002/diff/120001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:874: //
RenderWidgetHostViewGuest for the embedder so that the
On 2015/09/01 19:45:47, lazyboy wrote:
> embedder shouldn't have RWHVGuest, you mean just RenderWidgetHostView?
> (RWHVAura/Mac).

I mean, the View which takes BrowserPlugin's IPC for events. That is the RWHVG
which then forwards the event back to the guest.

https://codereview.chromium.org/1293963002/diff/120001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:882:
scoped_ptr<ExtensionTestMessageListener> transform_set_listener(
On 2015/09/01 19:45:47, lazyboy wrote:
> scoped_ptr seems unnecessary here, you can just use a local variable.

Done.

https://codereview.chromium.org/1293963002/diff/120001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:891: int
windowX = 150, windowY = 150;
On 2015/09/01 19:45:46, lazyboy wrote:
> nit: window_x, window_y, better set it as point gfx::Point window_point(150,
> 150); then you can do a more readable construct:
> 
> window_point += guest_view_bounds.origin() - embedder_view_bounds.origin();

Done.

https://codereview.chromium.org/1293963002/diff/120001/chrome/test/data/exten...
File
chrome/test/data/extensions/platform_apps/web_view/context_menus/coordinates_with_transforms/embedder.js
(right):

https://codereview.chromium.org/1293963002/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/web_view/context_menus/coordinates_with_transforms/embedder.js:12:
webview.style.transform = (tr !== "NONE") ? tr : "";
On 2015/09/01 19:45:47, lazyboy wrote:
> nit: 'NONE'

Done.

https://codereview.chromium.org/1293963002/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/web_view/context_menus/coordinates_with_transforms/embedder.js:17:
webview.setAttribute('src', url);
On 2015/09/01 19:45:47, lazyboy wrote:
> Instead of reading the url from .src and setting, you probably just want
> webview.reload()

Done.

[lazyboy, 2015-09-02 15:33:40]

lgtm

[EhsanK, 2015-09-02 15:34:55]

ekaramad@chromium.org changed reviewers:
+ ekaramad@chromium.org, fsamuel@chromium.org

[EhsanK, 2015-09-02 15:34:56]

+fsamuel for missing owner's lgtm.

PTAL.

[Fady Samuel, 2015-09-02 15:59:39]

lgtm

[EhsanK, 2015-09-02 16:01:19]

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

[commit-bot: I haz the power, 2015-09-02 16:01:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1293963002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1293963002/140001

[commit-bot: I haz the power, 2015-09-02 16:03:36]

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

[commit-bot: I haz the power, 2015-09-02 16:03:37]

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

[Ehsaan, 2015-09-02 20:27:44]

The CQ bit was checked by ekaramad@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-02 20:28:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1293963002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1293963002/160001

[commit-bot: I haz the power, 2015-09-02 20:50:50]

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

[commit-bot: I haz the power, 2015-09-02 20:50:51]

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

[Ehsaan, 2015-09-03 14:23:52]

The CQ bit was checked by ekaramad@google.com

[Ehsaan, 2015-09-03 14:23:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
wjmaclean@chromium.org, sievers@chromium.org, fsamuel@chromium.org,
lazyboy@chromium.org, ekaramad@chromium.org
Link to the patchset: https://codereview.chromium.org/1293963002/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2015-09-03 14:23:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1293963002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1293963002/160001

[commit-bot: I haz the power, 2015-09-03 14:44:35]

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

[commit-bot: I haz the power, 2015-09-03 14:44:36]

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

[Ehsaan, 2015-09-03 15:56:59]

The CQ bit was checked by ekaramad@google.com

[Ehsaan, 2015-09-03 15:57:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, wjmaclean@chromium.org, lazyboy@chromium.org,
sievers@chromium.org, ekaramad@chromium.org
Link to the patchset: https://codereview.chromium.org/1293963002/#ps180001
(title: "Fixed a Build Error in MAC")

[commit-bot: I haz the power, 2015-09-03 15:57:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1293963002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1293963002/180001

[commit-bot: I haz the power, 2015-09-03 18:26:11]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-09-03 18:26:46]

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