[webkit-reviews] review denied: [Bug 37163] [Qt] inputMethodQuery returns coordinates in web page coordinates rather than in item coordinates. : [Attachment 53703] Patch round 2

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Apr 19 16:33:47 PDT 2010


Simon Hausmann <hausmann at webkit.org> has denied John Pavan
<john.pavan at nokia.com>'s request for review:
Bug 37163: [Qt] inputMethodQuery returns coordinates in web page coordinates
rather than in item coordinates.
https://bugs.webkit.org/show_bug.cgi?id=37163

Attachment 53703: Patch round 2
https://bugs.webkit.org/attachment.cgi?id=53703&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>

Good observation with the bug and good that you made a unit test!

I don't agree about your approach though. I think the scroll position of the
frame should be taken into account
right in QWebPage::inputMethodQuery, so that it works for QWebView and
QGraphicsWebView. The code also becomes
a lot simpler.

Joe, Janne: What do you think?


> ===================================================================
> --- WebKit/qt/ChangeLog	(revision 57608)
> +++ WebKit/qt/ChangeLog	(working copy)
> @@ -1,3 +1,17 @@
> +2010-04-14  John Pavan  <john.pavan at nokia.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   inputMethodQuery returns coordinates in web page coordinates rather
than in item coordinates.

Please put a newline between the title of the bug and the description of the
fix.
It's also good practice to prefix the title with [Qt] if it's a qt specific
issue.


> +    // john.pavan at nokia.com:
> +    // in cases of coordinates the QGraphicsWebView (as a QGraphicsWidget)
> +    // should be returning coordinates in the reference frame of the
> +    // item (qgraphicswebview), not the coordinates on the web page.
> +    // To do this we need the viewport.

You don't have to put your email address as a comment into the
code. svn annotate / git blame will find out :-)


More information about the webkit-reviews mailing list