Bug 78768 – Refactor DragData class to use PlatformStrategies in the Mac implementation

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

[Enrica Casucci]

This bug track another step in the direction of removing access to NSPasteboard from the WebProcess.
The Mac implementation of the DragData class needs to be redesigned to use a platform strategy that can provide different implementations for WebKit and WebKit2 and remove direct access to the NSPasteboard object.

[Enrica Casucci]

Created attachment 127289 [details]
patch

[Ryosuke Niwa]

Comment on attachment 127289 [details]
patch

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

> Source/WebCore/platform/DragData.h:124
> -    NSPasteboard *pasteboard() { return m_pasteboard.get(); }
> +    const String& pasteboardName() { return m_pasteboardName; }

Yay! one less dependency on NSPasteboard in WebCore.

LGTM. I'll let someone from Mac port review this patch though.

[Darin Adler]

Comment on attachment 127289 [details]
patch

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

> Source/WebCore/platform/PasteboardStrategy.h:31
> +#include "Color.h"

Having a return type of Color requires only a forward declaration of the Color type, not an include. Files with call sites that actually call the function can include Color.h but the header need not.

> Source/WebCore/platform/PlatformPasteboard.h:29
> +#include "Color.h"

Ditto.

> Source/WebCore/platform/mac/DragDataMac.mm:70
> +    Pasteboard pasteboard(m_pasteboardName);
> +    return pasteboard.canSmartReplace();

I think this would read better without a local variable:

    return Pasteboard(m_pasteboardName).canSmartReplace();

> Source/WebCore/platform/mac/DragDataMac.mm:77
> +    return types.contains(String(NSColorPboardType));

Is the explicit String() needed? I am surprised it is. Did you try compiling without it?

> Source/WebCore/platform/mac/DragDataMac.mm:84
> +    return types.contains(String(NSFilenamesPboardType));

Ditto. Lots more examples of this below.

> Source/WebCore/platform/mac/DragDataMac.mm:90
> +    platformStrategies()->pasteboardStrategy()->getPathnamesForType(files, String(NSFilenamesPboardType), m_pasteboardName);

Ditto.

> Source/WebCore/platform/mac/DragDataMac.mm:114
> +    Pasteboard pasteboard(m_pasteboardName);
>      return pasteboard.plainText(frame);

Same comment about not reading better without a local variable.

> Source/WebCore/platform/mac/DragDataMac.mm:136
> +    return types.contains(String(WebArchivePboardType))
> +        || types.contains(String(NSHTMLPboardType))
> +        || types.contains(String(NSFilenamesPboardType))
> +        || types.contains(String(NSTIFFPboardType))
> +        || types.contains(String(NSPDFPboardType))
> +        || types.contains(String(NSURLPboardType))
> +        || types.contains(String(NSRTFDPboardType))
> +        || types.contains(String(NSRTFPboardType))
> +        || types.contains(String(NSStringPboardType))
> +        || types.contains(String(NSColorPboardType))
> +        || types.contains(String(kUTTypePNG));

This function is going to be really slow, creating and destroying lots of String objects. That might not matter, but if there is a chance it does matter we might want to allocate the String objects only once instead of making them every time.

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:86
> +    return makeRGBA((int)([color redComponent] * 255.0 + 0.5), (int)([color greenComponent] * 255.0 + 0.5), 
> +                    (int)([color blueComponent] * 255.0 + 0.5), (int)([color alphaComponent] * 255.0 + 0.5));    

I am really puzzled by the * 255 + 0.5 math here. Not at all new to this patch, but also does not seem to be the correct way to convert floating point RGBA colors to RGBA 8/8/8/8 colors.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:181
> +    PlatformPasteboard pasteboard(pasteboardName);
> +    return pasteboard.color();    

Again, probably better without a local variable.

> Source/WebKit/mac/WebCoreSupport/WebPlatformStrategies.mm:144
> +    PlatformPasteboard pasteboard(pasteboardName);
> +    return pasteboard.color();    

Ditto.

[Enrica Casucci]

Thanks for the review.
I've addressed your comments on the inclusion of "Color.h". I've avoided using a local variable for Pasteboard and PlatformPasteboard, also in other places landed as part of a previous patch.

> Is the explicit String() needed? I am surprised it is. Did you try compiling without it?
> 
Yes, it fails to compile without it.

> 
> > Source/WebCore/platform/mac/DragDataMac.mm:136
> > +    return types.contains(String(WebArchivePboardType))
> > +        || types.contains(String(NSHTMLPboardType))
> > +        || types.contains(String(NSFilenamesPboardType))
> > +        || types.contains(String(NSTIFFPboardType))
> > +        || types.contains(String(NSPDFPboardType))
> > +        || types.contains(String(NSURLPboardType))
> > +        || types.contains(String(NSRTFDPboardType))
> > +        || types.contains(String(NSRTFPboardType))
> > +        || types.contains(String(NSStringPboardType))
> > +        || types.contains(String(NSColorPboardType))
> > +        || types.contains(String(kUTTypePNG));
> 
> This function is going to be really slow, creating and destroying lots of String objects. That might not matter, but if there is a chance it does matter we might want to allocate the String objects only once instead of making them every time.

I want to change all this in a separate patch. There are still several places where we need to convert from NSString to String and I want to do it once and for all.
> 
> > Source/WebCore/platform/mac/PlatformPasteboardMac.mm:86
> > +    return makeRGBA((int)([color redComponent] * 255.0 + 0.5), (int)([color greenComponent] * 255.0 + 0.5), 
> > +                    (int)([color blueComponent] * 255.0 + 0.5), (int)([color alphaComponent] * 255.0 + 0.5));    
> 
> I am really puzzled by the * 255 + 0.5 math here. Not at all new to this patch, but also does not seem to be the correct way to convert floating point RGBA colors to RGBA 8/8/8/8 colors.

I'm not sure about this code either, but I didn't want to change it without fully understanding.

[Enrica Casucci]

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