Mac side of https://bugs.webkit.org/show_bug.cgi?id=35811 .
Created attachment 52503 [details] Patch. This implements things on OS X. However, I hit a DCHECK, so this isn't ready for review. It sounds like the patch on the win/linux side has this problem as well, so the OS X patch in this bug is probably fine. ASSERTION FAILED: !m_doingDragAndDrop (/Users/thakis/src/chrome-git/src/third_party/WebKit/WebKit/chromium/src/WebViewImpl.cpp:1943 void WebKit::WebViewImpl::startDragging(const WebKit::WebDragData&, WebKit::WebDragOperationsMask, const WebKit::WebImage&, const WebKit::WebPoint&))
Also needs http://codereview.chromium.org/1539018 (which can be landed independently) to function.
Created attachment 52504 [details] Patch without debugging fprintf.
The DCHECK was caused by an unrelated change in my tree.
Attachment 52504 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/platform/chromium/DragImageChromiumMac.cpp:60: Use 0 instead of NULL. [readability/null] [5] WebCore/platform/chromium/DragImageChromiumMac.cpp:77: Use 0 instead of NULL. [readability/null] [5] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 52505 [details] Patch, address stylebot comments
Bug 35811 is currently believed to cause chromium test regressions. Does this cause the regressions to go away?
Created attachment 52662 [details] Rebase, now that the dependency has landed. This is ready for review.
Not authorized to actually comment on the patch directly; here are some thoughts: > Index: WebCore/platform/chromium/DragImageChromiumMac.cpp > =================================================================== > +#include <CoreGraphics/CGBitmapContext.h> > +#include <CoreGraphics/CGImage.h> Including headers of a sub-framework is discouraged. Try <ApplicationServices/ApplicationServices.h>. > DragImageRef scaleDragImage(DragImageRef image, FloatSize scale) > { ... > + size_t width = roundf(CGImageGetWidth(image) * scale.width()); > + size_t height = roundf(CGImageGetHeight(image) * scale.height()); > + CGContextRef context = CGBitmapContextCreate(0, width, height, 8, width * 4, CGImageGetColorSpace(image), kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Host); We usually only need to specify flags or byte ordering if we're hammering at the bitmap that we're creating. Since we're going straight to a CGImage, we probably don't need to specify anything special here. (And ditto for several similar uses below.) > + if (!context) > + return 0; > + CGContextDrawImage(context, CGRectMake(0, 0, width, height), image); > + CGImageRelease(image); I'm not familiar with the ownership context. Is the caller relinquishing ownership in a call named scaleDragImage? (And ditto for several similar uses below.)
> We usually only need to specify flags or byte ordering if we're hammering at > the bitmap that we're creating. Since we're going straight to a CGImage, we > probably don't need to specify anything special here. (And ditto for several > similar uses below.) Yes, but it's customary to specify this anyway. It's consistent if nothing else: $ ack 'CGImageCreate\(' third_party/WebKit/WebCore/ > > + if (!context) > > + return 0; > > + CGContextDrawImage(context, CGRectMake(0, 0, width, height), image); > > + CGImageRelease(image); > > I'm not familiar with the ownership context. Is the caller relinquishing > ownership in a call named scaleDragImage? (And ditto for several similar uses > below.) Yes, as far as I understand the code. This is called roughly like the following in WebCore/page/DragController.cpp: void DragController::doImageDrag() { ... dragImage = createDragImageFromImage(image)); ... dragImage = fitDragImageToMaxSize(dragImage, rect.size(), maxDragImageSize()); // calls the scale method ... deleteDragImage(dragImage); } This means the scale method needs to delete its argument. The windows version of the class does that too.
(In reply to comment #10) > Yes, but it's customary to specify this anyway. OK. > > Is the caller relinquishing > > ownership in a call named scaleDragImage? > Yes, as far as I understand the code. Great! LGTM
Comment on attachment 52662 [details] Rebase, now that the dependency has landed. in avi we trust.
Comment on attachment 52662 [details] Rebase, now that the dependency has landed. Clearing flags on attachment: 52662 Committed r57180: <http://trac.webkit.org/changeset/57180>
All reviewed patches have been landed. Closing bug.