Various screenshot collection code is spread between Frame, FrameSnapshotting, and platform-specific implementations for each. We should have all of this code in one place, preferably DragImage (as almost all uses of screenshots within WebKit are for this purpose). For inspector clients and other WK/WK2 code to encode these screenshots with the platform-specific (and probably faster) encoder provided by ImageBuffer::toDataURL, we need a thin wrapper to break dependencies on "ImageBuffer.h" as well. (Patch forthcoming..)
Created attachment 216888 [details] v1
Comment on attachment 216888 [details] v1 Attachment 216888 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/22700246
Comment on attachment 216888 [details] v1 Attachment 216888 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/21250192
Comment on attachment 216888 [details] v1 Attachment 216888 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/22540138
Comment on attachment 216888 [details] v1 Attachment 216888 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/22899212
Comment on attachment 216888 [details] v1 Attachment 216888 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/22780113
Comment on attachment 216888 [details] v1 Attachment 216888 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/22640223
Created attachment 216938 [details] v1.1
Comment on attachment 216938 [details] v1.1 Attachment 216938 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/21908028
Created attachment 216943 [details] v1.2
Some test failures, investigating..
Comment on attachment 216943 [details] v1.2 View in context: https://bugs.webkit.org/attachment.cgi?id=216943&action=review > Source/WebCore/bindings/objc/DOM.mm:292 > - return frame->nodeImage(node).get(); > + return [createDragImageForNode(*frame, node) autorelease]; You need to [[createDragImageForFrameSelection(…) retain] autorelease]. right now you are over releasing. I am surprised it worked before without a retain/autorelease.
Comment on attachment 216943 [details] v1.2 View in context: https://bugs.webkit.org/attachment.cgi?id=216943&action=review >> Source/WebCore/bindings/objc/DOM.mm:292 >> + return [createDragImageForNode(*frame, node) autorelease]; > > You need to [[createDragImageForFrameSelection(…) retain] autorelease]. right now you are over releasing. I am surprised it worked before without a retain/autorelease. I meant createDragImageForNode in this case, but this needs fixed everywhere you do create and autorelease.
Created attachment 216956 [details] v2.0
(In reply to comment #13) > (From update of attachment 216943 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=216943&action=review > > >> Source/WebCore/bindings/objc/DOM.mm:292 > >> + return [createDragImageForNode(*frame, node) autorelease]; > > > > You need to [[createDragImageForFrameSelection(…) retain] autorelease]. right now you are over releasing. I am surprised it worked before without a retain/autorelease. > > I meant createDragImageForNode in this case, but this needs fixed everywhere you do create and autorelease. Addressed these. Only the botched one in Clipboard.cpp was causing test crashes, so thanks for pointing it out.
Comment on attachment 216956 [details] v2.0 View in context: https://bugs.webkit.org/attachment.cgi?id=216956&action=review > Source/WebCore/platform/DragImage.cpp:202 > + paintingRect.setWidth(paintingRect.width() * deviceScaleFactor); > + paintingRect.setHeight(paintingRect.height() * deviceScaleFactor); paintingRect.scale(deviceScaleFactor); > Source/WebCore/platform/DragImage.cpp:216 > +DragImageRef createDragImageForNode(Frame& frame, Node* node) Wonder if there’s not more code that can be factored out of these two. > Source/WebCore/platform/DragImage.cpp:242 > + paintingRect.setWidth(paintingRect.width() * deviceScaleFactor); > + paintingRect.setHeight(paintingRect.height() * deviceScaleFactor); paintingRect.scale(deviceScaleFactor);
Comment on attachment 216956 [details] v2.0 View in context: https://bugs.webkit.org/attachment.cgi?id=216956&action=review I think we should keep FrameSnapshotting.cpp and put most of the cross-platform snapshotting code there. I'm not convinced that FrameSnapshot needs to exist. > Source/WebCore/bindings/objc/DOM.mm:338 > + return [[createDragImageForRange(*frame, range, forceBlackText) retain] autorelease]; I don't think "createDragImageForRange" is a good name for the generic image creation function, since it's confusing to see a function called renderedImageForcingBlackText: using a drag image. > Source/WebCore/page/FrameSnapshot.h:41 > +OwnPtr<ImageBuffer> createImageFromFrameRect(Frame&, const IntRect&, bool includeSelection = true, bool useViewCoordinates = true); You should use an enum rather than bools to avoid call points being hard to read. > Source/WebCore/page/FrameSnapshot.h:44 > +class FrameSnapshot { I don't think FrameSnapshot is a good name. It sounds like it's creating a snapshot for an entire frame. > Source/WebCore/page/FrameSnapshot.h:48 > + FrameSnapshot(OwnPtr<ImageBuffer>); > + ~FrameSnapshot(); > + static std::unique_ptr<FrameSnapshot> createFromRect(Frame&, const IntRect&, bool includeSelection = true, bool useViewCoordinates = true); It's odd to create FrameSnapshot with an imagebuffer, but then allow createFromRect() to be called more than once. Why not pass the ImageBuffer into createFromRect()? > Source/WebCore/page/FrameSnapshot.h:50 > + String toDataURL(const String& mimeType, double* quality = nullptr); Doesn't seem worth having. > Source/WebCore/platform/DragImage.cpp:96 > +DragImageRef createDragImageForRange(Frame& frame, Range* range, bool forceBlackText) > +{ > + frame.view()->setPaintBehavior(PaintBehaviorSelectionOnly | (forceBlackText ? PaintBehaviorForceBlackText : 0)); > + frame.document()->updateLayout(); > + RenderView* view = frame.contentRenderer(); > + if (!view) > + return nullptr; > + > + Position start = range->startPosition(); > + Position candidate = start.downstream(); > + if (candidate.deprecatedNode() && candidate.deprecatedNode()->renderer()) > + start = candidate; > + > + Position end = range->endPosition(); > + candidate = end.upstream(); > + if (candidate.deprecatedNode() && candidate.deprecatedNode()->renderer()) > + end = candidate; > + > + if (start.isNull() || end.isNull() || start == end) > + return nullptr; > + > + RenderObject* savedStartRenderer; I don't think all the new code belongs in something called DragImage, since it's used for much more than creating drag images.
Thanks for the quickness! (In reply to comment #17) > (From update of attachment 216956 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=216956&action=review > > I think we should keep FrameSnapshotting.cpp and put most of the cross-platform snapshotting code there. I'm not convinced that FrameSnapshot needs to exist. > > > Source/WebCore/bindings/objc/DOM.mm:338 > > + return [[createDragImageForRange(*frame, range, forceBlackText) retain] autorelease]; > > I don't think "createDragImageForRange" is a good name for the generic image creation function, since it's confusing to see a function called renderedImageForcingBlackText: using a drag image. I kept with that convention because DragImage is return type (a typedef for the platform handle). However, most uses of that typedef have nothing to do with dragging either. Omitting 'Drag' would yield createImageFromFoo, but not return an Image. It's not clear how to easily incorporate the code into Image or ImageBuffer, nor is their functionality necessary for uses as far as I can tell. Maybe DragImage needs a better name? ImageHandle? I don't know the platform handle naming conventions well. It looks like NativeImagePtr is a bit similar, but put into smart pointers? Only createDragImageFromImage() sets drag state before snapshotting. Others have various flags to make snapshots suitable for drag previews, but aren't always used that way. > > > Source/WebCore/page/FrameSnapshot.h:41 > > +OwnPtr<ImageBuffer> createImageFromFrameRect(Frame&, const IntRect&, bool includeSelection = true, bool useViewCoordinates = true); > > You should use an enum rather than bools to avoid call points being hard to read. OK > > > Source/WebCore/page/FrameSnapshot.h:44 > > +class FrameSnapshot { > > I don't think FrameSnapshot is a good name. It sounds like it's creating a snapshot for an entire frame. > > > Source/WebCore/page/FrameSnapshot.h:48 > > + FrameSnapshot(OwnPtr<ImageBuffer>); > > + ~FrameSnapshot(); > > + static std::unique_ptr<FrameSnapshot> createFromRect(Frame&, const IntRect&, bool includeSelection = true, bool useViewCoordinates = true); > > It's odd to create FrameSnapshot with an imagebuffer, but then allow createFromRect() to be called more than once. Why not pass the ImageBuffer into createFromRect()? > > > > Source/WebCore/page/FrameSnapshot.h:50 > > + String toDataURL(const String& mimeType, double* quality = nullptr); > > Doesn't seem worth having. > If it's acceptable to set ImageBufferData.h and ImageBufferDataCG.h as private headers, then this class can be removed. createImageFromFrameRect() is used to create a DragImageRef (canvas path), and to get a data URL for inspector with a single API call. I'd prefer if there was a way to keep using that API. > > Source/WebCore/platform/DragImage.cpp:96 > > +DragImageRef createDragImageForRange(Frame& frame, Range* range, bool forceBlackText) > > +{ > > + frame.view()->setPaintBehavior(PaintBehaviorSelectionOnly | (forceBlackText ? PaintBehaviorForceBlackText : 0)); > > + frame.document()->updateLayout(); > > + RenderView* view = frame.contentRenderer(); > > + if (!view) > > + return nullptr; > > + > > + Position start = range->startPosition(); > > + Position candidate = start.downstream(); > > + if (candidate.deprecatedNode() && candidate.deprecatedNode()->renderer()) > > + start = candidate; > > + > > + Position end = range->endPosition(); > > + candidate = end.upstream(); > > + if (candidate.deprecatedNode() && candidate.deprecatedNode()->renderer()) > > + end = candidate; > > + > > + if (start.isNull() || end.isNull() || start == end) > > + return nullptr; > > + > > + RenderObject* savedStartRenderer; > > I don't think all the new code belongs in something called DragImage, since it's used for much more than creating drag images. Should DragImage be renamed? Or, split half of the image-creating functions based on how they are used elsewhere? I don't really have a preference.
I would like nodeImage, selectionImage, and imageFromRect to be cross-platform and used by the inspector. I propose these methods belong in FrameSnapshotting and deal with ImageBuffers or Images*. The other methods that are only used for dragging should go in DragImage. So, here's a plan for that: 1. Move Frame::nodeImage to FrameSnapshotting. 2. Delete Frame::dragImageForSelection, as it just called selectionImage(). 3. Move snapshotDragImage to DragImage[Mac].h It's not useful for anything else. 4. Change FrameSnapshotting::{selectionImage, rangeImage, nodeImage, imageFromRect} to return ImageBuffers. 5. Optionally rename FrameSnapshotting methods to 'createSnapshotForFoo' (a la the create rule) or 'snapshotFooImage' (to make snapshot the relevant verb). Let me know if this is more sensible than the current approach. * In a second patch, InspectorClient will use these APIs to create screenshots of nodes, rects, etc and turn them into data urls. This will require using ImageBuffer from WebKit/WebKit2. If this is a layering violation, then the APIs can use Image instead.
Created attachment 217137 [details] v2.0
Attachment 217137 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/objc/DOM.mm', u'Source/WebCore/dom/Clipboard.cpp', u'Source/WebCore/dom/ClipboardMac.mm', u'Source/WebCore/page/DragController.cpp', u'Source/WebCore/page/Frame.cpp', u'Source/WebCore/page/Frame.h', u'Source/WebCore/page/FrameSnapshotting.cpp', u'Source/WebCore/page/FrameSnapshotting.h', u'Source/WebCore/page/mac/FrameMac.mm', u'Source/WebCore/page/mac/FrameSnapshottingMac.h', u'Source/WebCore/page/mac/FrameSnapshottingMac.mm', u'Source/WebCore/page/win/FrameWin.cpp', u'Source/WebCore/page/win/FrameWin.h', u'Source/WebCore/platform/DragImage.cpp', u'Source/WebCore/platform/DragImage.h', u'Source/WebKit/ios/ChangeLog', u'Source/WebKit/ios/WebCoreSupport/WebFrameIOS.mm', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebView/WebHTMLView.mm', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/DOMCoreClasses.cpp']" exit_code: 1 Source/WebCore/platform/DragImage.cpp:143: The parameter name "view" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/page/FrameSnapshotting.cpp:37: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 217138 [details] v2.0 (appease style bot)
Comment on attachment 217138 [details] v2.0 (appease style bot) Attachment 217138 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/25378022
New patch up. Not sure what's up with win-ewe build fail, doesn't look related?
(In reply to comment #24) > New patch up. Not sure what's up with win-ewe build fail, doesn't look related? It is absolutely related, win ews build logs are just completely painful to peruse. You need to update Windows exports: Creating library C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\lib32\WebKit.lib and object C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\lib32\WebKit.exp 1>WebCore.lib(DragImage.obj) : error LNK2019: unresolved external symbol "class WTF::OwnPtr<class WebCore::ImageBuffer> __cdecl WebCore::snapshotNode(class WebCore::Frame &,class WebCore::Node *)" (?snapshotNode@WebCore@@YA?AV?$OwnPtr@VImageBuffer@WebCore@@@WTF@@AAVFrame@1@PAVNode@1@@Z) referenced in function "struct HBITMAP__ * __cdecl WebCore::createDragImageForNode(class WebCore::Frame &,class WebCore::Node *)" (?createDragImageForNode@WebCore@@YAPAUHBITMAP__@@AAVFrame@1@PAVNode@1@@Z) 1>WebCore.lib(DragImage.obj) : error LNK2019: unresolved external symbol "class WTF::OwnPtr<class WebCore::ImageBuffer> __cdecl WebCore::snapshotSelection(class WebCore::Frame &,unsigned int)" (?snapshotSelection@WebCore@@YA?AV?$OwnPtr@VImageBuffer@WebCore@@@WTF@@AAVFrame@1@I@Z) referenced in function "struct HBITMAP__ * __cdecl WebCore::createDragImageForSelection(class WebCore::Frame &,bool)" (?createDragImageForSelection@WebCore@@YAPAUHBITMAP__@@AAVFrame@1@_N@Z) 1>C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\bin32\WebKit.dll : fatal error LNK1120: 2 unresolved externals 1>Done Building Project "C:\cygwin\home\buildbot\WebKit\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj" (build target(s)) -- FAILED.
(In reply to comment #25) > (In reply to comment #24) > > New patch up. Not sure what's up with win-ewe build fail, doesn't look related? > > It is absolutely related, win ews build logs are just completely painful to peruse. > > You need to update Windows exports: > Oh touché, I totally missed that. Is there any rule for ordering the win exports (aside from feature flags)?
Comment on attachment 217138 [details] v2.0 (appease style bot) View in context: https://bugs.webkit.org/attachment.cgi?id=217138&action=review Overall, looks great. r=me with the changes. > Source/WebCore/page/FrameSnapshotting.h:44 > +enum { > + SnapshotOptionsExcludeSelectionHighlighting = 1 << 1, > + SnapshotOptionsInViewCoordinates = 1 << 2, > + SnapshotOptionsForceBlackText = 1 << 3, > +}; > +typedef uint32_t SnapshotOptions; This is C++, so this should probably use enum SnapshotOptions { ... } instead of the typedef. > Source/WebCore/page/FrameSnapshotting.h:47 > +OwnPtr<ImageBuffer> snapshotNode(Frame&, Node*); This should take the Node by reference. > Source/WebCore/page/FrameSnapshotting.h:48 > +OwnPtr<ImageBuffer> snapshotFrameRect(Frame&, const IntRect&, SnapshotOptions = 0); > +OwnPtr<ImageBuffer> snapshotNode(Frame&, Node*); > +OwnPtr<ImageBuffer> snapshotSelection(Frame&, SnapshotOptions = 0); These should return std::unique_ptr<ImageBuffer>
Created attachment 217155 [details] v2.1 (missing win exports)
Comment on attachment 217155 [details] v2.1 (missing win exports) Attachment 217155 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/27248027
Comment on attachment 217155 [details] v2.1 (missing win exports) Attachment 217155 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/25598061
Created attachment 217157 [details] v2.2 (missing win exports)
(In reply to comment #27) > > Source/WebCore/page/FrameSnapshotting.h:44 > > +enum { > > + SnapshotOptionsExcludeSelectionHighlighting = 1 << 1, > > + SnapshotOptionsInViewCoordinates = 1 << 2, > > + SnapshotOptionsForceBlackText = 1 << 3, > > +}; > > +typedef uint32_t SnapshotOptions; > > This is C++, so this should probably use enum SnapshotOptions { ... } instead of the typedef. I made the other changes but not this one. Since these are not mutually exclusive options, I did the same thing as elsewhere for this circumstance (i.e. WebKit2/shared/ImageOptions.h). Making the values strongly-typed prevents masking out some options, such as forcing selection painting for snapshotSelection(). New win export symbols are coming when the bots catch up.
Comment on attachment 217157 [details] v2.2 (missing win exports) Attachment 217157 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/25618051
Created attachment 217159 [details] v2.2
Comment on attachment 217159 [details] v2.2 Attachment 217159 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/24028221
Created attachment 217162 [details] v2.3
Comment on attachment 217162 [details] v2.3 View in context: https://bugs.webkit.org/attachment.cgi?id=217162&action=review > Source/WebCore/page/FrameSnapshotting.cpp:86 > + float deviceScaleFactor = frame.page() ? 1 : frame.page()->deviceScaleFactor(); Is this backwards? I would expect that If there is a page this should be the page's deviceScaleFactor, otherwise 1.
Comment on attachment 217162 [details] v2.3 View in context: https://bugs.webkit.org/attachment.cgi?id=217162&action=review >> Source/WebCore/page/FrameSnapshotting.cpp:86 >> + float deviceScaleFactor = frame.page() ? 1 : frame.page()->deviceScaleFactor(); > > Is this backwards? I would expect that If there is a page this should be the page's deviceScaleFactor, otherwise 1. Yes, the old code was: float deviceScaleFactor = 1; if (m_page) deviceScaleFactor = m_page->deviceScaleFactor();
Created attachment 217223 [details] v2.4
Comment on attachment 217223 [details] v2.4 Clearing flags on attachment: 217223 Committed r159455: <http://trac.webkit.org/changeset/159455>
All reviewed patches have been landed. Closing bug.
This patch broke two API tests; see https://bugs.webkit.org/show_bug.cgi?id=124564. Please look into this ASAP; I'll roll out tomorrow if it hasn't been fixed.
Re-opened since this is blocked by bug 124568
I didn't realize TestWebKitAPI was not apart of EWS bots.. :( I'm investigating API test failures, should not be too hard. I think some of the old methods assumed different defaults (w.r.t. selection inclusion) than are implemented by FrameSnapshotting methods.
(In reply to comment #44) > I didn't realize TestWebKitAPI was not apart of EWS bots.. :( There's a bug somewhere to make that happen, but they're not at the moment. You should always be running the tests locally anyway, especially for a patch of this size.
Created attachment 217767 [details] v3 - fix TestWebKitAPI Regressions
Comment on attachment 217767 [details] v3 - fix TestWebKitAPI Regressions 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.
Comment on attachment 217767 [details] v3 - fix TestWebKitAPI Regressions 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.
(In reply to comment #48) > > 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. Maybe others were able to review the changes to DragImage.h despite the re-indenting, but I didn’t really manage to do that. Since it seems others have reviewed I suppose we are OK.
Created attachment 217774 [details] v4
Created attachment 217916 [details] copy of last attachment, this time for EWS with deps landed (dup for EWS' sake) Patch addresses comments from Darin Adler; rebase on top of r159791.
Created attachment 217917 [details] v4 - an actual patch, not the url
Comment on attachment 217917 [details] v4 - an actual patch, not the url View in context: https://bugs.webkit.org/attachment.cgi?id=217917&action=review > Source/WebCore/bindings/objc/DOM.mm:338 > + return [createDragImageForRange(*frame, *range, forceBlackText).leakRef() autorelease]; createDragImageForRange is the wrong name for that function; it's nothing to do with dragging at many call sites.
(In reply to comment #53) > > + return [createDragImageForRange(*frame, *range, forceBlackText).leakRef() autorelease]; > > createDragImageForRange is the wrong name for that function; it's nothing to do with dragging at many call sites. The phrase DragImage here is simply repeating the name of the type. We could rename the type and the function at the same time but given the current name of the type I think the function name is good.
Comment on attachment 217917 [details] v4 - an actual patch, not the url I agree. We can rename the DragImage functions and types in a follow up change, which Brian filed as bug 125200.
Comment on attachment 217917 [details] v4 - an actual patch, not the url 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.
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
(In reply to comment #57) > > > > 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? Looking deeper, I think this should be left alone. Refactoring it without also refactoring FrameSelection::setSelection (i.e., what it's emulating) is just going to lead to unexpected differences and test failures. And I have no desire to learn (god forbid, touch) that part of layout to land snapshot functionality for the inspector.
Created attachment 218467 [details] v5
(In reply to comment #57) > > > 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? Doesn’t seem to me like something to be mentioned in the style guide, but maybe. We have plenty of functions that return null when they fail, but generally that is for failures other than memory exhaustion. > > > 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? Good point. We do need to update layout after setting the drag state. I now see that this was mentioned in an existing comment that this patch deletes. // forces style recalc - needed since changing the drag state might // imply new styles, plus JS could have changed other things Not a great comment, but did answer the question that came to mind when I read the code. It’s good to have “why” comments--those are the ones we encourage in WebKit. I think a comment that explicitly mentions that drag state can affect style would be good to add. It’s actually quite strange to have a flag in a renderer that can affect style, because normally the architecture is that DOM plus style is used to create renderers. Having the renderer state also affect style is a sort of “circular” concept that doesn’t fit too well and because of that I think this is something that eventually we’ll want to fix by moving the state out of the renderer. But it’s not right to call node.renderer() without first calling updateLayout(), so I we need additional calls to updateLayout() in a few places: 1) at the start of the snapshotNode function 2) here in ScopedNodeDragEnabler’s constructor, just before calling node.renderer() 3) in createDragImageFromSnapshot, in the CSS_IMAGE_ORIENTATION code 4) at the start of the createDragImageForImage, although maybe we can just rely on the side effect from ScopedNodeDragEnabler > > > 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.) I think the issue here is that the guarantee that there is no layout is far away from this helper class. It’s fragile to have the assumptions relatively far away from the code that makes the assumption valid. But I think it’s just a coding style issue, not an actual bug.
Comment on attachment 218467 [details] v5 View in context: https://bugs.webkit.org/attachment.cgi?id=218467&action=review > Source/WebCore/page/FrameSnapshotting.cpp:113 > + options |= SnapshotOptionsPaintSelectionOnly; > + return snapshotFrameRect(frame, enclosingIntRect(frame.selection().bounds()), options); Awkward to modify the options argument with |= rather than just using | in the function call. return snapshotFrameRect(frame, enclosingIntRect(frame.selection().bounds()), options | SnapshotOptionsPaintSelectionOnly); > Source/WebCore/platform/DragImage.h:46 > -//We need to #define YOffset as it needs to be shared with WebKit > +// We need to #define YOffset as it needs to be shared with WebKit > #define DragLabelBorderYOffset 2 I don’t understand this comment that we are reformatting. How on earth does defining DragLabelBorderYOffset (not YOffset) allow it to be “shared with WebKit”. > Source/WebCore/platform/DragImage.h:65 > #elif PLATFORM(WIN) > - typedef HBITMAP DragImageRef; > +typedef HBITMAP DragImageRef; > #elif PLATFORM(GTK) || PLATFORM(NIX) > - typedef cairo_surface_t* DragImageRef; > +typedef cairo_surface_t* DragImageRef; > #elif PLATFORM(EFL) || PLATFORM(BLACKBERRY) > - typedef void* DragImageRef; > +typedef void* DragImageRef; > #endif We should really change all of these to use some kind of object or smart pointer, so we don’t need deleteDragImage. > Source/WebKit/mac/WebView/WebHTMLView.mm:5977 > if (![self _hasSelection]) > return nil; > - return selectionImage(core([self _frame]), forceBlackText); > + > + Frame* coreFrame = core([self _frame]); > + if (!coreFrame) > + return nil; > + return [createDragImageForSelection(*coreFrame, forceBlackText).leakRef() autorelease]; The paragraphing here is strange. I would omit the blank line or include a blank line after the second return nil.
Comment on attachment 218467 [details] v5 Clearing flags on attachment: 218467 Committed r160152: <http://trac.webkit.org/changeset/160152>
This seems to have totally broken the selection dragging snapshot on Mac...
(In reply to comment #64) > This seems to have totally broken the selection dragging snapshot on Mac... Filed https://bugs.webkit.org/show_bug.cgi?id=125375