Fix 3474: Context menu issues when page is zoomed....

Fix 3474: Context menu issues when page is zoomed.

When a page is zoomed the context menu would appear in the wrong
position and somtimes display the wrong menu type. 

Cause of issue is due to lack of transformation to account
for the page zoom.

Added a transformation of the context menu to resolve this issue.

BUG=3474
(http://crbug.com/3474)

[jroesslein, 2008-12-21 22:06:10]

Please commit this change once it looks okay.
Thanks.

[brettw, 2008-12-22 17:09:21]

http://codereview.chromium.org/16204/diff/202/4
File webkit/glue/context_menu_client_impl.cc (right):

http://codereview.chromium.org/16204/diff/202/4#newcode142
Line 142: WebCore::Frame* selected_frame =
r.innerNonSharedNode()->document()->frame();
The un-adjusted hit test result is used several times before you adjust it for
the zoom factor. Are you sure this is correct?

[jroesslein, 2008-12-22 18:03:45]

I have tested this and it does work fine here.
Are you suggesting it be moved into ContextMenuController or
just move it up before line 142? 

On 2008/12/22 17:09:21, brettw wrote:
> http://codereview.chromium.org/16204/diff/202/4
> File webkit/glue/context_menu_client_impl.cc (right):
> 
> http://codereview.chromium.org/16204/diff/202/4#newcode142
> Line 142: WebCore::Frame* selected_frame =
> r.innerNonSharedNode()->document()->frame();
> The un-adjusted hit test result is used several times before you adjust it for
> the zoom factor. Are you sure this is correct?

[brettw, 2008-12-22 18:24:37]

On 2008/12/22 18:03:45, jroesslein wrote:
> I have tested this and it does work fine here.

Are you saying that you think the code looks right and you're satisfied with
your answer? This seems so obviously questionable that we need to understand
what's going on. However...

> Are you suggesting it be moved into ContextMenuController

this does seem like something WebKit should be doing, and not something we
should have to patch up in the glue layer. The question is: does Safari with
full page zoom have the same bug? http://webkit.org/blog/165/full-page-zoom/
If so, we should fix WebCore, if not, we should make our glue match whatever
Safari is doing to get the right answer.

[jroesslein, 2008-12-22 18:30:50]

I downloaded and installed the latest safari and it doesn't have this problem.
So I'm guessing they are doing to same thing in their glue code.

On 2008/12/22 18:24:37, brettw wrote:
> On 2008/12/22 18:03:45, jroesslein wrote:
> > I have tested this and it does work fine here.
> 
> Are you saying that you think the code looks right and you're satisfied with
> your answer? This seems so obviously questionable that we need to understand
> what's going on. However...
> 
> > Are you suggesting it be moved into ContextMenuController
> 
> this does seem like something WebKit should be doing, and not something we
> should have to patch up in the glue layer. The question is: does Safari with
> full page zoom have the same bug? http://webkit.org/blog/165/full-page-zoom/
> If so, we should fix WebCore, if not, we should make our glue match whatever
> Safari is doing to get the right answer.

[jroesslein, 2008-12-22 19:11:35]

Ok running safari with the nightly build of webkit does have this issue. So it
seems this fix should be moved into ContextMenuController and upstreamed to
webkit. I'll work on putting this together.
On 2008/12/22 18:30:50, jroesslein wrote:
> I downloaded and installed the latest safari and it doesn't have this problem.
> So I'm guessing they are doing to same thing in their glue code.
> 
> On 2008/12/22 18:24:37, brettw wrote:
> > On 2008/12/22 18:03:45, jroesslein wrote:
> > > I have tested this and it does work fine here.
> > 
> > Are you saying that you think the code looks right and you're satisfied with
> > your answer? This seems so obviously questionable that we need to understand
> > what's going on. However...
> > 
> > > Are you suggesting it be moved into ContextMenuController
> > 
> > this does seem like something WebKit should be doing, and not something we
> > should have to patch up in the glue layer. The question is: does Safari with
> > full page zoom have the same bug? http://webkit.org/blog/165/full-page-zoom/
> > If so, we should fix WebCore, if not, we should make our glue match whatever
> > Safari is doing to get the right answer.

[jroesslein, 2008-12-23 06:46:09]

I've filed a bug report for this and also the patch to fix it. Not sure how long
it will take to get reviewed and landed.

Webkit Bug 22968 (https://bugs.webkit.org/show_bug.cgi?id=22968)

[jroesslein, 2009-01-02 05:58:42]

Closed issue. Has been resolved.