[webkit-reviews] review denied: [Bug 26422] [Qt] QWebPagePrivate::frameAt calculates wrong frame : [Attachment 31870] Added method frameAt to QWebPage

[bugzilla-daemon at webkit.org]

Adam Treat <treat at kde.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 31870: Added method frameAt	to QWebPage
https://bugs.webkit.org/attachment.cgi?id=31870&action=review

------- Additional Comments from Adam Treat <treat at kde.org>
> Index: WebKit/qt/ChangeLog
> ===================================================================
> --- WebKit/qt/ChangeLog	(revision 45179)
> +++ WebKit/qt/ChangeLog	(working copy)
> @@ -1,3 +1,20 @@
> +2009-06-25  Joe Ligman  <joseph.ligman at nokia.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   * Api/qwebpage.cpp:
> +	   (QWebPage::frameAt):
> +	   (QWebPage::swallowContextMenuEvent):
> +	   * Api/qwebpage.h:
> +	   * Api/qwebpage_p.h:
> +	   * tests/qwebpage/frametest/iframe.html: Added.
> +	   * tests/qwebpage/frametest/iframe2.html: Added.
> +	   * tests/qwebpage/frametest/iframe3.html: Added.
> +	   * tests/qwebpage/tst_qwebpage.cpp:
> +	   (frameAtHelper):
> +	   (tst_QWebPage::frameAt):
> +	   * tests/qwebpage/tst_qwebpage.qrc:
> 

You are missing a description in the ChangeLog about what this commit does.  In
theory, you should document everything you've changed in the ChangeLog with
details about every method.  At the least, you need to add some message
indicating the overall thrust of the commit.

> +
> +/*!
> +    Returns the frame at the given point.
> +
> +    \sa mainFrame(), currentFrame()
> +*/
> +QWebFrame *QWebPage::frameAt(const QPoint &pos) const
> +{
> +    QWebFrame *frame = mainFrame();
> +    if (!frame->geometry().contains(pos))
> +	   return 0;
> +    QWebHitTestResult hitTestResult = mainFrame()->hitTestContent(pos);
> +    return hitTestResult.frame();
> +}
> +

Notice that this uses Qt coding style throughout this new method...
 
> @@ -2195,7 +2193,7 @@ bool QWebPage::swallowContextMenuEvent(Q
>  {
>      d->page->contextMenuController()->clearContextMenu();
>  
> -    if (QWebFrame* webFrame = d->frameAt(event->pos())) {
> +    if (QWebFrame* webFrame = frameAt(event->pos())) {
>	   Frame* frame = QWebFramePrivate::core(webFrame);
>	   if (Scrollbar* scrollbar =
frame->view()->scrollbarUnderMouse(PlatformMouseEvent(event, 1))) {
>	       return scrollbar->contextMenu(PlatformMouseEvent(event, 1));

And here we're using WebKit coding style for the body of the method while the
signature of the method continues to use Qt coding style.  All in the same
file!  That is wrong.  It needs to be determined what the proper coding style
is supposed to be for this file and the whole file should then adopt it.  At
the least, new code should adopt it.  I believe Qt coding style should be used
here.  Regardless, we need to make a decision and document it on the wiki so we
can refer to it in the future.

> +void frameAtHelper(QWebPage* page, QWebFrame* frame, QPoint framePosition)

WebKit coding style...

> +void tst_QWebPage::frameAt()
> +{
> +    QWebView view;
> +    QWebPage *page = view.page();

Qt coding style...

Same file.

The patch looks good in theory and the tests look much more better.  However,
do to the missing comments in the ChangeLog and the nits over the changing
coding style, r- for now.