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

[bugzilla-daemon at webkit.org]

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





--- Comment #17 from Enrica Casucci <enrica at apple.com>  2010-09-23 18:00:15 PST ---
(In reply to comment #14)
> 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.

I will change it.
> 
> > 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.
> 

I agree, I will change it.
> > 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.
>

I've copied this code from WebKit. I assumed this was correct and I've looked at the definition of SEL.
I believe this is correct, also per Sam's comment.

> > 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.

Again, this is existing WebKit code, that I always assume is correct.

> > 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!
Sorry, leftover of a previous version of my implementation. Will remove it.

> 
> > 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.
>
You're right. I'll fix it.

> > 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.

I didn't know. I will change this per our conversation.

> 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.
>
Same as above.

> > 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.
>

I can change this.

> > 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?

I followed what WebKit does. I will double-check if it is really needed.
Thank you for all the comments.

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