[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


Simon Fraser (smfr) <simon.fraser at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #216956|review?                     |review-
               Flag|                            |




--- Comment #17 from Simon Fraser (smfr) <simon.fraser at apple.com>  2013-11-14 11:06:56 PST ---
(From update of attachment 216956)
View in context: https://bugs.webkit.org/attachment.cgi?id=216956&action=review

I think we should keep FrameSnapshotting.cpp and put most of the cross-platform snapshotting code there. I'm not convinced that FrameSnapshot needs to exist.

> Source/WebCore/bindings/objc/DOM.mm:338
> +    return [[createDragImageForRange(*frame, range, forceBlackText) retain] autorelease];

I don't think "createDragImageForRange" is a good name for the generic image creation function, since it's confusing to see a function called renderedImageForcingBlackText: using a drag image.

> Source/WebCore/page/FrameSnapshot.h:41
> +OwnPtr<ImageBuffer> createImageFromFrameRect(Frame&, const IntRect&, bool includeSelection = true, bool useViewCoordinates = true);

You should use an enum rather than bools to avoid call points being hard to read.

> Source/WebCore/page/FrameSnapshot.h:44
> +class FrameSnapshot {

I don't think FrameSnapshot is a good name. It sounds like it's creating a snapshot for an entire frame.

> Source/WebCore/page/FrameSnapshot.h:48
> +    FrameSnapshot(OwnPtr<ImageBuffer>);
> +    ~FrameSnapshot();
> +    static std::unique_ptr<FrameSnapshot> createFromRect(Frame&, const IntRect&, bool includeSelection = true, bool useViewCoordinates = true);

It's odd to create FrameSnapshot with an imagebuffer, but then allow createFromRect() to be called more than once. Why not pass the ImageBuffer into createFromRect()?

> Source/WebCore/page/FrameSnapshot.h:50
> +    String toDataURL(const String& mimeType, double* quality = nullptr);

Doesn't seem worth having.

> Source/WebCore/platform/DragImage.cpp:96
> +DragImageRef createDragImageForRange(Frame& frame, Range* range, bool forceBlackText)
> +{
> +    frame.view()->setPaintBehavior(PaintBehaviorSelectionOnly | (forceBlackText ? PaintBehaviorForceBlackText : 0));
> +    frame.document()->updateLayout();
> +    RenderView* view = frame.contentRenderer();
> +    if (!view)
> +        return nullptr;
> +
> +    Position start = range->startPosition();
> +    Position candidate = start.downstream();
> +    if (candidate.deprecatedNode() && candidate.deprecatedNode()->renderer())
> +        start = candidate;
> +
> +    Position end = range->endPosition();
> +    candidate = end.upstream();
> +    if (candidate.deprecatedNode() && candidate.deprecatedNode()->renderer())
> +        end = candidate;
> +
> +    if (start.isNull() || end.isNull() || start == end)
> +        return nullptr;
> +
> +    RenderObject* savedStartRenderer;

I don't think all the new code belongs in something called DragImage, since it's used for much more than creating drag images.

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