[Webkit-unassigned] [Bug 42317] Pasteboard doesn't work in WebKit2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=42317


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #68622|review?                     |review-
               Flag|                            |




--- Comment #14 from Darin Adler <darin at apple.com>  2010-09-23 17:23:50 PST ---
(From update of attachment 68622)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.