[webkit-reviews] review granted: [Bug 124325] Consolidate and expose Frame/Node/Selection screenshot capabilities : [Attachment 218467] v5

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brian Burg <burg at cs.washington.edu>'s
request for review:
Bug 124325: Consolidate and expose Frame/Node/Selection screenshot capabilities
https://bugs.webkit.org/show_bug.cgi?id=124325

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=218467&action=review


> Source/WebCore/page/FrameSnapshotting.cpp:113
> +    options |= SnapshotOptionsPaintSelectionOnly;
> +    return snapshotFrameRect(frame,
enclosingIntRect(frame.selection().bounds()), options);

Awkward to modify the options argument with |= rather than just using | in the
function call.

    return snapshotFrameRect(frame,
enclosingIntRect(frame.selection().bounds()), options |
SnapshotOptionsPaintSelectionOnly);

> Source/WebCore/platform/DragImage.h:46
> -//We need to #define YOffset as it needs to be shared with WebKit
> +// We need to #define YOffset as it needs to be shared with WebKit
>  #define DragLabelBorderYOffset 2

I don’t understand this comment that we are reformatting. How on earth does
defining DragLabelBorderYOffset (not YOffset) allow it to be “shared with
WebKit”.

> Source/WebCore/platform/DragImage.h:65
>  #elif PLATFORM(WIN)
> -    typedef HBITMAP DragImageRef;
> +typedef HBITMAP DragImageRef;
>  #elif PLATFORM(GTK) || PLATFORM(NIX)
> -    typedef cairo_surface_t* DragImageRef;
> +typedef cairo_surface_t* DragImageRef;
>  #elif PLATFORM(EFL) || PLATFORM(BLACKBERRY)
> -    typedef void* DragImageRef;
> +typedef void* DragImageRef;
>  #endif

We should really change all of these to use some kind of object or smart
pointer, so we don’t need deleteDragImage.

> Source/WebKit/mac/WebView/WebHTMLView.mm:5977
>      if (![self _hasSelection])
>	   return nil;
> -    return selectionImage(core([self _frame]), forceBlackText);
> +
> +    Frame* coreFrame = core([self _frame]);
> +    if (!coreFrame)
> +	   return nil;
> +    return [createDragImageForSelection(*coreFrame,
forceBlackText).leakRef() autorelease];

The paragraphing here is strange. I would omit the blank line or include a
blank line after the second return nil.