[webkit-reviews] review granted: [Bug 52496] Drag and drop support: refactoring of image from link and image from selection : [Attachment 79044] Patch2

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 52496: Drag and drop support: refactoring of image from link and image from
selection
https://bugs.webkit.org/show_bug.cgi?id=52496

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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