Bug 86625 – [Chromium] Implement the ContextMenuItem SubMenu type

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Alexander Pavlov (apavlov)]

This is necessary for rendering submenu items in the Web Inspector's context menu.

[Alexander Pavlov (apavlov)]

The bug is tracked downstream as https://code.google.com/p/chromium/issues/detail?id=128351 as it requires the Chromium non-WebKit code changes.

[Alexander Pavlov (apavlov)]

Created attachment 142261 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 142261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142261&action=review

The API change LGTM.  The rest of the patch looks plausible, but I'm not the right person to review it.

> Source/WebCore/platform/chromium/ContextMenuChromium.cpp:68
> +    Vector<Vector<ContextMenuItem>*> menuItemStack;

Would it be better to use Vector<Vector<ContextMenuItem>&> to avoid all the conversion back and forth between references and pointers?

[Alexander Pavlov (apavlov)]

(In reply to comment #4)

> Would it be better to use Vector<Vector<ContextMenuItem>&> to avoid all the conversion back and forth between references and pointers?

Does not compile for me: gives a bunch of messages like
../../third_party/WebKit/Source/WTF/wtf/Vector.h:489: error: forming pointer to reference type 'WTF::Vector<WebCore::ContextMenuItem, 0ul>&'
(which refers to "typedef T* iterator;").

[Adam Barth]

Interesting.

[Kent Tamura]

Comment on attachment 142261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142261&action=review

> Source/WebKit/chromium/public/WebMenuItemInfo.h:68
>      bool hasTextDirectionOverride;
>      bool enabled;
>      bool checked;
> +    WebVector<WebMenuItemInfo> subMenuItems;

We had better put subMenuItems before hasTextDirectionOverride to minimize padding.
Other than this point, WebKit API change looks ok.

[Alexander Pavlov (apavlov)]

Created attachment 142676 [details]
Patch

[WebKit Review Bot]

Comment on attachment 142676 [details]
Patch

Clearing flags on attachment: 142676

Committed r117970: <http://trac.webkit.org/changeset/117970>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.