[Webkit-unassigned] [Bug 124325] Consolidate and expose Frame/Node/Selection screenshot capabilities
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Nov 14 11:08:19 PST 2013
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.
More information about the webkit-unassigned
mailing list