Bug 77567 – Refactor Mac platform implementation of the Pasteboard class

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Enrica Casucci]

We should move all access to NSPasteboard object to a separate class, so that we can have a different implementation for WebKit1 and WebKit2.
We don't wan't to access the NSPasteboard object from the WebProcess.

[Enrica Casucci]

Created attachment 124986 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 124986 [details]
Patch

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

> Source/WebCore/ChangeLog:29
> +        ():

The boilerplate is expected to be edited to remove such garbage.

> Source/WebCore/platform/Pasteboard.h:72
> +class PasteboardImpl;

There is an include of PasteboardImpl.h above - is it needed?

> Source/WebCore/platform/mac/PasteboardImpl.h:38
> +class Clipboard;
> +class Clipboard;

Double Clipboard.

> Source/WebCore/platform/mac/PasteboardImpl.h:44
> +class Range;
> +class ArchiveResource;

Please sort these alphabetically.

> Source/WebCore/platform/mac/PasteboardImpl.h:51
> +class PasteboardImpl : public RefCounted<PasteboardImpl> {
> +public:
> +    virtual ~PasteboardImpl() { };
> +    virtual NSArray* readTypes() = 0;

Will we need to switch implementations at runtime? Otherwise, an abstract interface is a wrong idiom to use. I think that we normally use strategies to switch between WK1 and WK2 implementations.

> Source/WebCore/platform/mac/PasteboardImpl.h:69
> +class PasteboardImplWK1: public PasteboardImpl {

The approach being unusual is reflected in the naming - we don't have any other class with WK1 in the name.

> Source/WebCore/platform/mac/PasteboardImplWK1.mm:115
> +    if (string == nil)

Should be just "if (string)" per WebKit coding style.

[Enrica Casucci]

> Will we need to switch implementations at runtime? Otherwise, an abstract interface is a wrong idiom to use. I think that we normally use strategies to switch between WK1 and WK2 implementations.
> 
I spoke with Sam about this and agreed on the use of strategies. It is a simple change from the existing implementation.

[Enrica Casucci]

Created attachment 125197 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 125197 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        writeSelectionForTypes has been left to support Mac OSX services.

s/Mac OSX/OS X/

> Source/WebCore/platform/Pasteboard.h:-84
> +    // This is required to support Mac OSX services.
>      void writeSelectionForTypes(NSArray* pasteboardTypes, Range* selectedRange, bool canSmartCopyOrDelete, Frame*);
> -    void writeURLForTypes(NSArray* types, const KURL&, const String& titleStr, Frame*);

s/Mac OSX/OS X/

I'm not sure if I fully understand why one of the two methods needed for services can be moved, but not another.

> Source/WebCore/platform/mac/DragDataMac.mm:168
> +    // FIXME: using the editorClient to call into webkit, for now, since 
> +    // calling webkit_canonicalize from WebCore involves migrating a sizable amount of 
> +    // helper code that should either be done in a separate patch or figured out in another way.

s/webkit/WebKit/.

Why do we need WebKit to canonicalize a URL? Normally, KURL does that.

> Source/WebCore/platform/mac/DragDataMac.mm:187
> +        // FIXME: Maybe it makes more sense to allow multiple files and only use the first one?

This is something Jon Lee would have an opinion about.

> Source/WebCore/platform/mac/PasteboardMac.mm:380
> -PassRefPtr<DocumentFragment> Pasteboard::documentFragmentWithRtf(Frame* frame, NSString* pboardType)
> +static PassRefPtr<DocumentFragment> documentFragmentWithRtf(Frame* frame, NSString* pboardType, NSPasteboard* pasteboard)

You can consider fixing coding style when touching code like this.

[Enrica Casucci]

Thanks for the review!

> > Source/WebCore/platform/Pasteboard.h:-84
> > +    // This is required to support Mac OSX services.
> >      void writeSelectionForTypes(NSArray* pasteboardTypes, Range* selectedRange, bool canSmartCopyOrDelete, Frame*);
> > -    void writeURLForTypes(NSArray* types, const KURL&, const String& titleStr, Frame*);
> 
> I'm not sure if I fully understand why one of the two methods needed for services can be moved, but not another.
To support Mac OS X services we need to implement the NSServicesRequests protocols that includes the writeSelectionToPasteboard:types: method.
writeSelectionForTypes is the WebCore method that implements that functionality.
writeURLForTypes was just an implementation choice in Pasteboard.
> 
> > Source/WebCore/platform/mac/DragDataMac.mm:168
> > +    // FIXME: using the editorClient to call into webkit, for now, since 
> > +    // calling webkit_canonicalize from WebCore involves migrating a sizable amount of 
> > +    // helper code that should either be done in a separate patch or figured out in another way.
> 
> Why do we need WebKit to canonicalize a URL? Normally, KURL does that.

I don't know the answer to this question. I simply moved this code from Pasteboard to this file.
> 
> > Source/WebCore/platform/mac/DragDataMac.mm:187
> > +        // FIXME: Maybe it makes more sense to allow multiple files and only use the first one?
> 
> This is something Jon Lee would have an opinion about.

Again, this is code moved from Pasteboard with the  relevant comments. Addressing this issue is beyond the scope of this patch.

[Jon Lee]

Comment on attachment 125197 [details]
Patch

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

>> Source/WebCore/platform/mac/DragDataMac.mm:187
>> +        // FIXME: Maybe it makes more sense to allow multiple files and only use the first one?
> 
> This is something Jon Lee would have an opinion about.

I think we should just restrict this to just one file and disallow multiple files. Otherwise using the suggested solution produces unpredictable results for the user. It's the same thing as another WK bug (can't find it right now), where dragging multiple files into a single-file input results in nothing being accepted, for the same reason.

[Enrica Casucci]

http://trac.webkit.org/changeset/106872