[webkit-reviews] review granted: [Bug 51825] [Qt] [WK2] create an initial implementation of the context menu handling for WebKit 2 : [Attachment 77814] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jan 3 11:26:10 PST 2011


Kenneth Rohde Christiansen <kenneth at webkit.org> has granted Benjamin Poulain
<benjamin.poulain at nokia.com>'s request for review:
Bug 51825: [Qt] [WK2] create an initial implementation of the context menu
handling for WebKit 2
https://bugs.webkit.org/show_bug.cgi?id=51825

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

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=77814&action=review

> WebCore/platform/qt/ContextMenuQt.cpp:76
> +Vector<ContextMenuItem> contextMenuItemVector(PlatformMenuDescription
platformDescription)

I would just write "description",  but as this is already paltform code, I
would replace PlatformMenuDescription with what it is defined to, ie
QList<ContextMenuItem>.

> WebKit2/UIProcess/API/qt/qgraphicswkview.cpp:330
> +    // Remove the active menu in case this function is called twice.

I think it is called "in the case". You can replace twice with repeatedly.

> WebKit2/UIProcess/API/qt/qgraphicswkview.cpp:341
> +    // FIXME: try to determine the view from the position of the incoming
events?
> +    QWidget* view = 0;
> +    if (QGraphicsScene* myScene = scene()) {
> +	   const QList<QGraphicsView*> views = myScene->views();
> +	   view = views.value(0, 0);
> +    }

Did you see how I did this with the vkb? I think that I tried handling this.

> WebKit2/UIProcess/qt/WebContextMenuProxyQt.cpp:49
> +    default: break;

I think break should go to the next line

> WebKit2/UIProcess/qt/WebContextMenuProxyQt.cpp:109
> +
> +    // don't show sub-menus with just disabled actions

With capital and ending with a dot

> WebKit2/UIProcess/qt/WebContextMenuProxyQt.cpp:114
> +    bool anyEnabledAction = false;

anyActionEnabled, anyActionWasEnabled? depending on what it really represents.


More information about the webkit-reviews mailing list