[Webkit-unassigned] [Bug 124325] Consolidate and expose Frame/Node/Selection screenshot capabilities

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=124325


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #218467|review?, commit-queue?      |review+, commit-queue+
               Flag|                            |




--- Comment #61 from Darin Adler <darin at apple.com>  2013-12-04 18:37:26 PST ---
(From update of attachment 218467)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.