[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 #57 from Brian Burg <burg at cs.washington.edu>  2013-12-04 13:29:25 PST ---
Thanks for the additional comments. Everything unquoted will be fixed.

(In reply to comment #56)
> (From update of attachment 217917 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217917&action=review
> 
> > Source/WebCore/page/FrameSnapshotting.cpp:75
> > +    FrameView::SelectionInSnapshot shouldIncludeSelection = FrameView::IncludeSelection;
> > +    if (options & SnapshotOptionsExcludeSelectionHighlighting)
> > +        shouldIncludeSelection = FrameView::ExcludeSelection;
> > +
> > +    FrameView::CoordinateSpaceForSnapshot coordinateSpace = FrameView::DocumentCoordinates;
> > +    if (options & SnapshotOptionsInViewCoordinates)
> > +        coordinateSpace = FrameView::ViewCoordinates;
> 
> I would have written these with ? : instead of an if statement.

I kept with the style used by other recently-added enum arguments. I prefer it as written, so that someone can tell what the default setting is without scrolling their editor rightward. Without any line length/enum conversion pattern recommendation in the style guide, I have no way to reconcile contradictory style nits.

> > Source/WebCore/page/FrameSnapshotting.cpp:95
> > +    std::unique_ptr<ImageBuffer> buffer = ImageBuffer::create(usedRect.size(), deviceScaleFactor, ColorSpaceDeviceRGB);
> > +    if (!buffer)
> > +        return nullptr;
> 
> Does ImageBuffer::create really return null when it fails? I am surprised? Normally if we can’t allocate memory we crash instead of returning null unless the function is named “try”.

Yes, it does. Should this be added to the style guide?

> > Source/WebCore/page/FrameSnapshotting.h:33
> > +#ifndef FrameSnapshotting_h
> > +#define FrameSnapshotting_h
> > +
> > +namespace WebCore {
> 
> This needs to #include <memory> for std::unique_ptr.

Should this always be the case? If so, please comment here: https://bugs.webkit.org/show_bug.cgi?id=125247

> > Source/WebCore/platform/DragImage.cpp:80
> > +        frame.document()->updateLayout();
> 
> It doesn’t make sense to do this after using node.renderer(). It needs to be done *before* calling node.renderer().

Perhaps the original author was confused as to whether setting drag state entailed any layout invalidation. I was under the impression that it would trigger changes because of -webkit-drag styles. Is this incorrect?

> > Source/WebCore/platform/DragImage.cpp:123
> > +    std::unique_ptr<ImageBuffer> snapshot = snapshotNode(frame, node);
> > +    return createDragImageFromSnapshot(std::move(snapshot), &node);
> 
> This would read better without the local variable:
> 
>     return createDragImageFromSnapshot(snapshotNode(frame, node), &node);
> 
> > Source/WebCore/platform/DragImage.cpp:144
> > +        if (RenderView* root = frame.contentRenderer())
> > +            root->setSelection(startRenderer, startOffset, endRenderer, endOffset, RenderView::RepaintNothing);
> 
> This doesn’t seem right. What if contentRenderer now returns non-null but in the constructor it returned null? Then we’ll be setting the selection to random values!

Why would it become null? There is no layout run between construction and destruction. (I'm not aware of any other way to serialize the RenderView's selection.)

> > Source/WebCore/platform/DragImage.cpp:178
> > +    RenderObject* startRenderer = start.deprecatedNode()->renderer();
> > +    RenderObject* endRenderer = end.deprecatedNode()->renderer();
> 
> I don’t think it’s right for this function to be using deprecatedNode at all. That needs to be fixed.

OK. I didn't write this code, so I'm unfamiliar with Position or how to handle deprecatedNode. What are the correct replacements? Is there a better approach to accomplishing what this function does?

> 
> > Source/WebKit/mac/WebView/WebHTMLView.mm:1828
> > +    NSImage *dragImage = [createDragImageForSelection(*coreFrame).leakRef() autorelease];
> 
> We should add a leakRef/autorelease function to RetainPtr itself. Doing it like this directly is unnecessarily low level and risky!

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

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