[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