[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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #217917|review?, commit-queue?      |review+, commit-queue-
               Flag|                            |




--- Comment #56 from Darin Adler <darin at apple.com>  2013-12-04 11:38:45 PST ---
(From update of attachment 217917)
View in context: https://bugs.webkit.org/attachment.cgi?id=217917&action=review

This looks good but still substantial room for improvement.

> Source/WebCore/page/FrameSnapshotting.cpp:64
> +    Node* node;
> +    PaintBehavior paintBehavior;
> +    Color backgroundColor;

In a follow-up we could mark these data members const, and then we would not have to use const when using this type to define local variables below.

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

> Source/WebCore/page/FrameSnapshotting.cpp:83
> +        paintBehavior = PaintBehaviorSelectionOnly;

Here we are blowing away the old paint behavior. Is that what we intend? Or should this be |=?

> Source/WebCore/page/FrameSnapshotting.cpp:87
> +    frame.document()->updateLayout();

It seems like we should do this at the very top of the function, not here in the middle of setting up for painting.

> Source/WebCore/page/FrameSnapshotting.cpp:89
> +    float deviceScaleFactor = frame.page() ? frame.page()->deviceScaleFactor() : 1;

Should we just return nullptr when frame.page() is null instead? Maybe not, if we can eliminate the other reasons to return null.

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

> Source/WebCore/page/FrameSnapshotting.cpp:109
> +    // Force painting of only the selection area.
> +    options |= SnapshotOptionsPaintSelectionOnly;

Since this is setting the flag, I suspect the "=" instead of "|=" above is a typo, and I wonder why it’s not covered by tests.

Also, I don’t think the comment here adds anything; it says the same thing the code does. I suggest omitting it.

> Source/WebCore/page/FrameSnapshotting.cpp:111
> +    IntRect selectionRect = enclosingIntRect(frame.selection().bounds());
> +    return snapshotFrameRect(frame, selectionRect, options);

To me this would read better without the local variable. Maybe even without the local variable for options.

    return snapshotFrameRect(frame, enclosingIntRect(frame.selection().bounds()), options | SnapshotOptionsPaintSelectionOnly);

I like the way that looks.

> Source/WebCore/page/FrameSnapshotting.cpp:127
> +    IntRect paintingRect = pixelSnappedIntRect(node.renderer()->paintingRootRect(topLevelRect));
> +    return snapshotFrameRect(frame, paintingRect);

Local variable doesn’t add anything.

    return snapshotFrameRect(frame, pixelSnappedIntRect(node.renderer()->paintingRootRect(topLevelRect));

> Source/WebCore/page/FrameSnapshotting.h:33
> +#ifndef FrameSnapshotting_h
> +#define FrameSnapshotting_h
> +
> +namespace WebCore {

This needs to #include <memory> for std::unique_ptr.

> Source/WebCore/platform/DragImage.cpp:73
> +struct ScopedNodeDragState {

This should be ScopedNodeDragEnabler, since it’s not just state.

> Source/WebCore/platform/DragImage.cpp:78
> +        ASSERT(node.renderer());

Why is it correct to assert this? I see no obvious reason that we know this to be true.

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

> Source/WebCore/platform/DragImage.cpp:87
> +            frame.document()->updateLayout();

This seems wrong. Why would we calling updateLayout synchronously here?

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

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

> Source/WebCore/platform/DragImage.cpp:183
> +    SnapshotOptions options = forceBlackText ? SnapshotOptionsForceBlackText : SnapshotOptionsNone;
> +    options |= SnapshotOptionsPaintSelectionOnly;

Should be done as a single expression.

> Source/WebCore/platform/DragImage.cpp:188
> +    std::unique_ptr<ImageBuffer> snapshot = snapshotFrameRect(frame, view->selectionBounds(), options);
> +    return createDragImageFromSnapshot(std::move(snapshot), nullptr);

Would read better without the local variable.

> Source/WebCore/platform/DragImage.cpp:210
> +    std::unique_ptr<ImageBuffer> snapshot = snapshotNode(frame, node);
> +    return createDragImageFromSnapshot(std::move(snapshot), &node);

Would read better without the local variable.

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

> Source/WebKit/mac/WebView/WebHTMLView.mm:5977
> +    return [createDragImageForSelection(*coreFrame, forceBlackText).leakRef() autorelease];

Ditto.

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