Bug 42317 – Pasteboard doesn't work in WebKit2

• Component: WebKit2
• Status: RESOLVED FIXED

[Maciej Stachowiak]

This makes at least the the following tests fail:

editing/pasteboard/copy-resolves-urls.html

[Maciej Stachowiak]

<rdar://problem/7660537>

[Sam Weinig]

*** Bug 42780 has been marked as a duplicate of this bug. ***

[Enrica Casucci]

Created attachment 67746 [details]
Patch

This is not the full implementation, but it is a step forward.
The bug should not be resolved when this patch is reviewed and landed.

[Enrica Casucci]

Committed revision 67631

[WebKit Review Bot]

http://trac.webkit.org/changeset/67631 might have broken Chromium Mac Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/67626
http://trac.webkit.org/changeset/67627
http://trac.webkit.org/changeset/67628
http://trac.webkit.org/changeset/67629
http://trac.webkit.org/changeset/67630
http://trac.webkit.org/changeset/67631

[Enrica Casucci]

Created attachment 67878 [details]
Patch2

Added RTF format support.

[Darin Adler]

Comment on attachment 67878 [details]
Patch2

> +        * loader/appcache/ApplicationCacheStorage.cpp:
> +        (WebCore::ApplicationCacheStorage::setCacheDirectory): Remove assert
> +        on the cachedirectory name, now that we can load a WebView also in the
> +        Web process for WebKit2.

I don’t understand the connection. Could you explain further?

[Darin Adler]

Comment on attachment 67878 [details]
Patch2

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

I think the load of the other WebKit framework in WebKit2 should be dynamic. Under many circumstances we can use WebKit2 without ever needing to run this code. The code should only load the WebKit framework if it’s actually run.

Loading the WebKit framework for converting RTF seems OK. Loading it just to convert the URL seems like a bad idea, although perhaps OK for now.

> WebKit2/WebProcess/WebPage/WebPage.h:53
> +#ifdef __OBJC__
> +@class NSAttributedString;
> +@class NSString;
> +@class NSURL;
> +#else

Is this part needed? I know the !OBJC part is needed, but I thought that when compiling Obj-C we would always have the data types from Foundation included as part of the precompiled header.

> WebKit2/WebProcess/WebPage/WebPage.h:158
> +#if PLATFORM(MAC)
> +    NSString* userVisibleString(NSURL*);
> +    WebCore::DocumentFragment* documentFragmentFromAttributedString(NSAttributedString*, Vector<WebCore::ArchiveResource*>&);
> +#endif

Do we really need to use the Cocoa types here? That makes this code Mac-specific when it could otherwise possibly be shared with other platforms like Windows when they use CF. What about the CF types, CFStringRef, CFURLRef, and CFAttributedStringRef? At least the first two are “toll-free bridged”.

The new code is inconsistent about the position of the "*" characters. Our strange rule is that Obj-C types always have a space before the *. I would like to change that rule in the future, but I would like you to be consistent within the patch at least, if possible.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:291
> +static NSArray* excludedElementsForAttributedStringConversion()

A better style for this is to make a separate create function for the one time code, and then write:

    static NSArray *elements = createExcludedElementsArray();
    return elements;

It’s cleaner without the "== nil" and if statement and such.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:305
> +    static NSArray *elements = nil;
> +    if (elements == nil) {
> +        elements = [[NSArray alloc] initWithObjects:
> +                    // Omit style since we want style to be inline so the fragment can be easily inserted.
> +                    @"style",
> +                    // Omit xml so the result is not XHTML.
> +                    @"xml", 
> +                    // Omit tags that will get stripped when converted to a fragment anyway.
> +                    @"doctype", @"html", @"head", @"body",
> +                    // Omit deprecated tags.
> +                    @"applet", @"basefont", @"center", @"dir", @"font", @"isindex", @"menu", @"s", @"strike", @"u",
> +                    // Omit object so no file attachments are part of the fragment.
> +                    @"object", nil];

I understand that you were copying existing code, but typically we avoid code lined up with the previous lines like this, because it’s not friendly when renaming among other reasons.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:313
> +    WebView* view = [[WebView alloc] initWithFrame:NSMakeRect(0, 0, 0, 0)];

Calling the init method will call initWithFrame, so there’s no need to call initWithFrame:. Also, you can use NSEmptyRect in cases like this if you do need this rectangle.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:317
> +    NSDictionary *dictionary = [[NSDictionary alloc] initWithObjectsAndKeys:
> +                                excludedElementsForAttributedStringConversion(), NSExcludedElementsDocumentAttribute,
> +                                nil, @"WebResourceHandler", nil];

Again, I prefer to avoid lining up like this.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:319
> +    NSArray* s;

I much prefer words for local variable names over letters. I would name this “subresources”.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:324
> +    NSEnumerator *e = [s objectEnumerator];

And this, “enumerator”.

Since this code doesn’t need to run on systems older than Snow Leopard, you can use Objective-C 2.0 style enumeration which is a lot cleaner. I don’t know the syntax myself, but I know it’s the preferred style.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:327
> +        RefPtr<ArchiveResource>  ar = [r _coreResource];

There’s an extra space here. I don’t think you need to put this into a local variable. It would read better if you didn’t.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:328
> +        resources.append(ar.get());

Why is resources an array of raw pointers? What guarantees the ArchiveResource objects will stay alive? I think it probably needs to be a vector of RefPtr, unless I’m missing something.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:338
> +NSString* WebPage::userVisibleString(NSURL* URL)
> +{
> +    return [URL _web_userVisibleString];
> +}

I don’t understand why we are indirecting here through a WebPage function. Why can’t WebEditorClient just call this function directly?

In fact, in both cases I am a little puzzled. It seems that putting this code into WebPage just makes WebPage more of a god class. Perhaps there’s a way we can avoid this. These functions don’t seem to rely on being members of WebPage, so they could be static members if they are going to be members, but I am not sure they get any benefit from being class members.

[Enrica Casucci]

(In reply to comment #8)
> I think the load of the other WebKit framework in WebKit2 should be dynamic. Under many circumstances we can use WebKit2 without ever needing to run this code. The code should only load the WebKit framework if it’s actually run.

We changed that now. The next patch will not do that.
 
> Loading the WebKit framework for converting RTF seems OK. Loading it just to convert the URL seems like a bad idea, although perhaps OK for now.
> 
> > WebKit2/WebProcess/WebPage/WebPage.h:53
> > +#ifdef __OBJC__
> > +@class NSAttributedString;
> > +@class NSString;
> > +@class NSURL;
> > +#else
> 
> Is this part needed? I know the !OBJC part is needed, but I thought that when compiling Obj-C we would always have the data types from Foundation included as part of the precompiled header.

You're right, I just followed another example, but I tried what you're saying and it's not necessary when __OBJC__.
 
> > WebKit2/WebProcess/WebPage/WebPage.h:158
> > +#if PLATFORM(MAC)
> > +    NSString* userVisibleString(NSURL*);
> > +    WebCore::DocumentFragment* documentFragmentFromAttributedString(NSAttributedString*, Vector<WebCore::ArchiveResource*>&);
> > +#endif
> 
> Do we really need to use the Cocoa types here? That makes this code Mac-specific when it could otherwise possibly be shared with other platforms like Windows when they use CF. What about the CF types, CFStringRef, CFURLRef, and CFAttributedStringRef? At least the first two are “toll-free bridged”.
> 
I didn't know this was possible.

> The new code is inconsistent about the position of the "*" characters. Our strange rule is that Obj-C types always have a space before the *. I would like to change that rule in the future, but I would like you to be consistent within the patch at least, if possible.
>

I'm not sure I understand what you want me to do here. No space before * for C++ and space before * for obj-c?
 
> > WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:291
> > +static NSArray* excludedElementsForAttributedStringConversion()
> 
> A better style for this is to make a separate create function for the one time code, and then write:
> 
>     static NSArray *elements = createExcludedElementsArray();
>     return elements;
> 
> It’s cleaner without the "== nil" and if statement and such.
> 
> > WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:305
> > +    static NSArray *elements = nil;
> > +    if (elements == nil) {
> > +        elements = [[NSArray alloc] initWithObjects:
> > +                    // Omit style since we want style to be inline so the fragment can be easily inserted.
> > +                    @"style",
> > +                    // Omit xml so the result is not XHTML.
> > +                    @"xml", 
> > +                    // Omit tags that will get stripped when converted to a fragment anyway.
> > +                    @"doctype", @"html", @"head", @"body",
> > +                    // Omit deprecated tags.
> > +                    @"applet", @"basefont", @"center", @"dir", @"font", @"isindex", @"menu", @"s", @"strike", @"u",
> > +                    // Omit object so no file attachments are part of the fragment.
> > +                    @"object", nil];
> 
> I understand that you were copying existing code, but typically we avoid code lined up with the previous lines like this, because it’s not friendly when renaming among other reasons.
> 

As you said, I copied the code and I assumed that, since it was in, it was ok. What kind of alignment should I use here?

> > WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:313
> > +    WebView* view = [[WebView alloc] initWithFrame:NSMakeRect(0, 0, 0, 0)];
> 
> Calling the init method will call initWithFrame, so there’s no need to call initWithFrame:. Also, you can use NSEmptyRect in cases like this if you do need this rectangle.
> 

Not applicable anymore, since there is no WebView.

> > WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:319
> > +    NSArray* s;
> 
> I much prefer words for local variable names over letters. I would name this “subresources”.
>

Ok.
 
> > WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:324
> > +    NSEnumerator *e = [s objectEnumerator];
> 
> And this, “enumerator”.
> 
> Since this code doesn’t need to run on systems older than Snow Leopard, you can use Objective-C 2.0 style enumeration which is a lot cleaner. I don’t know the syntax myself, but I know it’s the preferred style.
>

I'll look it up.
 
> > WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:327
> > +        RefPtr<ArchiveResource>  ar = [r _coreResource];
> 
> There’s an extra space here. I don’t think you need to put this into a local variable. It would read better if you didn’t.
>

ok.
 
> > WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:328
> > +        resources.append(ar.get());
> 
> Why is resources an array of raw pointers? What guarantees the ArchiveResource objects will stay alive? I think it probably needs to be a vector of RefPtr, unless I’m missing something.
> 

You're probably right. It got lost in the WebKit to WebCore conversion. I'll check the WebKit version I checked in at the beginning of the week to verify if the same problem is there.

> > WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:338
> > +NSString* WebPage::userVisibleString(NSURL* URL)
> > +{
> > +    return [URL _web_userVisibleString];
> > +}
> 
> I don’t understand why we are indirecting here through a WebPage function. Why can’t WebEditorClient just call this function directly?

I don't know how to call obj-c from a cpp file.

> 
> In fact, in both cases I am a little puzzled. It seems that putting this code into WebPage just makes WebPage more of a god class. Perhaps there’s a way we can avoid this. These functions don’t seem to rely on being members of WebPage, so they could be static members if they are going to be members, but I am not sure they get any benefit from being class members.

Same reason as above.
Is it possible to do so?

[Enrica Casucci]

Created attachment 67975 [details]
Patch3

No more use of WebView nor linking of WebKit.
I believe I've addressed all the comments.

[Sam Weinig]

Comment on attachment 67975 [details]
Patch3

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

> WebCore/loader/EmptyClients.h:438
> +    virtual DocumentFragment* documentFragmentFromAttributedString(NSAttributedString*, Vector< RefPtr<ArchiveResource> >&) { return 0; };

No space before the RefPtr is needed.

> WebCore/page/EditorClient.h:156
> +    virtual DocumentFragment* documentFragmentFromAttributedString(NSAttributedString*, Vector< RefPtr<ArchiveResource> >&) = 0;

Here too.

> WebCore/platform/mac/PasteboardMac.mm:410
> +    Vector< RefPtr<ArchiveResource> > resources;

And here.

> WebKit2/WebProcess/WebCoreSupport/mac/WebEditorClientMac.mm:84
> +DocumentFragment* WebEditorClient::documentFragmentFromAttributedString(NSAttributedString *string, Vector< RefPtr<ArchiveResource> >& resources)
> +{

And here.

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:362
> +DocumentFragment* WebEditorClient::documentFragmentFromAttributedString(NSAttributedString *string, Vector< RefPtr<ArchiveResource> >& resources)
>  {

And here,


Otherwise, r=me.

[Enrica Casucci]

Committed patch3. http://trac.webkit.org/changeset/67864

[Enrica Casucci]

Created attachment 68622 [details]
Patch4

Last piece.

[Darin Adler]

Comment on attachment 68622 [details]
Patch4

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?

[Early Warning System Bot]

Attachment 68622 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4079093

[Sam Weinig]

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

SEL is defined as typedef struct objc_selector 	*SEL; so this should work fine.

[Enrica Casucci]

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

[Enrica Casucci]

Created attachment 68788 [details]
Patch5

This patch addresses all the comments.

[Early Warning System Bot]

Attachment 68788 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4021153

[Darin Adler]

Comment on attachment 68788 [details]
Patch5

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

This patch seems to break the Qt WebKit2. If you resolve that seems OK to land.

> WebKit2/UIProcess/API/mac/WKView.mm:72
> +        MenuItemInfo(): _isEnabled(false), _state(0) {};
> +        MenuItemInfo(bool isEnabled, int state): _isEnabled(isEnabled), _state(state) {};

Need a space before the ":". No semicolon needed after the "{}". We normally put a space in the "{ }".

You call this EditCommandState in the PageClient API. Why not call it that here too?

No need for underscores in this class’s members. We normally don’t use them for struct-style classes like this one. We do use them for Objective-C classes.

I suggest struct instead of class for something where everything is public.

Not sure it’s good to nest a C++ class inside an Objective-C class definition. I suggest putting it outside WKViewData.

> WebKit2/UIProcess/API/mac/WKView.mm:207
> +// FIXME: we should add here all the commands as we implement them.

Grammar mistake. Should be "We should add all the commands here as we implement them."

> WebKit2/UIProcess/API/mac/WKView.mm:227
> +        return NO;      // FIXME: we need to be able to handle other user interface elements.

We capitalize comments, using sentence style. We normally put comments only one space past the code, not 6 spaces.

> WebKit2/UIProcess/API/mac/WKView.mm:496
> +    _data->_needsDataForValidation = false;

I think the “needs data for validation” mechanism is a bit roundabout. We could try replacing it with a boolean that means “I am inside the update call” instead to see if that results in easier to read code.

[Enrica Casucci]

Committed revision 68318.