Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
976543002:
Fix pixel snapping issue when transforming (Closed)
|
Created:
5 years, 9 months ago by qiankun Modified:
5 years, 7 months ago CC:
blink-reviews, blink-reviews-paint_chromium.org, dshwang, slimming-paint-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionFix pixel snapping issue when transforming
Border with fractional location and size is accumulating the subpixel
offset while walking the rendering tree. But its clip rect doesn't
accumulate the offset. This results some pixels will be clipped by the
clip rect.
This change applies sub-pixel accumulation to layerBounds, foregroundBounds
and backgroundBounds if render layer doesn't paint into its own backing layer.
This CL needs to land after https://codereview.chromium.org/999033002/ which
advances document life cycle to CompositingClean before hit testing to avoid
assert failure.
BUG=332197
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193611
Patch Set 1 #
Total comments: 4
Patch Set 2 : add ref test and fix nit #Patch Set 3 : remove unnecessary styling #Patch Set 4 : #
Total comments: 1
Patch Set 5 : update to ToT #Patch Set 6 : fix build failure: renderer -> layoutObject #Patch Set 7 : fix layout tests #
Total comments: 2
Patch Set 8 : rebase and go back to last patch #
Total comments: 8
Patch Set 9 : fix comments of esprehn #
Total comments: 1
Patch Set 10 : add missing space #
Messages
Total messages: 54 (15 generated)
qiankun.miao@intel.com changed reviewers: + chrishtr@chromium.org, leviw@chromium.org
PTAL.
Seems probably correct. Levi?
Code change LGTM. Can You make this a ref test? https://codereview.chromium.org/976543002/diff/1/LayoutTests/fast/sub-pixel/f... File LayoutTests/fast/sub-pixel/fractional-border-location-and-size.html (right): https://codereview.chromium.org/976543002/diff/1/LayoutTests/fast/sub-pixel/f... LayoutTests/fast/sub-pixel/fractional-border-location-and-size.html:1: <html> Nit: doctype? https://codereview.chromium.org/976543002/diff/1/LayoutTests/fast/sub-pixel/f... LayoutTests/fast/sub-pixel/fractional-border-location-and-size.html:12: font-size: 534%; Nit: can we remove unnecessary styling? We shouldn't need font-size, and perhaps not padding either.
Thanks for review. I updated the CL, please take another look. https://codereview.chromium.org/976543002/diff/1/LayoutTests/fast/sub-pixel/f... File LayoutTests/fast/sub-pixel/fractional-border-location-and-size.html (right): https://codereview.chromium.org/976543002/diff/1/LayoutTests/fast/sub-pixel/f... LayoutTests/fast/sub-pixel/fractional-border-location-and-size.html:1: <html> On 2015/03/04 19:32:34, leviw wrote: > Nit: doctype? Done. https://codereview.chromium.org/976543002/diff/1/LayoutTests/fast/sub-pixel/f... LayoutTests/fast/sub-pixel/fractional-border-location-and-size.html:12: font-size: 534%; On 2015/03/04 19:32:34, leviw wrote: > Nit: can we remove unnecessary styling? We shouldn't need font-size, and perhaps > not padding either. Padding and font size are related to the border size and they are necessary for producing the bug.
> https://codereview.chromium.org/976543002/diff/1/LayoutTests/fast/sub-pixel/f... > LayoutTests/fast/sub-pixel/fractional-border-location-and-size.html:12: font-size: 534%; > On 2015/03/04 19:32:34, leviw wrote: > > Nit: can we remove unnecessary styling? We shouldn't need font-size, and perhaps > > not padding either. > > Padding and font size are related to the border size and they are necessary for producing the bug. I'm not sure I follow... You should be able to position the borders independently of both these properties.
On 2015/03/05 10:00:59, leviw wrote: > > > https://codereview.chromium.org/976543002/diff/1/LayoutTests/fast/sub-pixel/f... > > LayoutTests/fast/sub-pixel/fractional-border-location-and-size.html:12: > font-size: 534%; > > On 2015/03/04 19:32:34, leviw wrote: > > > Nit: can we remove unnecessary styling? We shouldn't need font-size, and > perhaps > > > not padding either. > > > > Padding and font size are related to the border size and they are necessary > for producing the bug. > > I'm not sure I follow... You should be able to position the borders > independently of both these properties. DONE.
LGTM.
The CQ bit was checked by qiankun.miao@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/976543002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/5...)
Updated the patch to fix multi-column-layout tests failure. Can I accept try-bot permission for pre-commit testing?
On 2015/03/09 09:42:12, qiankun wrote: > Updated the patch to fix multi-column-layout tests failure. Can I accept try-bot > permission for pre-commit testing? accept should be apply.
Hi, I don't see the multi-column fix in this patchset. ?
Thanks for review. Could you help me to trigger try-bots? https://codereview.chromium.org/976543002/diff/60001/Source/core/layout/Layer... File Source/core/layout/Layer.cpp (right): https://codereview.chromium.org/976543002/diff/60001/Source/core/layout/Layer... Source/core/layout/Layer.cpp:1639: fragment.moveBy(fragment.paginationOffset + offsetOfPaginationLayerFromRoot + subPixelAccumulationIfNeeded); Hi Chris, this code is what I said fixed the multi-column tests: virtual/regionbasedmulticol/fast/multicol/composited-with-overflow-in-next-column.html virtual/slimmingpaint/fast/multicol/composited-with-overflow-in-next-column.html
The CQ bit was checked by leviw@chromium.org
LGTM.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/976543002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_blink_compile_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
On 2015/03/10 03:14:50, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > android_blink_compile_rel on tryserver.blink (JOB_FAILED, > http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...) Rebased to latest code.
LGTM
leviw@chromium.org changed reviewers: + pdr@chromium.org
LGTPDR
The CQ bit was checked by qiankun.miao@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org Link to the patchset: https://codereview.chromium.org/976543002/#ps80001 (title: "update to ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/976543002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_blink_compile_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
The CQ bit was checked by qiankun.miao@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org, pdr@chromium.org Link to the patchset: https://codereview.chromium.org/976543002/#ps100001 (title: "fix build failure: renderer -> layoutObject")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/976543002/100001
The CQ bit was unchecked by qiankun.miao@intel.com
Sorry to bother you again. PTAL. https://codereview.chromium.org/976543002/diff/120001/Source/core/layout/Laye... File Source/core/layout/Layer.cpp (right): https://codereview.chromium.org/976543002/diff/120001/Source/core/layout/Laye... Source/core/layout/Layer.cpp:1631: if (isAllowedToQueryCompositingState()) Check isAllowedToQueryCompositingState() to make sure compositingState() is available, and make following layout test happy: virtual/regionbasedmulticol/fast/multicol/hit-test-float.html virtual/slimmingpaint/fast/multicol/hit-test-translate-z.html virtual/slimmingpaint/fast/multicol/hit-test-float.html virtual/regionbasedmulticol/fast/multicol/hit-test-translate-z.html
https://codereview.chromium.org/976543002/diff/120001/Source/core/layout/Laye... File Source/core/layout/Layer.cpp (right): https://codereview.chromium.org/976543002/diff/120001/Source/core/layout/Laye... Source/core/layout/Layer.cpp:1631: if (isAllowedToQueryCompositingState()) On 2015/03/10 at 10:13:16, qiankun wrote: > Check isAllowedToQueryCompositingState() to make sure compositingState() is available, and make following layout test happy: > virtual/regionbasedmulticol/fast/multicol/hit-test-float.html > virtual/slimmingpaint/fast/multicol/hit-test-translate-z.html > virtual/slimmingpaint/fast/multicol/hit-test-float.html > virtual/regionbasedmulticol/fast/multicol/hit-test-translate-z.html This isn't the right fix. I talked to chrishtr and we agreed that we should ensure that the document lifecycle is advanced to CompositingClean prior to hit testing. That would avoid the issue and assert and unblock your previous change. I suggest doing that in a separate patch.
On 2015/03/10 17:55:51, leviw wrote: > https://codereview.chromium.org/976543002/diff/120001/Source/core/layout/Laye... > File Source/core/layout/Layer.cpp (right): > > https://codereview.chromium.org/976543002/diff/120001/Source/core/layout/Laye... > Source/core/layout/Layer.cpp:1631: if (isAllowedToQueryCompositingState()) > On 2015/03/10 at 10:13:16, qiankun wrote: > > Check isAllowedToQueryCompositingState() to make sure compositingState() is > available, and make following layout test happy: > > virtual/regionbasedmulticol/fast/multicol/hit-test-float.html > > virtual/slimmingpaint/fast/multicol/hit-test-translate-z.html > > virtual/slimmingpaint/fast/multicol/hit-test-float.html > > virtual/regionbasedmulticol/fast/multicol/hit-test-translate-z.html > > This isn't the right fix. > > I talked to chrishtr and we agreed that we should ensure that the document > lifecycle is advanced to CompositingClean prior to hit testing. That would avoid > the issue and assert and unblock your previous change. I suggest doing that in a > separate patch. Done in https://codereview.chromium.org/999033002/.
lgtm
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
https://codereview.chromium.org/976543002/diff/140001/LayoutTests/fast/sub-pi... File LayoutTests/fast/sub-pixel/fractional-border-location-and-size-expected.html (right): https://codereview.chromium.org/976543002/diff/140001/LayoutTests/fast/sub-pi... LayoutTests/fast/sub-pixel/fractional-border-location-and-size-expected.html:4: <title>Fractional border</title> leave out html, head and title. https://codereview.chromium.org/976543002/diff/140001/LayoutTests/fast/sub-pi... LayoutTests/fast/sub-pixel/fractional-border-location-and-size-expected.html:17: <body> remove body. https://codereview.chromium.org/976543002/diff/140001/LayoutTests/fast/sub-pi... File LayoutTests/fast/sub-pixel/fractional-border-location-and-size.html (right): https://codereview.chromium.org/976543002/diff/140001/LayoutTests/fast/sub-pi... LayoutTests/fast/sub-pixel/fractional-border-location-and-size.html:4: <title>Fractional border</title> ditto, all this markup isn't needed. https://codereview.chromium.org/976543002/diff/140001/Source/core/layout/Laye... File Source/core/layout/Layer.cpp (right): https://codereview.chromium.org/976543002/diff/140001/Source/core/layout/Laye... Source/core/layout/Layer.cpp:1638: fragment.moveBy(fragment.paginationOffset + offsetOfPaginationLayerFromRoot + subPixelAccumulationIfNeeded); extra space
Thanks esprehn for review. https://codereview.chromium.org/976543002/diff/140001/LayoutTests/fast/sub-pi... File LayoutTests/fast/sub-pixel/fractional-border-location-and-size-expected.html (right): https://codereview.chromium.org/976543002/diff/140001/LayoutTests/fast/sub-pi... LayoutTests/fast/sub-pixel/fractional-border-location-and-size-expected.html:4: <title>Fractional border</title> On 2015/03/24 23:13:23, esprehn wrote: > leave out html, head and title. Done. https://codereview.chromium.org/976543002/diff/140001/LayoutTests/fast/sub-pi... LayoutTests/fast/sub-pixel/fractional-border-location-and-size-expected.html:17: <body> On 2015/03/24 23:13:23, esprehn wrote: > remove body. Done. https://codereview.chromium.org/976543002/diff/140001/LayoutTests/fast/sub-pi... File LayoutTests/fast/sub-pixel/fractional-border-location-and-size.html (right): https://codereview.chromium.org/976543002/diff/140001/LayoutTests/fast/sub-pi... LayoutTests/fast/sub-pixel/fractional-border-location-and-size.html:4: <title>Fractional border</title> On 2015/03/24 23:13:23, esprehn wrote: > ditto, all this markup isn't needed. Done. https://codereview.chromium.org/976543002/diff/140001/Source/core/layout/Laye... File Source/core/layout/Layer.cpp (right): https://codereview.chromium.org/976543002/diff/140001/Source/core/layout/Laye... Source/core/layout/Layer.cpp:1638: fragment.moveBy(fragment.paginationOffset + offsetOfPaginationLayerFromRoot + subPixelAccumulationIfNeeded); On 2015/03/24 23:13:23, esprehn wrote: > extra space Done.
lgtm https://codereview.chromium.org/976543002/diff/160001/LayoutTests/fast/sub-pi... File LayoutTests/fast/sub-pixel/fractional-border-location-and-size.html (right): https://codereview.chromium.org/976543002/diff/160001/LayoutTests/fast/sub-pi... LayoutTests/fast/sub-pixel/fractional-border-location-and-size.html:4: transform:scale(1); nit missing space after :
The CQ bit was checked by qiankun.miao@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org, leviw@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/976543002/#ps180001 (title: "add missing space")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/976543002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://src.chromium.org/viewvc/blink?view=rev&revision=193611
Message was sent while issue was closed.
How exactly is https://codereview.chromium.org/999033002/ directly related to this CL? I can still get the test from this CL to pass even if CL 999033002 is reverted.
Message was sent while issue was closed.
On 2015/05/05 at 22:25:23, chrishtr wrote: > How exactly is https://codereview.chromium.org/999033002/ directly related to this CL? I can still get the test from this CL to pass even if CL 999033002 is reverted. Can you get the tests from c#35 to pass as well? I don't think it was claimed that the test on this CL was crashing without https://codereview.chromium.org/999033002/
Message was sent while issue was closed.
On 2015/05/05 at 22:27:57, leviw wrote: > On 2015/05/05 at 22:25:23, chrishtr wrote: > > How exactly is https://codereview.chromium.org/999033002/ directly related to this CL? I can still get the test from this CL to pass even if CL 999033002 is reverted. > > Can you get the tests from c#35 to pass as well? I don't think it was claimed that the test on this CL was crashing without https://codereview.chromium.org/999033002/ Those other tests crash. I just don't get the connection between the two CLs.
Message was sent while issue was closed.
On 2015/05/05 at 22:33:08, chrishtr wrote: > On 2015/05/05 at 22:27:57, leviw wrote: > > On 2015/05/05 at 22:25:23, chrishtr wrote: > > > How exactly is https://codereview.chromium.org/999033002/ directly related to this CL? I can still get the test from this CL to pass even if CL 999033002 is reverted. > > > > Can you get the tests from c#35 to pass as well? I don't think it was claimed that the test on this CL was crashing without https://codereview.chromium.org/999033002/ > > Those other tests crash. I just don't get the connection between the two CLs. I'm not sure I understand... The change in this patch would cause those tests to crash without the other CL. I wanted that other CL split out first since it's not actually about pixel snapping.
Message was sent while issue was closed.
On 2015/05/05 at 22:35:47, leviw wrote: > On 2015/05/05 at 22:33:08, chrishtr wrote: > > On 2015/05/05 at 22:27:57, leviw wrote: > > > On 2015/05/05 at 22:25:23, chrishtr wrote: > > > > How exactly is https://codereview.chromium.org/999033002/ directly related to this CL? I can still get the test from this CL to pass even if CL 999033002 is reverted. > > > > > > Can you get the tests from c#35 to pass as well? I don't think it was claimed that the test on this CL was crashing without https://codereview.chromium.org/999033002/ > > > > Those other tests crash. I just don't get the connection between the two CLs. > > I'm not sure I understand... The change in this patch would cause those tests to crash without the other CL. I wanted that other CL split out first since it's not actually about pixel snapping. I'm just not sure why those tests would crash with this patch and without the other one...trying to grok how. (I tried to revert this patch but it didn't go out cleanly.) I suspect it was coincidental because compositingState happened to get called, and this code was not the root cause. I have a pending patch that I think improves things.
Message was sent while issue was closed.
On 2015/05/05 at 22:40:47, chrishtr wrote: > On 2015/05/05 at 22:35:47, leviw wrote: > > On 2015/05/05 at 22:33:08, chrishtr wrote: > > > On 2015/05/05 at 22:27:57, leviw wrote: > > > > On 2015/05/05 at 22:25:23, chrishtr wrote: > > > > > How exactly is https://codereview.chromium.org/999033002/ directly related to this CL? I can still get the test from this CL to pass even if CL 999033002 is reverted. > > > > > > > > Can you get the tests from c#35 to pass as well? I don't think it was claimed that the test on this CL was crashing without https://codereview.chromium.org/999033002/ > > > > > > Those other tests crash. I just don't get the connection between the two CLs. > > > > I'm not sure I understand... The change in this patch would cause those tests to crash without the other CL. I wanted that other CL split out first since it's not actually about pixel snapping. > > I'm just not sure why those tests would crash with this patch and without the other one...trying to grok how. (I tried to revert this patch but it didn't go out cleanly.) I suspect it was coincidental because compositingState happened to get called, and this code was not the root cause. I have a pending patch that I think improves things. This patch adds the compositingState() check to collectFragments. collectFragments is used for hit testing as well as paint. That's why it causes those tests to fail.
Message was sent while issue was closed.
On 2015/05/05 22:25:23, chrishtr wrote: > How exactly is https://codereview.chromium.org/999033002/ directly related to > this CL? I can still get the test from this CL to pass even if CL 999033002 is > reverted. After reverting CL 999033002, I got crash for the tests in comments#35. Do you run release or debug layout tests? I can get crash with debug version locally but not release.
Message was sent while issue was closed.
On 2015/05/06 at 03:23:57, qiankun.miao wrote: > On 2015/05/05 22:25:23, chrishtr wrote: > > How exactly is https://codereview.chromium.org/999033002/ directly related to > > this CL? I can still get the test from this CL to pass even if CL 999033002 is > > reverted. > > After reverting CL 999033002, I got crash for the tests in comments#35. Do you run release or debug layout tests? I can get crash with debug version locally but not release. I was able to get them to crash, yes. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
