[webkit-reviews] review denied: [Bug 42317] Pasteboard doesn't work in WebKit2 : [Attachment 68622] Patch4

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Enrica Casucci <enrica at apple.com>'s
request for review:
Bug 42317: Pasteboard doesn't work in WebKit2
https://bugs.webkit.org/show_bug.cgi?id=42317

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=68622&action=review

Looks like a good general approach, but there are some things that are not
quite right.

> WebKit2/UIProcess/PageClient.h:54
> +    virtual void enableMenuItem(const String& commandName, bool isEnabled) =
0;

While this terminology is used in Windows APIs -- a function named "enable"
that sets whether an item is enabled or not -- I have always found it awkward.
Cocoa APIs call this "setEnabled", which would lead to the name
setMenuItemEnabled. But also, is this really specific to menu items? Maybe this
should be named setEditCommandEnabled.

> WebKit2/UIProcess/WebPageProxy.h:135
> +    void editingCommand(const String& commandName);

Since this function performs and operation, its name should be a verb phrase,
not a noun phrase. It could be executeEditingCommand, or you could find some
other verb. But just "editing command" seems wrong.

> WebKit2/UIProcess/API/mac/WKView.mm:157
> +typedef HashMap<SEL, String> SelectorNameMap;

Does this type of HashMap work correctly? For selectors, we can use pointer
equality rather than string equality. I think you may need to specify PtrHash
explicitly to get the right behavior. I’m not sure exactly what type SEL is.

> WebKit2/UIProcess/API/mac/WKView.mm:161
> +static const SelectorNameMap* createSelectorExceptionMap()

It might be nicer to use PassOwnPtr here and call leak at the call site. Or use
"leak" in the name of the function.

> WebKit2/UIProcess/API/mac/WKView.mm:197
> +#define WEBCORE_COMMAND(command, name) - (void)command:(id)sender {
_data->_page->editingCommand(commandNameForSelector(_cmd)); }

What is the name argument for? It’s not used!

> WebKit2/UIProcess/API/mac/WKView.mm:213
> +    if (_data->_menuMap.isEmpty()) {

If the menu map is non-empty there’s no guarantee that the item is another menu
item from the same menu. This code assumes that validateUserInterfaceItem: is
called only for menu items within one menu at a time, but that’s wrong since
it’s called for things that are not menu items and can be called for more than
one menu at a time.

> WebKit2/UIProcess/API/mac/WKView.mm:216
> +	   _data->_currentMenu = [(NSMenuItem *)item menu];

The item is not guaranteed to be a menu item. It could be a toolbar button, for
example. So this cast is not correct. You could get method-not-found.

We can’t just keep a pointer to an NSMenu *. It might be deallocated and we
could use a freed pointer later! If we want to keep a pointer to it we have to
retain it.

> WebKit2/WebProcess/WebPage/WebPage.cpp:218
> +    if (m_page->focusController()->focusedOrMainFrame())
> +	  
m_page->focusController()->focusedOrMainFrame()->editor()->command(commandName)
.execute(argument);

Normally in WebKit code we do an early exit:

    Frame* frame = m_page->focusController()->focusedOrMainFrame();
    if (!frame)
	return;

> WebKit2/WebProcess/WebPage/WebPage.cpp:224
> +    if (frame) {

Same thing here. Early exist is the normal pattern.

> WebKit2/WebProcess/WebPage/WebPage.cpp:227
> +	   if (command.isSupported())
> +	       return command.isEnabled();

This can just say:

    return command.isSupported() && command.isEnabled();

But do we have to check both? Isn’t isEnabled guaranteed to return false if
isSupported is false?