[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 #48 from Brian Burg <burg at cs.washington.edu>  2013-11-24 18:49:55 PST ---
(From update of attachment 217767)
View in context: https://bugs.webkit.org/attachment.cgi?id=217767&action=review

Thanks for the feedback, I addressed everything in the next patch.

>> Source/WebCore/page/FrameSnapshotting.cpp:30
>> +#include "FrameSnapshotting.h"
> 
> In the WebKit coding style, there is no blank line between these includes.

Woops- filed an issue: https://bugs.webkit.org/show_bug.cgi?id=124821

>> Source/WebCore/page/FrameSnapshotting.cpp:47
>> +    , 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.

Woops- filed another issue: https://bugs.webkit.org/show_bug.cgi?id=124820

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

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

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

No particular reason; I was looking at flags in WebKit2, some of which seem to use uint32_t (WKFindOptions.h, ImageOptions.h).

>> Source/WebCore/platform/DragImage.h:50
>> +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.

I agree with your sentiment. Should I do so for this patch as well? It is probably late to matter much for this patch.

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