Bug 37069 – [chromium] DragImage implementation for mac

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Nico Weber]

Mac side of https://bugs.webkit.org/show_bug.cgi?id=35811 .

[Nico Weber]

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&))

[Nico Weber]

Also needs http://codereview.chromium.org/1539018 (which can be landed independently) to function.

[Nico Weber]

Created attachment 52504 [details]
Patch without debugging fprintf.

[Nico Weber]

The DCHECK was caused by an unrelated change in my tree.

[WebKit Review Bot]

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.

[Nico Weber]

Created attachment 52505 [details]
Patch, address stylebot comments

[Eric Seidel (no email)]

Bug 35811 is currently believed to cause chromium test regressions.  Does this cause the regressions to go away?

[Nico Weber]

Created attachment 52662 [details]
Rebase, now that the dependency has landed.

This is ready for review.

[Avi Drissman]

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.)

[Nico Weber]

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

[Avi Drissman]

(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

[Dimitri Glazkov (Google)]

Comment on attachment 52662 [details]
Rebase, now that the dependency has landed.

in avi we trust.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.