[webkit-reviews] review denied: [Bug 48720] Customizable context menu support in WK2 : [Attachment 73800] Patch v1 (without ChangeLogs)

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Brady Eidson <beidson at apple.com>'s
request for review:
Bug 48720: Customizable context menu support in WK2
https://bugs.webkit.org/show_bug.cgi?id=48720

Attachment 73800: Patch v1 (without ChangeLogs)
https://bugs.webkit.org/attachment.cgi?id=73800&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=73800&action=review

> WebKit2/Shared/API/c/WKContextMenuItem.cpp:66
> +    ImmutableArray* array = toImpl(submenuItems);
> +    size_t size = array->size();
> +    
> +    Vector<WebContextMenuItemData> webSubmenu;
> +    webSubmenu.reserveCapacity(size);
> +    
> +    for (size_t i = 0; i < size; ++i) {
> +	   WebContextMenuItem* item = array->at<WebContextMenuItem>(i);
> +	   if (item)
> +	       webSubmenu.append(*item->data());
> +    }
> +    
> +    return
toAPI(WebContextMenuItem::create(WebContextMenuItemData(ContextMenuItemTagNoAct
ion, toImpl(title)->string(), enabled, webSubmenu)).leakRef());

This should all be in WebContextMenuItem.h/cpp instead of in the API wrapper.

> WebKit2/Shared/API/c/WKContextMenuItem.cpp:72
> +    static WKContextMenuItemRef separatorItem =
toAPI(WebContextMenuItem::create(WebContextMenuItemData(SeparatorType,
ContextMenuItemTagNoAction, String(), true, false)).leakRef());
> +    return separatorItem;

This static should be too.

> WebKit2/Shared/API/c/WKContextMenuItem.cpp:117
> +    WebContextMenuItemData* data = toImpl(itemRef)->data();
> +    if (data->type() != SubmenuType)
> +	   return 0;
> +    
> +    const Vector<WebContextMenuItemData>& submenuVector(data->submenu());
> +    unsigned size = submenuVector.size();
> +    
> +    RefPtr<MutableArray> submenu = MutableArray::create();
> +    submenu->reserveCapacity(size);
> +    
> +    for (unsigned i = 0; i < size; ++i) {
> +	   RefPtr<WebContextMenuItem> item =
WebContextMenuItem::create(submenuVector[i]);
> +	   submenu->append(item.get());
> +    }
> +    
> +    return toAPI(submenu.release().leakRef());

Same here.

The WKArrayRef should probably be created by first generating a
Vector<RefPtr<WebContextMenuItem> > and then using ImmutableArray::adopt().

> WebKit2/Shared/API/c/WKContextMenuItem.h:28
> +#ifndef WKContextMenuItemTags_h
> +#define WKContextMenuItemTags_h
> +

This header guard seems wrong.

> WebKit2/Shared/API/c/WKContextMenuItem.h:54
> +#endif /* WKContextMenuItemTags_h */

This one too.

> WebKit2/Shared/API/c/WKSharedAPICast.h:413
> +	   return action;

This should probably have an explicit cast to make it clear what it is
happening.

> WebKit2/UIProcess/WebAPIContextMenuClient.h:31
> +#include <wtf/Vector.h>

Can we avoid #including <wtf/Vector.h> here in favor of <wtf/Forward.h>

> WebKit2/UIProcess/WebAPIContextMenuClient.h:40
> +class WebAPIContextMenuClient : public APIClient<WKPageContextMenuClient> {
> +public:

I don't like the API in this class name, can we replace it with Page?

> WebKit2/UIProcess/WebPageProxy.cpp:1193
> +    RefPtr<APIObject> userData;
> +    WebContextUserMessageDecoder messageDecoder(userData,
pageNamespace()->context());
> +    if (!arguments->decode(messageDecoder))
> +	   return;
> +	   

Probably best to do this as the first thing in this function.

> WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:28
> +#include "ImmutableDictionary.h"

Why is this change needed?