[webkit-reviews] review denied: [Bug 62191] [Qt] Creating webkit2 popup menu hooks and adding popup menus to MiniBrowser. : [Attachment 96214] patch

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied Luiz Agostini <luiz at webkit.org>'s
request for review:
Bug 62191: [Qt] Creating webkit2 popup menu hooks and adding popup menus to
MiniBrowser.
https://bugs.webkit.org/show_bug.cgi?id=62191

Attachment 96214: patch
https://bugs.webkit.org/attachment.cgi?id=96214&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=96214&action=review

I like where this is going. Let's do another iteration though. :)

> Source/WebKit2/UIProcess/API/qt/qwkpage.h:108
> +    typedef QWKPopupMenu* (*CreatePopupMenuFn)(void *);

Style, void * -> void*.

> Source/WebKit2/UIProcess/API/qt/qwkpage.h:110
> +    QWKPopupMenu* craetePopupMenu();

Typo, createPopupMenu().

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:45
> +

Unnecessary newline.

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:49
> +	       return Separator;

Not sure it makes sense that invalid indices claim to be Separators.. is there
a precedent for this?
Same comment repeated for all functions in this class, basically. :)

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:78
> +

Unnecessary newline.

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:84
> +    QRect m_rect;
> +    WebCore::TextDirection m_direction;
> +    double m_scaleFactor;
> +    Vector<WebKit::WebPopupItem> m_items;
> +    int m_selectedIndex;

It's good practice to order member variables by size in descending order, to
avoid ballooning the class with padding.

> Tools/MiniBrowser/qt/PopupMenu.cpp:117
> +    m_timer.start(1);

Why not start(0)?