[webkit-reviews] review denied: [Bug 124325] Consolidate and expose Frame/Node/Selection screenshot capabilities : [Attachment 217767] v3 - fix TestWebKitAPI Regressions

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Nov 24 17:11:32 PST 2013


Darin Adler <darin 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 217767: v3 - fix TestWebKitAPI Regressions
https://bugs.webkit.org/attachment.cgi?id=217767&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=217767&action=review


Thanks for tackling this. It seems to generally be a step in the right
direction.

I did a basic review but I did not read everything carefully.

How did you test?

> Source/WebCore/bindings/objc/DOM.mm:290
> +    if (!frame || !node)

Don’t add this worthless null check. We just dereferenced node on the line
above, so there’s no way we could get here if it was null.

> Source/WebCore/bindings/objc/DOM.mm:292
> +    return [[createDragImageForNode(*frame, *node) retain] autorelease];

If the return type is RetainPtr, then the correct idiom would be
leakRef/autorelease, not retain/autorelease.

> Source/WebCore/bindings/objc/DOM.mm:335
> +    if (!frame || !range)

Don’t add this worthless null check. We just dereferenced range on the line
above, so there’s no way we could get here if it was null.

> Source/WebCore/bindings/objc/DOM.mm:338
> +    return [[createDragImageForRange(*frame, *range, forceBlackText) retain]
autorelease];

Ditto.

> Source/WebCore/page/FrameSnapshotting.cpp:2
> + *  Copyright (C) 2013 University of Washington. All rights reserved.

Most of this file is already-written code that already has a copyright. Fine to
add your copyright if you wrote enough new code, but the original copyright
needs be listed here too; you must not claim copyright on code just because you
moved it to a new file.

> Source/WebCore/page/FrameSnapshotting.cpp:30
> +#include "config.h"
> +
> +#include "FrameSnapshotting.h"

In the WebKit coding style, there is no blank line between these includes.

> Source/WebCore/page/FrameSnapshotting.cpp:43
> +    ScopedFramePaintingState(Frame& frame, Node* node)

Seems silly that we use a single class for both the "with node" and "without
node" cases. Should just have a derived class that adds the node. But not new
to your patch.

> Source/WebCore/page/FrameSnapshotting.cpp:47
> +    : frame(frame)
> +    , node(node)
> +    , paintBehavior(frame.view()->paintBehavior())
> +    , backgroundColor(frame.view()->baseBackgroundColor())

In WebKit coding style there are indented one tab stop further in, as they were
in the place you copied this code from.

> Source/WebCore/page/FrameSnapshotting.cpp:81
> +    PaintBehavior textPaintBehavior = 0;
> +    if (options & SnapshotOptionsForceBlackText)
> +	   textPaintBehavior = PaintBehaviorForceBlackText;
> +
> +    PaintBehavior selectionOnlyBehavior = 0;
> +    if (options & SnapshotOptionsPaintSelectionOnly)
> +	   textPaintBehavior = PaintBehaviorSelectionOnly;

Using two separate local variables for these additional flags is a strange
idiom. I would write this:

    PaintBehavior paintBehavior = state.paintBehavior;
    if (options & SnapshotOptionsForceBlackText)
	paintBehavior |= PaintBehaviorForceBlackText;
    if (options & SnapshotOptionsPaintSelectionOnly)
	paintBehavior |= PaintBehaviorSelectionOnly;
    frame.view()->setPaintBehavior(paintBehavior);

The version with three local variables is less clear and there is no compelling
reason to paragraph the setPaintBehavior call along with the updateLayout call.
In fact, it seems it would be better to call updateLayout considerably earlier
in the function; I don’t understand why it’s done after
ScopedFramePaintingState.

> Source/WebCore/page/FrameSnapshotting.cpp:94
> +    std::unique_ptr<ImageBuffer> buffer =
std::unique_ptr<ImageBuffer>(ImageBuffer::create(usedRect.size(),
deviceScaleFactor, ColorSpaceDeviceRGB).leakPtr());

We have gone out of our way to avoid unique_ptr/leakPtr pairs in WebKit up
until now. Could you do a patch that changes ImageBuffer::create to return a
unique_ptr first to prepare for this and avoid this problem?

> Source/WebCore/page/FrameSnapshotting.cpp:126
> +    // Document::updateLayout may have blown away the original renderer.

A far better way to deal with this would be to call updateLayout at the top of
the function, before null checking node.renderer() in the first place!

> Source/WebCore/page/FrameSnapshotting.h:30
> +#include <wtf/Forward.h>

I don’t see any reason to include this header.

> Source/WebCore/page/FrameSnapshotting.h:46
> +typedef uint32_t SnapshotOptions;

Most other cases of flags like this in WebKit use the type unsigned; for
example, FindOptions in FindOptions.h, and many other types with the suffix
“Flags”. Any good reason to use the type uint32_t here instead?

> Source/WebCore/platform/DragImage.cpp:76
> +    : frame(frame)
> +    , node(node)

Wrong formatting for WebKit.

> Source/WebCore/platform/DragImage.h:50
> -    class Image;
> -    class URL;
> +class Frame;

Reindenting the whole file to make the style bot happy at the same time you
make substantive changes to the file is not good, because the patch reviewer
can’t easily see what the changes are. Please consider submitting a “reindent”
patch first. That’s what people have done in similar situations before.

> Source/WebKit/mac/WebView/WebHTMLView.mm:1828
> +    NSImage *dragImage = [[createDragImageForSelection(*coreFrame) retain]
autorelease];

If the return type is RetainPtr, then the correct idiom would be
leakRef/autorelease, not retain/autorelease.

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

Ditto.


More information about the webkit-reviews mailing list