Bug 37069 - [chromium] DragImage implementation for mac
Summary: [chromium] DragImage implementation for mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 35811
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-03 21:35 PDT by Nico Weber
Modified: 2010-04-06 18:02 PDT (History)
4 users (show)

See Also:


Attachments
Patch. (6.82 KB, patch)
2010-04-03 21:42 PDT, Nico Weber
no flags Details | Formatted Diff | Diff
Patch without debugging fprintf. (6.70 KB, patch)
2010-04-03 21:48 PDT, Nico Weber
no flags Details | Formatted Diff | Diff
Patch, address stylebot comments (6.69 KB, patch)
2010-04-03 22:50 PDT, Nico Weber
no flags Details | Formatted Diff | Diff
Rebase, now that the dependency has landed. (6.74 KB, patch)
2010-04-06 13:33 PDT, Nico Weber
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nico Weber 2010-04-03 21:35:13 PDT
Mac side of https://bugs.webkit.org/show_bug.cgi?id=35811 .
Comment 1 Nico Weber 2010-04-03 21:42:13 PDT
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&))
Comment 2 Nico Weber 2010-04-03 21:44:33 PDT
Also needs http://codereview.chromium.org/1539018 (which can be landed independently) to function.
Comment 3 Nico Weber 2010-04-03 21:48:29 PDT
Created attachment 52504 [details]
Patch without debugging fprintf.
Comment 4 Nico Weber 2010-04-03 22:45:43 PDT
The DCHECK was caused by an unrelated change in my tree.
Comment 5 WebKit Review Bot 2010-04-03 22:47:42 PDT
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.
Comment 6 Nico Weber 2010-04-03 22:50:41 PDT
Created attachment 52505 [details]
Patch, address stylebot comments
Comment 7 Eric Seidel (no email) 2010-04-04 15:24:57 PDT
Bug 35811 is currently believed to cause chromium test regressions.  Does this cause the regressions to go away?
Comment 8 Nico Weber 2010-04-06 13:33:45 PDT
Created attachment 52662 [details]
Rebase, now that the dependency has landed.

This is ready for review.
Comment 9 Avi Drissman 2010-04-06 14:25:53 PDT
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.)
Comment 10 Nico Weber 2010-04-06 14:33:10 PDT
> 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.
Comment 11 Avi Drissman 2010-04-06 14:44:55 PDT
(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 12 Dimitri Glazkov (Google) 2010-04-06 15:00:37 PDT
Comment on attachment 52662 [details]
Rebase, now that the dependency has landed.

in avi we trust.
Comment 13 WebKit Commit Bot 2010-04-06 18:02:18 PDT
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>
Comment 14 WebKit Commit Bot 2010-04-06 18:02:23 PDT
All reviewed patches have been landed.  Closing bug.