[webkit-reviews] review denied: [Bug 26422] [Qt] QWebPagePrivate::frameAt calculates wrong frame : [Attachment 31505] Fix QWebPagePrivate::frameAt by adding up the frame positions.

[bugzilla-daemon at webkit.org]

Holger Freyther <zecke at selfish.org> has denied Joseph Ligman
<joseph.ligman at nokia.com>'s request for review:
Bug 26422: [Qt] QWebPagePrivate::frameAt calculates wrong frame
https://bugs.webkit.org/show_bug.cgi?id=26422

Attachment 31505: Fix QWebPagePrivate::frameAt by adding up the frame
positions.
https://bugs.webkit.org/attachment.cgi?id=31505&action=review

------- Additional Comments from Holger Freyther <zecke at selfish.org>
> Index: WebKit/qt/ChangeLog
> ===================================================================
> --- WebKit/qt/ChangeLog	(revision 44815)
> +++ WebKit/qt/ChangeLog	(working copy)
> @@ -1,3 +1,19 @@
> +2009-06-18  Joe Ligman  <joseph.ligman at nokia.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Bug 26422: [Qt] QWebPagePrivate::frameAt calculates wrong frame
> +	   Add the parent frame's geometric position and subtract the parent
frame's scroll position.

Great.


>  {
>      QWebFrame *frame = mainFrame;
> +    if (!frame->geometry().contains(pos))
> +	   return 0;
>  
> +    QPoint absoluteFramePosition = QPoint(frame->pos() -
frame->scrollPosition());
>  redo:
>      QList<QWebFrame*> children = frame->childFrames();
>      for (int i = 0; i < children.size(); ++i) {
> -	   if (children.at(i)->geometry().contains(pos)) {
> +	   QRect frameRect(children.at(i)->pos() + absoluteFramePosition,
children.at(i)->geometry().size());
> +	   if (frameRect.contains(pos)) {
> +	       absoluteFramePosition = QPoint(frameRect.topLeft() -
children.at(i)->scrollPosition());
>	       frame = children.at(i);
>	       goto redo;
>	   }
>      }
> -    if (frame->geometry().contains(pos))
> -	   return frame;
> -    return 0;
> +    return frame;
>  }


Any reason to not just use a HitTest here? But from what I understand this
looks okay.



>  
> +void tst_QWebPage::rightclickContextMenu()
> +{
> +    QWebView view;
> +    QWebPage *page = view.page();
> +    QSignalSpy loadSpy(page, SIGNAL(loadFinished(bool)));
> +    QUrl url = QUrl("qrc:///frametest/iframe.html");
> +    page->mainFrame()->load(url);
> +    QTRY_COMPARE(loadSpy.count(), 1);
> +    QWebFrame *frame = page->mainFrame();
> +    QRect frameRect = frame->geometry();
> +    for (int i = 0; i < 2; i++) {
> +	   QList<QWebFrame*> children = frame->childFrames(); 
> +	   if (children.size() > 0) {
> +	       frame = children.at(0);
> +	       frameRect = QRect(frame->geometry().topLeft() +
frameRect.topLeft(), frame->geometry().size());
> +	       QContextMenuEvent event(QContextMenuEvent::Mouse,
> +				       QPoint(frameRect.right() - 5,
frameRect.center().y()),
> +				       QPoint(frameRect.right() - 5,
frameRect.center().y()),
> +				       Qt::NoModifier);
> +	       page->swallowContextMenuEvent(&event);
> +	   }
> +    }
>

Okay, I r=- because this test case is not complete. Your test case should have
at least the following characteristics... Fail now, after you patch succeed. I
don't see any kind of verification in the above code snippet... maybe I'm just
too blind.