Bug 43449 - [chromium] Generate drag images for HTML elements and selections.
Summary: [chromium] Generate drag images for HTML elements and selections.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-03 17:04 PDT by Daniel Cheng
Modified: 2010-08-24 09:07 PDT (History)
9 users (show)

See Also:


Attachments
Patch (5.08 KB, patch)
2010-08-03 17:10 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (5.56 KB, patch)
2010-08-04 14:46 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (5.90 KB, patch)
2010-08-05 14:55 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (6.55 KB, patch)
2010-08-10 14:45 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (6.64 KB, patch)
2010-08-10 15:52 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (13.04 KB, patch)
2010-08-13 15:18 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (13.06 KB, patch)
2010-08-17 15:29 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (13.08 KB, patch)
2010-08-17 15:45 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (13.34 KB, patch)
2010-08-17 17:38 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (13.50 KB, patch)
2010-08-18 13:56 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (13.35 KB, patch)
2010-08-18 14:31 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (13.00 KB, patch)
2010-08-19 14:35 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (16.59 KB, patch)
2010-08-20 16:03 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Cheng 2010-08-03 17:04:20 PDT
http://code.google.com/p/chromium/issues/detail?id=16142 for test case.
Comment 1 Daniel Cheng 2010-08-03 17:10:58 PDT
Created attachment 63393 [details]
Patch
Comment 2 Daniel Cheng 2010-08-03 17:16:00 PDT
There's a few issues with this patch.

1) On GTK (and possibly other platforms, since I haven't tested yet), selection text gets rendered with a white text color, so the drag image looks terrible. I fixed this by passing PaintBehaviorForceBlackText, but this is suboptimal as well, since this forces all text to be black.

2) When setting an element as the drag image, the background is also drawn. If you try the test case attached to the Chromium bug, we end up dragging around a long white block around. We don't want (always) want the background. I'm not sure how I can force the renderer to skip painting the background though... the Mac port seems to get around this by scraping pixels directly from the NSView.
Comment 3 Daniel Cheng 2010-08-03 18:30:45 PDT
It also renders different on Windows and GTK+ though I don't understand why...
The code in the patch isn't really platform-specific...
Comment 4 Daniel Cheng 2010-08-04 14:46:12 PDT
Created attachment 63493 [details]
Patch
Comment 5 Eric Seidel (no email) 2010-08-04 18:56:09 PDT
Comment on attachment 63493 [details]
Patch

WebCore/page/Frame.h:225
 +          DragImageRef dragImageForElement(Node* node);
Generally we don't give arguments names when the names don't add clarity.

WebCore/page/chromium/FrameChromium.cpp:47
 +      renderer->updateDragState(true);
I probably would have created saveState/restoreState functions for these.

WebCore/page/chromium/FrameChromium.cpp:67
 +      // Restore original state.
Functional devisions ad more clarity than comment blocks.  Even if the functions are only called once.

I would actually probably have used a RAII instead of two functions even.WebCore/page/chromium/FrameChromium.cpp:82
 +          return result;
Eh, just return 0 here.

WebCore/page/chromium/FrameChromium.cpp:102
 +      result = createDragImageFromImage(buffer->image());
And just return directly here.  no need for result.

WebCore/page/chromium/FrameChromium.cpp:84
 +      IntRect paintingRect = enclosingIntRect(selectionBounds());
Are we intentionally rounding here...

WebCore/page/chromium/FrameChromium.cpp:89
 +      buffer->context()->clip(FloatRect(0, 0, paintingRect.right(), paintingRect.bottom()));
Only then to convert back to floats here?

I wish we had some way to test this stuff.  DRT ideally should have a way to dump the drag image.  Or expose it to javascript so it could be drawn to a canvas.
Comment 6 Eric Seidel (no email) 2010-08-04 18:56:53 PDT
Comment on attachment 63493 [details]
Patch

I'd like to see one more round.  Perhaps one of the other CC'd folks would have ideas about testing.
Comment 7 Daniel Cheng 2010-08-05 14:55:42 PDT
Created attachment 63640 [details]
Patch
Comment 8 Daniel Cheng 2010-08-05 14:58:59 PDT
(In reply to comment #5)
> (From update of attachment 63493 [details])
> WebCore/page/Frame.h:225
>  +          DragImageRef dragImageForElement(Node* node);
> Generally we don't give arguments names when the names don't add clarity.
> 

Done.

> WebCore/page/chromium/FrameChromium.cpp:47
>  +      renderer->updateDragState(true);
> I probably would have created saveState/restoreState functions for these.
> 
> WebCore/page/chromium/FrameChromium.cpp:67
>  +      // Restore original state.
> Functional devisions ad more clarity than comment blocks.  Even if the functions are only called once.
> 

Done.

> I would actually probably have used a RAII instead of two functions even.
>
> WebCore/page/chromium/FrameChromium.cpp:82
>  +          return result;
> Eh, just return 0 here.
> 

Done.

> WebCore/page/chromium/FrameChromium.cpp:102
>  +      result = createDragImageFromImage(buffer->image());
> And just return directly here.  no need for result.
> 

Done.

> WebCore/page/chromium/FrameChromium.cpp:84
>  +      IntRect paintingRect = enclosingIntRect(selectionBounds());
> Are we intentionally rounding here...
> 
> WebCore/page/chromium/FrameChromium.cpp:89
>  +      buffer->context()->clip(FloatRect(0, 0, paintingRect.right(), paintingRect.bottom()));
> Only then to convert back to floats here?
> 

Yes. FrameView::paint() and GraphicsContext::translate() take integer arguments.

> I wish we had some way to test this stuff.  DRT ideally should have a way to dump the drag image.  Or expose it to javascript so it could be drawn to a canvas.

Several questions:
1) Is there a better way to do dragImageForSelection()? PaintBehaviorForceBlackText seems like an ugly hack, and it's unnecessary on the Mac. Is there some way to tell the renderer to paint the selection, but without the selection highlight?

2) dragImageForElement and dragImageForSelection are (to the best of my understanding) platform independent. Would it make sense to move this into Frame so all the other ports can use it? It's worth pointing out that some ports already have their own implementations of these functions (with different names) as well...
Comment 9 Daniel Cheng 2010-08-10 14:45:28 PDT
Created attachment 64045 [details]
Patch
Comment 10 Daniel Cheng 2010-08-10 14:46:38 PDT
I made a minor update to the patch so I don't need to force foreground text color to be black anymore.
Comment 11 Daniel Cheng 2010-08-10 15:52:13 PDT
Created attachment 64053 [details]
Patch

Forgot to update the ChangeLog
Comment 12 Eric Seidel (no email) 2010-08-11 16:51:32 PDT
Comment on attachment 64053 [details]
Patch

How do other platforms do this?  Why is dragImageForElement chromium-only?
Comment 13 Daniel Cheng 2010-08-11 17:03:52 PDT
(In reply to comment #12)
> (From update of attachment 64053 [details])
> How do other platforms do this?  Why is dragImageForElement chromium-only?

This is unimplemented on almost all platforms except Mac/Win Safari. They have their own implementation (see Frame::snapshotDragImage and Frame::selectionImage).

The only 2 reasons the two methods are Chromium-only is because I'm not sure how to handle the platforms that already have their own implementation. I'm also not certain of the correctness--on Windows and GTK, I have no problems, but on OS X, I'm seeing occasional crashes in gfx::CGImageToSkBitmap when dragging selections (I haven't reproed it when DOM nodes are set as the drag image yet) and I don't understand why.
Comment 14 Tony Chang 2010-08-12 13:35:20 PDT
It looks like Safari uses Frame::nodeImage(Node*).  Can we do the same for chromium?  Bonus points if you make the return type a DragImageRef and pull the function out from behind the platform #ifs.
Comment 15 Tony Chang 2010-08-12 13:39:40 PDT
(In reply to comment #14)
> It looks like Safari uses Frame::nodeImage(Node*).  Can we do the same for chromium?  Bonus points if you make the return type a DragImageRef and pull the function out from behind the platform #ifs.

That is, safari win uses Frame::nodeImage(Node*) and safari mac uses Frame::snapshotDragImage(Node*, NSRect* imageRect, NSRect* elementRect).  However both have a nodeImage(Node*) method and that's the signature we want for chromium, so it seems worthwhile to pull that out and use it in Chromium.
Comment 16 Daniel Cheng 2010-08-12 14:30:08 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > It looks like Safari uses Frame::nodeImage(Node*).  Can we do the same for chromium?  Bonus points if you make the return type a DragImageRef and pull the function out from behind the platform #ifs.
> 
> That is, safari win uses Frame::nodeImage(Node*) and safari mac uses Frame::snapshotDragImage(Node*, NSRect* imageRect, NSRect* elementRect).  However both have a nodeImage(Node*) method and that's the signature we want for chromium, so it seems worthwhile to pull that out and use it in Chromium.

I'll rename the existing nodeImage methods, but before I do that, I'm also wondering--is it worth replacing the current Safari implementations with the new dragImage* methods? Since they're platform neutral, it'd seem to make sense but maybe I'm missing something...
Comment 17 Daniel Cheng 2010-08-13 15:18:35 PDT
Created attachment 64379 [details]
Patch
Comment 18 WebKit Review Bot 2010-08-14 00:55:54 PDT
Attachment 64379 [details] did not build on win:
Build output: http://queues.webkit.org/results/3774154
Comment 19 Tony Chang 2010-08-16 11:21:43 PDT
Comment on attachment 64379 [details]
Patch

The changes to Frame* look right to me.  I don't know enough about the RenderObject.cpp to say whether that change is correct or not.  Can a reviewer more familiar with that code comment?

> diff --git a/WebCore/page/chromium/FrameChromium.cpp b/WebCore/page/chromium/FrameChromium.cpp
> +    // When generating the drag image for an element, ignore the document background.
> +    m_view->setBaseBackgroundColor(Color(255, 255, 255, 0));
Can you use Color::white here?  Maybe you can use colorWithOverrideAlpha?
Comment 20 Daniel Cheng 2010-08-17 15:29:28 PDT
Created attachment 64646 [details]
Patch
Comment 21 Daniel Cheng 2010-08-17 15:45:46 PDT
Created attachment 64649 [details]
Patch

Use colorWithOverrideAlpha instead.
Comment 22 Daniel Cheng 2010-08-17 17:38:57 PDT
Created attachment 64657 [details]
Patch

Speculative compile fix for Qt/Wince.
Comment 23 Eric Seidel (no email) 2010-08-18 09:52:25 PDT
Comment on attachment 64657 [details]
Patch

Looks good.  I still think a RIIA is a more fail-safe way of doing save/restore. :)

I don't understand the RenderObject change, and it's not documented in the ChangeLog

     if (style()->userSelect() == SELECT_NONE
 1595         || (frame()->view()->paintBehavior() & PaintBehaviorSelectionOnly))
Comment 24 Daniel Cheng 2010-08-18 13:56:10 PDT
Created attachment 64768 [details]
Patch

Added a comment for the RenderObject change.\n\nEric, I'd be happy to use RAII but I'm not sure how...? Maybe there's some obvious RenderState RAII object in WebKit I'm overlooking.
Comment 25 Eric Seidel (no email) 2010-08-18 14:02:44 PDT
Comment on attachment 64768 [details]
Patch

I don't feel like I'm a very good reviewer for this, but this generally looks sane to me.  Folks who should have more knowledge of this are CC'd, so if I'm doing something wrong here, they should feel encouraged to yell.
Comment 26 Eric Seidel (no email) 2010-08-18 14:03:55 PDT
There is no "view state" RAII, you'd have to create one.  I'm not really sure why the rendering state/view state needs to be saved for these.
Comment 27 James Robinson 2010-08-18 14:07:17 PDT
I'd really like to see some tests and checks for very large node bounds.  Just doing something simple (like clamping the drag image to 512x512 or something small) would be sufficient, so long as it's tested.
Comment 28 Daniel Cheng 2010-08-18 14:08:23 PDT
Comment on attachment 64768 [details]
Patch

Per jamesr comments, I need to add a check for ImageBuffer::create failure.

For what it's worth, I'm saving the various bits that I twiddle in the view, because that's what other ports have done in their nodeImage implementations. I tried leaving it out once, which resulted in rather interesting painting results, so I think it's required.
Comment 29 Daniel Cheng 2010-08-18 14:31:45 PDT
Created attachment 64773 [details]
Patch

Add RAII and ImageBuffer creation check.
Comment 30 Eric Seidel (no email) 2010-08-18 14:36:59 PDT
Comment on attachment 64773 [details]
Patch

WebCore/page/chromium/FrameChromium.cpp:88
 +      m_doc->updateLayout();
Why do we need to call updateLayout a second time?

WebCore/page/chromium/FrameChromium.cpp:86
 +      // Restore original state.
Why wouldn't this save/restore just be part of your RAII?
(The answer is because you use your RAII in two places, I guess.)

WebCore/page/chromium/FrameChromium.cpp:66
 +      const ScopedViewState state(m_view);
This will end up restoring after the return statement (which is fine).  Just pointing out that its at a slightly different time than your previous patch.

WebCore/page/chromium/FrameChromium.cpp:120
 +      m_doc->updateLayout();
manual layout updates are rarely needed.  Why is this needed here?
Comment 31 Daniel Cheng 2010-08-18 14:45:24 PDT
(In reply to comment #30)
> (From update of attachment 64773 [details])
> WebCore/page/chromium/FrameChromium.cpp:88
>  +      m_doc->updateLayout();
> Why do we need to call updateLayout a second time?

I don't honestly know, but this is what FrameMac.mm does in selectionImage and snapshotDragImage.

> 
> WebCore/page/chromium/FrameChromium.cpp:86
>  +      // Restore original state.
> Why wouldn't this save/restore just be part of your RAII?
> (The answer is because you use your RAII in two places, I guess.)
> 

It could. I guess it's easier to just make it capture the superset of state that both functions care about.

> WebCore/page/chromium/FrameChromium.cpp:66
>  +      const ScopedViewState state(m_view);
> This will end up restoring after the return statement (which is fine).  Just pointing out that its at a slightly different time than your previous patch.

Oops. That is probably a mistake...

> 
> WebCore/page/chromium/FrameChromium.cpp:120
>  +      m_doc->updateLayout();
> manual layout updates are rarely needed.  Why is this needed here?

See above.
Comment 32 Eric Seidel (no email) 2010-08-18 14:55:32 PDT
We have ASSERTs to make sure layout is up to date.  So I would remove the latter two updateLayout() calls, as I don't think they're necessary (ASSERTs will tell us if they are).
Comment 33 Eric Seidel (no email) 2010-08-18 14:56:06 PDT
Again, thank you for your persistance with this bug.  I think you're making a significant improvement in the code quality here by sharing this method across platforms!
Comment 34 Daniel Cheng 2010-08-18 14:58:38 PDT
(In reply to comment #32)
> We have ASSERTs to make sure layout is up to date.  So I would remove the latter two updateLayout() calls, as I don't think they're necessary (ASSERTs will tell us if they are).

Does this mean the calls to updateLayout() on line 72 and 108 are possibly redundant as well?

(In reply to comment #33)
> Again, thank you for your persistance with this bug.  I think you're making a significant improvement in the code quality here by sharing this method across platforms!

It's not shared yet, but I'd like to share it in a later patch if there are no objections as this should simplify things for all the other ports as well.
Comment 35 Eric Seidel (no email) 2010-08-18 15:10:48 PDT
Well, so layout is required before painting or before grabbing rects from the rendering tree.  Because layout is how the RenderObjects figure out where they are.  So it makes sense that you'd need to call updateLayoutIfNeeded (I think that's the name?) or updateLayout right before grabbing a rect off the rendering tree (or you might ASSERT), and certainly before painting.  But you wouldn't after you're done since nothing you're doing in this patch should invalidate layout as far as I can tell?
Comment 36 Daniel Cheng 2010-08-19 14:35:48 PDT
Created attachment 64900 [details]
Patch

Fix RAII and remove unnecessary updateLayout calls.
Comment 37 Eric Seidel (no email) 2010-08-19 15:37:30 PDT
Comment on attachment 64900 [details]
Patch

Still LGTM.
Comment 38 WebKit Commit Bot 2010-08-19 17:20:48 PDT
Comment on attachment 64900 [details]
Patch

Rejecting patch 64900 from commit-queue.

Unexpected failure when processing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 64900, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
date_status
    return NetworkTransaction().run(lambda: self._post_status_to_server(queue_name, status, patch, results_file))
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", line 56, in run
    self._check_for_timeout()
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", line 63, in _check_for_timeout
    raise NetworkTimeout()
webkitpy.common.net.networktransaction.NetworkTimeout
Comment 39 Adam Barth 2010-08-19 17:23:23 PDT
Comment on attachment 64900 [details]
Patch

Status server down?  That's unpossible.
Comment 40 Eric Seidel (no email) 2010-08-19 18:01:53 PDT
Equally likely is that this patch breaks mac, and that it is failing to upload the failure log because it's too large.
Comment 41 WebKit Commit Bot 2010-08-19 18:55:57 PDT
Comment on attachment 64900 [details]
Patch

Rejecting patch 64900 from commit-queue.

Unexpected failure when processing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 64900, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
date_status
    return NetworkTransaction().run(lambda: self._post_status_to_server(queue_name, status, patch, results_file))
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", line 56, in run
    self._check_for_timeout()
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", line 63, in _check_for_timeout
    raise NetworkTimeout()
webkitpy.common.net.networktransaction.NetworkTimeout
Comment 42 Eric Seidel (no email) 2010-08-19 20:06:38 PDT
Yup.  This is an unfortunate bug in the commit-queue.  But I expect this is failing to build on mac, which is why it's timing out.
Comment 43 Daniel Cheng 2010-08-20 16:03:06 PDT
Created attachment 65004 [details]
Patch

Fix the Mac build.
Comment 44 Daniel Cheng 2010-08-20 16:05:27 PDT
(In reply to comment #43)
> Created an attachment (id=65004) [details]
> Patch
> 
> Fix the Mac build.

Also, it is weird/bad form to save an autoreleased pointer into RetainPtr<T>? It seems a little wasteful at the very least, because of refcount churn, but I'm not sure what's a better approach.
Comment 45 Eric Seidel (no email) 2010-08-23 16:42:32 PDT
Comment on attachment 65004 [details]
Patch

I think the bug here is that DragImageRef shouldn't be a RetainPtr on some platforms and a raw ptr on others.  You might use the getPtr() template, which is how we get pointers out of raw or smart pointers when the code doesn't necessarily know the type.

I think this is OK.  But we should file a bug about fixing DragImageRef on Mac.
Comment 46 WebKit Commit Bot 2010-08-23 18:49:48 PDT
Comment on attachment 65004 [details]
Patch

Rejecting patch 65004 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20898 test cases.
animations/dynamic-stylesheet-loading.html -> failed

Exiting early after 1 failures. 100 tests run.
42.20s total testing time

99 test cases (99%) succeeded
1 test case (1%) had incorrect layout

Full output: http://queues.webkit.org/results/3786488
Comment 47 Eric Seidel (no email) 2010-08-24 07:33:06 PDT
Comment on attachment 65004 [details]
Patch

That might be a flaky test.
Comment 48 WebKit Commit Bot 2010-08-24 09:07:10 PDT
Comment on attachment 65004 [details]
Patch

Clearing flags on attachment: 65004

Committed r65904: <http://trac.webkit.org/changeset/65904>
Comment 49 WebKit Commit Bot 2010-08-24 09:07:18 PDT
All reviewed patches have been landed.  Closing bug.