Bug 110512 – Change default background color of a dragged image

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Shawn Singh]

Most of the time, dragged images did not care what the background color of their nodeImage was.  However, now in some cases, this background color actually matters because the drag image becomes a webkit composited layer and paints with that color.  So, we should change this color to completely transparent.

dcheng@ can you please make sure what I said above is correct =)   Also, I did some archaeology and I think the original change was made in https://bugs.webkit.org/show_bug.cgi?id=43449 -- hopefully that may jog your memory if there really is an important reason why opaque white was chosen. There's no particular platform or other port that especially needs opaque white to paint the drag images correctly, is there?

Patch coming in a moment.

[Daniel Cheng]

Yes, that's the right patch. I already looked at it yesterday, but I still don't really remember the exact reason for picking a white background.

[Shawn Singh]

Created attachment 189601 [details]
Patch

[Daniel Cheng]

Comment on attachment 189601 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189601&action=review

> Source/WebCore/page/Frame.cpp:1082
> +    m_view->setBaseBackgroundColor(colorWithOverrideAlpha(Color::black, 0.0));

Perhaps just use Color::transparent?

[Shawn Singh]

Created attachment 189602 [details]
Use Color::transparent

[Build Bot]

Comment on attachment 189602 [details]
Use Color::transparent

Attachment 189602 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16692349

New failing tests:
svg/custom/empty-className-baseVal-crash.html

[Shawn Singh]

Still requesting review...

before patch + after patch layout tests seem the same on my mac desktop.  The ews bot failures are from the tree, very unlikely from this patch.

[Simon Fraser (smfr)]

Can you give specific examples of content that shows the problem you're trying to fix?

[Shawn Singh]

(In reply to comment #7)
> Can you give specific examples of content that shows the problem you're trying to fix?

The original chromium-side bug report is https://code.google.com/p/chromium/issues/detail?id=177036  - you can see the screenshot on the first post there.  For other reasons, the drag image is chosen to be larger than the image itself.  As I understand the situation, there's no way to reproduce this in a reduced test / layout test, since it's a uniqueness of chrome os that recycles the draggable image back into another webview for the ui.  At least, I think that's how it works.

[Shawn Singh]

Created attachment 190108 [details]
retrying ews bots

[Shawn Singh]

Is there any update to review?  tony@ I think you may have reviewed some dragging related patches in the past; could you please take a look?   Thanks in advance.

[Tony Chang]

Comment on attachment 190108 [details]
retrying ews bots

View in context: https://bugs.webkit.org/attachment.cgi?id=190108&action=review

Looks like this code originally came from Chromium's WebKit port and was refactored in r96339 to be used by Gtk+ as well.

Can you describe how to manually see the difference here?  Maybe you can upload a test case to the bug or add manual test to the ManualTests directory?

> Source/WebCore/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

This goes below the bug line.  See the other entries in the ChangeLog.

> Source/WebCore/ChangeLog:13
> +        This behavior is needed for chrome OS, the background color for
> +        nodeImage doesn't show up at all when dragging for other ports; so

Please mention that this change does not impact Mac or Win due to the #if.

[Shawn Singh]

Comment on attachment 190108 [details]
retrying ews bots

Thanks for review, removing r=? for now, I'll try to have a new patch soon.

[Shawn Singh]

(In reply to comment #11)
> (From update of attachment 190108 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190108&action=review
> 
> Looks like this code originally came from Chromium's WebKit port and was refactored in r96339 to be used by Gtk+ as well.
> 
> Can you describe how to manually see the difference here?

hshi@ uploaded a nice manual test on the chromium side - https://code.google.com/p/chromium/issues/detail?id=177036 - I'll create a webkit manual test based off that for the next patch.

Tony@ in the mean-time, did you have any further thoughts about  comments #39 and #40, point (2) ?

thanks

[Shawn Singh]

Created attachment 191569 [details]
Patch

[Shawn Singh]

> > Source/WebCore/ChangeLog:13
> > +        This behavior is needed for chrome OS, the background color for
> > +        nodeImage doesn't show up at all when dragging for other ports; so
> 
> Please mention that this change does not impact Mac or Win due to the #if.

Actually I did not understand what you mean here...  does this comment still apply to the latest patch?

Latest patch is complete this time, including a Manual test.

[Tony Chang]

(In reply to comment #15)
> > > Source/WebCore/ChangeLog:13
> > > +        This behavior is needed for chrome OS, the background color for
> > > +        nodeImage doesn't show up at all when dragging for other ports; so
> > 
> > Please mention that this change does not impact Mac or Win due to the #if.
> 
> Actually I did not understand what you mean here...  does this comment still apply to the latest patch?

Sorry, by Mac and Win I mean Apple Mac and Apple Win due to "#if !PLATFORM(MAC) && !PLATFORM(WIN)" a few lines up.

[Tony Chang]

Comment on attachment 191569 [details]
Patch

LGTM, please update the ChangeLog before landing.

[Shawn Singh]

Created attachment 191587 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 191587 [details]
Patch for landing

Rejecting attachment 191587 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-03', 'validate-changelog', '--non-interactive', 191587, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

/mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-commit-queue.appspot.com/results/17048199

[Shawn Singh]

Created attachment 191590 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 191590 [details]
Patch for landing

Clearing flags on attachment: 191590

Committed r144841: <http://trac.webkit.org/changeset/144841>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.