Bug 43449 – [chromium] Generate drag images for HTML elements and selections.

• Component: Platform
• Status: RESOLVED FIXED

[Daniel Cheng]

http://code.google.com/p/chromium/issues/detail?id=16142 for test case.

[Daniel Cheng]

Created attachment 63393 [details]
Patch

[Daniel Cheng]

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.

[Daniel Cheng]

It also renders different on Windows and GTK+ though I don't understand why...
The code in the patch isn't really platform-specific...

[Daniel Cheng]

Created attachment 63493 [details]
Patch

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Daniel Cheng]

Created attachment 63640 [details]
Patch

[Daniel Cheng]

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

[Daniel Cheng]

Created attachment 64045 [details]
Patch

[Daniel Cheng]

I made a minor update to the patch so I don't need to force foreground text color to be black anymore.

[Daniel Cheng]

Created attachment 64053 [details]
Patch

Forgot to update the ChangeLog

[Eric Seidel (no email)]

Comment on attachment 64053 [details]
Patch

How do other platforms do this?  Why is dragImageForElement chromium-only?

[Daniel Cheng]

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

[Tony Chang]

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.

[Tony Chang]

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

[Daniel Cheng]

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

[Daniel Cheng]

Created attachment 64379 [details]
Patch

[WebKit Review Bot]

Attachment 64379 [details] did not build on win:
Build output: http://queues.webkit.org/results/3774154

[Tony Chang]

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?

[Daniel Cheng]

Created attachment 64646 [details]
Patch

[Daniel Cheng]

Created attachment 64649 [details]
Patch

Use colorWithOverrideAlpha instead.

[Daniel Cheng]

Created attachment 64657 [details]
Patch

Speculative compile fix for Qt/Wince.

[Eric Seidel (no email)]

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

[Daniel Cheng]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[James Robinson]

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.

[Daniel Cheng]

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.

[Daniel Cheng]

Created attachment 64773 [details]
Patch

Add RAII and ImageBuffer creation check.

[Eric Seidel (no email)]

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?

[Daniel Cheng]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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!

[Daniel Cheng]

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

[Eric Seidel (no email)]

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?

[Daniel Cheng]

Created attachment 64900 [details]
Patch

Fix RAII and remove unnecessary updateLayout calls.

[Eric Seidel (no email)]

Comment on attachment 64900 [details]
Patch

Still LGTM.

[WebKit Commit Bot]

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

[Adam Barth]

Comment on attachment 64900 [details]
Patch

Status server down?  That's unpossible.

[Eric Seidel (no email)]

Equally likely is that this patch breaks mac, and that it is failing to upload the failure log because it's too large.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

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.

[Daniel Cheng]

Created attachment 65004 [details]
Patch

Fix the Mac build.

[Daniel Cheng]

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

[Eric Seidel (no email)]

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.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Comment on attachment 65004 [details]
Patch

That might be a flaky test.

[WebKit Commit Bot]

Comment on attachment 65004 [details]
Patch

Clearing flags on attachment: 65004

Committed r65904: <http://trac.webkit.org/changeset/65904>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.