[webkit-reviews] review granted: [Bug 78768] Refactor DragData class to use PlatformStrategies in the Mac implementation : [Attachment 127289] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Enrica Casucci <enrica at apple.com>'s
request for review:
Bug 78768: Refactor DragData class to use PlatformStrategies in the Mac
implementation
https://bugs.webkit.org/show_bug.cgi?id=78768

Attachment 127289: patch
https://bugs.webkit.org/attachment.cgi?id=127289&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.