Bug 52496 – Drag and drop support: refactoring of image from link and image from selection

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Enrica Casucci]

The Mac specific code that creates the drag image has some inconsistencies between the way the image is created from a selection and the one created from a link.
Also both call into the NSView directly from the WebCore level.
It would be nice to clean up this code, to avoid using the NSView from WebCore.

This is requirement for WebKit2.

[Enrica Casucci]

Created attachment 79033 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 79033 [details]
Patch

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

Just nit comments since I don't know Cocoa / Objective-C.

> Source/WebCore/page/mac/FrameMac.mm:281
> +    

Nit: space.

> Source/WebCore/page/mac/FrameMac.mm:284
> +    

Ditt.

> WebKit2/UIProcess/API/mac/WKView.mm:201
> +    //[self _registerDraggedTypes];

Maybe you want to delete this line?

> WebKit2/UIProcess/API/mac/WKView.mm:1018
> +/*
>  - (NSDragOperation)draggingEntered:(id <NSDraggingInfo>)draggingInfo

Ditto.

> WebKit2/UIProcess/API/mac/WKView.mm:1059
> +*/

Ditto.

> WebKit/mac/WebCoreSupport/WebDragClient.mm:64
> +#define DRAG_LABEL_BORDER_Y_OFFSET              2.0f
> +
> +#define MIN_DRAG_LABEL_WIDTH_BEFORE_CLIP        120.0f
> +#define MAX_DRAG_LABEL_WIDTH                    320.0f
> +
> +#define DRAG_LINK_LABEL_FONT_SIZE   11.0f
> +#define DRAG_LINK_URL_FONT_SIZE   10.0f

Maybe nice to align these numbers to above 3 numbers?

[Ryosuke Niwa]

Comment on attachment 79033 [details]
Patch

r- for now because of your commenting out code.

[Enrica Casucci]

(In reply to comment #3)
> (From update of attachment 79033 [details])
> r- for now because of your commenting out code.

Sorry that code was commented out by mistake. I was testing something and forgot to remove the comments before I prepared the patch. Uploading now the correct patch.

[Enrica Casucci]

Created attachment 79044 [details]
Patch2

[Alexey Proskuryakov]

Comment on attachment 79044 [details]
Patch2

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

I don't really understand why DragClient is a better place for drag image generation, but OK.

> Source/WebCore/page/mac/FrameMac.mm:-265
> +    PaintBehavior oldBehavior = m_view->paintBehavior();
> +    m_view->setPaintBehavior(oldBehavior | PaintBehaviorFlattenCompositingLayers);
>      
> -    PaintBehavior oldPaintBehavior = m_view->paintBehavior();
> -    m_view->setPaintBehavior(oldPaintBehavior | PaintBehaviorFlattenCompositingLayers);

What's wrong about "oldPaintBehavior" name? It seems more clear to me.

> Source/WebCore/page/mac/FrameMac.mm:-275
> -    // Round image rect size in window coordinate space to avoid pixel cracks at HiDPI (4622794)
> -    rect = [view convertRect:rect toView:nil];
>      rect.size.height = roundf(rect.size.height);
>      rect.size.width = roundf(rect.size.width);
> -    rect = [view convertRect:rect fromView:nil];

So, this is no longer necessary for 4622794?

> Source/WebCore/page/mac/FrameMac.mm:275
> +        m_view->paintContents(&graphicsContext, IntRect(rect));

Can m_view be null? I'm asking myself the same question as there's almost identical code in PrintContext, and it doesn't check for null - but general Frame code has a lot of null checks for null m_view.

> WebKit/mac/WebCoreSupport/WebDragClient.mm:65
> +#define DRAG_LABEL_BORDER_X                     4.0f
> +//Keep border_y in synch with DragController::LinkDragBorderInset
> +#define DRAG_LABEL_BORDER_Y                     2.0f
> +#define DRAG_LABEL_RADIUS                       5.0f
> +#define DRAG_LABEL_BORDER_Y_OFFSET              2.0f
> +
> +#define MIN_DRAG_LABEL_WIDTH_BEFORE_CLIP        120.0f
> +#define MAX_DRAG_LABEL_WIDTH                    320.0f
> +
> +#define DRAG_LINK_LABEL_FONT_SIZE               11.0f
> +#define DRAG_LINK_URL_FONT_SIZE                 10.0f
> +

We're now using consts (appropriately named, not in ALL_CAPS) instead of defines. Const values needn't have ".0f" suffixes.

> WebKit/mac/WebCoreSupport/WebDragClient.mm:165
> +    NSSize imageSize, urlStringSize;

Please don't declare two variables on one line. In this case, urlStringSize should go inside an if block below.

> WebKit/mac/WebCoreSupport/WebDragClient.mm:177
> +        } else {
> +            imageSize.width = max(labelSize.width + DRAG_LABEL_BORDER_X * 2, urlStringSize.width + DRAG_LABEL_BORDER_X * 2);
> +        }

No braces around single line blocks.

> WebKit/mac/WebCoreSupport/WebDragClient.mm:182
> +    [[NSColor colorWithDeviceRed: 0.7f green: 0.7f blue: 0.7f alpha: 0.8f] set];

I don't think that we're putting spaces after semicolons. This style mistake is repeated elsewhere in the moved code.

> WebKit/mac/WebCoreSupport/WebDragClient.mm:184
> +    // Drag a rectangle with rounded corners/

s/\//./

> WebKit/mac/WebCoreSupport/WebDragClient.mm:186
> +    [path appendBezierPathWithOvalInRect: NSMakeRect(0.0f, 0.0f, DRAG_LABEL_RADIUS * 2.0f, DRAG_LABEL_RADIUS * 2.0f)];

0.0.f -> 0; 2.0f -> 2

[Enrica Casucci]

(In reply to comment #6)
> (From update of attachment 79044 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79044&action=review
> 
> I don't really understand why DragClient is a better place for drag image generation, but OK.
The best place to have this code would be WebCore and avoid using a platform specific image, but I did not want to make too many changes to this code, since the goal is to support drag and drop.
At least not it is consistent with the Windows implementation.

As fas as all the other comments, I just copied the code from WebHTMLView.mm.
I'll make the suggested changes. Thank you.

[Enrica Casucci]

 > -    // Round image rect size in window coordinate space to avoid pixel cracks at HiDPI (4622794)
> > -    rect = [view convertRect:rect toView:nil];
> >      rect.size.height = roundf(rect.size.height);
> >      rect.size.width = roundf(rect.size.width);
> > -    rect = [view convertRect:rect fromView:nil];
> 
> So, this is no longer necessary for 4622794?
I spoke with Anders and he confirmed that we changed the way we handle this issue. Therefore I'll remove the 2 roundf statements as well.

[Enrica Casucci]

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