[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 #217767|review?                     |review-
               Flag|                            |




--- Comment #47 from Darin Adler <darin at apple.com>  2013-11-24 17:10:02 PST ---
(From update of attachment 217767)
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.

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