Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
286903011:
Show DragImage when the dragged element or its parent has transform (Closed)
|
Created:
6 years, 7 months ago by spartha Modified:
6 years, 6 months ago CC:
blink-reviews, blink-reviews-events_chromium.org, blink-reviews-rendering, dglazkov+blink, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr., rune+blink, spartha80_gmail.com, tkent, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionShow DragImage when the dragged element or its parent has transform
The DragEvent was ignoring the trasnform for the drag point, causing
the dragimage to be drawn at an incorrect location on the canvas. The
patch corrects this.
BUG=322201
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175567
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : Rebase #
Total comments: 2
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Total comments: 4
Patch Set 7 : Updated #Patch Set 8 : Rebase #
Messages
Total messages: 32 (0 generated)
I am not sure if we can add testcases other than Manual Tests. Older patches relied on Manual Tests. Will they be good enough for this patch?
The change looks reasonable to me, but I'm not a rendering expert. As for testing... it would be nice to provide some way for DRT to provide some way to catch the drag image and dump it for testing purposes. I think this could be fairly easily done by coordinating between TestRunner and WebTestProxy::startDragging.
levi@ or ojan@ are your best reviewers. This seems fine to me, but Ojan has been in this code more recently.
On 2014/05/27 20:50:36, dcheng wrote: > The change looks reasonable to me, but I'm not a rendering expert. > > As for testing... it would be nice to provide some way for DRT to provide some > way to catch the drag image and dump it for testing purposes. I think this could > be fairly easily done by coordinating between TestRunner and > WebTestProxy::startDragging. I found a way to write unit tests for this, https://codereview.chromium.org/307553007/ Would that suffice? If that is fine I can add the unit tests for this.
Thanks for adding a test. Overall, this looks reasonable to me but please wait for a reply from leviw@ or ojan@ before landing. https://codereview.chromium.org/286903011/diff/20001/Source/web/tests/WebFram... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/286903011/diff/20001/Source/web/tests/WebFram... Source/web/tests/WebFrameTest.cpp:5468: EXPECT_EQ(0, memcmp(bitmap.getPixels(), dragBitmap.getPixels(), bitmap.rowBytes() * 40)); You can replace bitmap.rowBytes() * 40 with bitmap.getSize()
https://codereview.chromium.org/286903011/diff/20001/Source/web/tests/WebFram... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/286903011/diff/20001/Source/web/tests/WebFram... Source/web/tests/WebFrameTest.cpp:5468: EXPECT_EQ(0, memcmp(bitmap.getPixels(), dragBitmap.getPixels(), bitmap.rowBytes() * 40)); On 2014/05/28 21:02:59, dcheng wrote: > You can replace bitmap.rowBytes() * 40 with bitmap.getSize() Done.
https://codereview.chromium.org/286903011/diff/60001/Source/web/tests/data/no... File Source/web/tests/data/nodeimage.html (right): https://codereview.chromium.org/286903011/diff/60001/Source/web/tests/data/no... Source/web/tests/data/nodeimage.html:5: <div style="-webkit-transform:translate(40px,40px)"> You don't need the -webkit prefix. Please add a 3d-transform case as well.
https://codereview.chromium.org/286903011/diff/60001/Source/web/tests/data/no... File Source/web/tests/data/nodeimage.html (right): https://codereview.chromium.org/286903011/diff/60001/Source/web/tests/data/no... Source/web/tests/data/nodeimage.html:5: <div style="-webkit-transform:translate(40px,40px)"> On 2014/05/30 18:37:31, leviw wrote: > You don't need the -webkit prefix. > > Please add a 3d-transform case as well. Done.
https://codereview.chromium.org/286903011/diff/100001/Source/web/tests/data/n... File Source/web/tests/data/nodeimage.html (right): https://codereview.chromium.org/286903011/diff/100001/Source/web/tests/data/n... Source/web/tests/data/nodeimage.html:10: <div style="transform:translate(40px,40px) rotate3d(1,0,0,180deg);"> This doesn't actually exercise the 3d part since you're taking a 40x40 box and rotating it 180 degrees.
https://codereview.chromium.org/286903011/diff/100001/Source/web/tests/data/n... File Source/web/tests/data/nodeimage.html (right): https://codereview.chromium.org/286903011/diff/100001/Source/web/tests/data/n... Source/web/tests/data/nodeimage.html:10: <div style="transform:translate(40px,40px) rotate3d(1,0,0,180deg);"> On 2014/05/30 21:38:08, leviw wrote: > This doesn't actually exercise the 3d part since you're taking a 40x40 box and > rotating it 180 degrees. Done.
Thanks! LGTM.
The CQ bit was checked by sudarshan.p@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/286903011/120001
The CQ bit was unchecked by sudarshan.p@samsung.com
On 2014/06/03 04:38:38, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/286903011/120001 @esprehn, Need an owner lgtm.
lgtm, couple nits that would be nice to fix before landing. https://codereview.chromium.org/286903011/diff/120001/Source/web/tests/WebFra... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/286903011/diff/120001/Source/web/tests/WebFra... Source/web/tests/WebFrameTest.cpp:5472: { I think it's preferable to split the test into two separate TEST_F and have a helper function that does the setup above. https://codereview.chromium.org/286903011/diff/120001/Source/web/tests/data/n... File Source/web/tests/data/nodeimage.html (right): https://codereview.chromium.org/286903011/diff/120001/Source/web/tests/data/n... Source/web/tests/data/nodeimage.html:3: <body> We usually leave off the <html>, <head> and <body> tags.
https://codereview.chromium.org/286903011/diff/120001/Source/web/tests/WebFra... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/286903011/diff/120001/Source/web/tests/WebFra... Source/web/tests/WebFrameTest.cpp:5472: { On 2014/06/04 18:13:45, esprehn wrote: > I think it's preferable to split the test into two separate TEST_F and have a > helper function that does the setup above. Done. https://codereview.chromium.org/286903011/diff/120001/Source/web/tests/data/n... File Source/web/tests/data/nodeimage.html (right): https://codereview.chromium.org/286903011/diff/120001/Source/web/tests/data/n... Source/web/tests/data/nodeimage.html:3: <body> On 2014/06/04 18:13:45, esprehn wrote: > We usually leave off the <html>, <head> and <body> tags. Done.
The CQ bit was checked by sudarshan.p@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/286903011/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10390) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/10750)
The CQ bit was unchecked by sudarshan.p@samsung.com
The CQ bit was checked by sudarshan.p@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/286903011/180001
The CQ bit was checked by sudarshan.p@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/286903011/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7031) linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7034)
The CQ bit was checked by sudarshan.p@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/286903011/180001
Message was sent while issue was closed.
Change committed as 175567 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
