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.
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.
Created attachment 189601 [details] Patch
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?
Created attachment 189602 [details] Use Color::transparent
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
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.
Can you give specific examples of content that shows the problem you're trying to fix?
(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.
Created attachment 190108 [details] retrying ews bots
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.
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.
Comment on attachment 190108 [details] retrying ews bots Thanks for review, removing r=? for now, I'll try to have a new patch soon.
(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
Created attachment 191569 [details] Patch
> > 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.
(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.
Comment on attachment 191569 [details] Patch LGTM, please update the ChangeLog before landing.
Created attachment 191587 [details] Patch for landing
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
Created attachment 191590 [details] Patch for landing
Comment on attachment 191590 [details] Patch for landing Clearing flags on attachment: 191590 Committed r144841: <http://trac.webkit.org/changeset/144841>
All reviewed patches have been landed. Closing bug.