[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





--- Comment #18 from Brian Burg <burg at cs.washington.edu>  2013-11-14 12:35:52 PST ---
Thanks for the quickness!

(In reply to comment #17)
> (From update of attachment 216956 [details])
> 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.

I kept with that convention because DragImage is return type (a typedef for the platform handle). However, most uses of that typedef have nothing to do with dragging either. Omitting 'Drag' would yield createImageFromFoo, but not return an Image. It's not clear how to easily incorporate the code into Image or ImageBuffer, nor is their functionality necessary for uses as far as I can tell. Maybe DragImage needs a better name? ImageHandle? I don't know the platform handle naming conventions well. It looks like NativeImagePtr is a bit similar, but put into smart pointers?

Only createDragImageFromImage() sets drag state before snapshotting. Others have various flags to make snapshots suitable for drag previews, but aren't always used that way.

> 
> > 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.

OK

> 
> > 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.
> 

If it's acceptable to set ImageBufferData.h and ImageBufferDataCG.h as private headers, then this class can be removed. createImageFromFrameRect() is used to create a DragImageRef (canvas path), and to get a data URL for inspector with a single API call. I'd prefer if there was a way to keep using that API.

> > 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.

Should DragImage be renamed? Or, split half of the image-creating functions based on how they are used elsewhere? I don't really have a preference.

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