Add the screen offset to the location of the flash context menu.

Add the screen offset to the location of the flash context menu.

This fixes a bug where the context menu would display on the wrong monitor if the plugin was in full screen and not on the primary monitor. The coordinates to |ShowContextMenu| are expected to be relative to the window position. However, the coordinates given by |windowRect| includes the screen offset so we need to correct for that.

BUG=139641
TEST=Tested context menu on win flapper on both the primary and secondary monitors. Mac context menus use the mouse coordinates. Linux fullscreen for multiple monitors is currently broken.
TBR=brettw
NOTRY=true


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150155

[viettrungluu, 2012-08-05 05:55:16]

https://chromiumcodereview.appspot.com/10837117/diff/1/content/renderer/peppe...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/10837117/diff/1/content/renderer/peppe...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1282: WebKit::WebRect
window_rect = render_view_->windowRect();
Are you sure this doesn't break multi-monitor fullscreen for Pepper, non-Flash
fullscreen?

This change feels rather wrong. At the same time, the existing coordinate
transformation feels rather wrong: The render view is interpreting the
coordinates relative to the normal, non-fullscreen window instead of the render
widget's (which is actually in |params| -- so the render view actually knows
which render widget is being targeted).

That said, I'm too lazy to go read code on the renderer host side, and if this
doesn't break anything, it'll do for now.

[raymes, 2012-08-05 06:19:57]

https://chromiumcodereview.appspot.com/10837117/diff/1/content/renderer/peppe...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/10837117/diff/1/content/renderer/peppe...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1282: WebKit::WebRect
window_rect = render_view_->windowRect();
On 2012/08/05 05:55:16, viettrungluu wrote:
> Are you sure this doesn't break multi-monitor fullscreen for Pepper, non-Flash
> fullscreen?

AFAICT this isn't currently used by anything other than flash (the only use I
see is from
http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/plugins/ppapi/ppb_f...
).  If there is some other non-flash usage, I'm happy to test. I'm not sure
whether it /would/ break non-flash plugins if it were used. 

> 
> This change feels rather wrong. At the same time, the existing coordinate
> transformation feels rather wrong: The render view is interpreting the
> coordinates relative to the normal, non-fullscreen window instead of the
render
> widget's (which is actually in |params| -- so the render view actually knows
> which render widget is being targeted).
> 
> That said, I'm too lazy to go read code on the renderer host side, and if this
> doesn't break anything, it'll do for now.

I agree, but I wasn't sure if this was intended/expected behavior since the
existing code intentionally did the transformation to the windows coordinate
space. I can file a bug to investigate.

[viettrungluu, 2012-08-06 17:33:59]

lgtm

[commit-bot: I haz the power, 2012-08-06 17:46:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/10837117/1

[commit-bot: I haz the power, 2012-08-06 17:46:55]

Presubmit check for 10837117-1 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/renderer

Presubmit checks took 1.2s to calculate.

[raymes, 2012-08-06 17:55:51]

+brettw for OWNERS

[commit-bot: I haz the power, 2012-08-06 19:15:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/10837117/1

[commit-bot: I haz the power, 2012-08-06 19:44:50]

Try job failure for 10837117-1 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-06 19:49:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/10837117/1

[commit-bot: I haz the power, 2012-08-06 20:24:18]

Try job failure for 10837117-1 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-06 20:42:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/10837117/1

[commit-bot: I haz the power, 2012-08-06 21:50:08]

Change committed as 150155