[webkit-reviews] review denied: [Bug 124325] Consolidate and expose Frame/Node/Selection screenshot capabilities : [Attachment 216956] v2.0

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied 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 216956: v2.0
https://bugs.webkit.org/attachment.cgi?id=216956&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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.