Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2688413002:
Calculate glyph overflow when computing overflow outside of layout (Closed)
|
Created:
3 years, 10 months ago by rhogan Modified:
3 years, 10 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCalculate glyph overflow when computing overflow outside of layout
Compute glyph overflow on the fly when we don't have it from a layout
pass. This ensures any glyph overflow is properly invalidated when we're
doing an overflow-only recalc after a style change.
BUG=667245
Review-Url: https://codereview.chromium.org/2688413002
Cr-Commit-Position: refs/heads/master@{#453087}
Committed: https://chromium.googlesource.com/chromium/src/+/38feb8be2c217b36fd46b228a69e31337fffe67e
Patch Set 1 #Patch Set 2 : 667245 #Patch Set 3 : 667245 #
Messages
Total messages: 26 (16 generated)
The CQ bit was checked by robhogan@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by robhogan@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
robhogan@gmail.com changed reviewers: + pdr@chromium.org, wangxianzhu@chromium.org
Hi there, Could you take a look at my test? The issue is easy to reproduce manually but I've struggled to make a functioning layout test of it.
Description was changed from ========== Repaint overflow on lineboxes adjacent to form elements BUG=667245 ========== to ========== Calculate glyph overflow when computing overflow outside of layout Compute glyph overflow on the fly when we don't have it from a layout pass. This ensures any glyph overflow is properly invalidated when we're doing an overflow-only recalc after a style change. BUG=667245 ==========
On 2017/02/14 20:50:54, rhogan wrote: > Hi there, > > Could you take a look at my test? The issue is easy to reproduce manually but > I've struggled to make a functioning layout test of it. I tried the test in ToT chrome. It seems that sometimes the black block is not fully painted initially (without the glyph overflow painted), so when it is hidden, there is no residue even if the paint invalidation rect is not big enough. I think you can change the test to make the initial color of the glyph red, then in repaintTest() change it to green. With the bug, there may be either red residue, or the final green block is smaller than expected. Ref test seems not a good way to test this, because the reference may also be affected by the same bug and produces the same result as the test. Text layout experts drott@ and eae@ should be better reviewers than me for this change.
On 2017/02/14 at 21:14:21, wangxianzhu wrote: > On 2017/02/14 20:50:54, rhogan wrote: > > Hi there, > > > > Could you take a look at my test? The issue is easy to reproduce manually but > > I've struggled to make a functioning layout test of it. > > I tried the test in ToT chrome. It seems that sometimes the black block is not fully painted initially (without the glyph overflow painted), so when it is hidden, there is no residue even if the paint invalidation rect is not big enough. I think you can change the test to make the initial color of the glyph red, then in repaintTest() change it to green. With the bug, there may be either red residue, or the final green block is smaller than expected. > > Ref test seems not a good way to test this, because the reference may also be affected by the same bug and produces the same result as the test. > > Text layout experts drott@ and eae@ should be better reviewers than me for this change. Still unable to get it working in layout test form - I suspect it's because the theme used for form elements in layout tests differs from that used by content_shell/chrome. I haven't been able to divine why that prevents it working in layout. Is it reasonable to give up and add it as a ManualTest?
robhogan@gmail.com changed reviewers: + drott@chromium.org, eae@chromium.org - pdr@chromium.org, wangxianzhu@chromium.org
robhogan@gmail.com changed reviewers: + wangxianzhu@chromium.org
On 2017/02/15 20:22:41, rhogan wrote: > On 2017/02/14 at 21:14:21, wangxianzhu wrote: > > On 2017/02/14 20:50:54, rhogan wrote: > > > Hi there, > > > > > > Could you take a look at my test? The issue is easy to reproduce manually > but > > > I've struggled to make a functioning layout test of it. > > > > I tried the test in ToT chrome. It seems that sometimes the black block is not > fully painted initially (without the glyph overflow painted), so when it is > hidden, there is no residue even if the paint invalidation rect is not big > enough. I think you can change the test to make the initial color of the glyph > red, then in repaintTest() change it to green. With the bug, there may be either > red residue, or the final green block is smaller than expected. > > > > Ref test seems not a good way to test this, because the reference may also be > affected by the same bug and produces the same result as the test. > > > > Text layout experts drott@ and eae@ should be better reviewers than me for > this change. > > Still unable to get it working in layout test form - I suspect it's because the > theme used for form elements in layout tests differs from that used by > content_shell/chrome. I haven't been able to divine why that prevents it working > in layout. > > Is it reasonable to give up and add it as a ManualTest? Can you add a unit test just testing the behavior of visual overflow calculation without layout? I also have several questions: 1. Where is the normal visual overflow calculation code that is executed during layout? 2. How does a form element trigger LineFlowBox overflow calculation without layout? 3. Are there other cases that a LineFlowBox calculates overflow without layout?
LGTM for code change. This code is hard to unit test given the dependencies sadly. We're fixing that with LayoutNG but in the current layout world it is rather tricky.
On 2017/02/21 at 06:53:59, wangxianzhu wrote: > 1. Where is the normal visual overflow calculation code that is executed during layout? This is done in LayoutBlockFlowLine, and the overflow glyphmap computed during layout is kept around until overflow is calculated. > 2. How does a form element trigger LineFlowBox overflow calculation without layout? Same as any other block-flow element really - outline change, etc. > 3. Are there other cases that a LineFlowBox calculates overflow without layout? No, just the appropropriate overflow-only style changes.
On 2017/02/24 at 04:00:33, eae wrote: > LGTM for code change. This code is hard to unit test given the dependencies sadly. We're fixing that with LayoutNG but in the current layout world it is rather tricky. OK to land a manual test rather than a layout test?
On 2017/02/24 16:17:04, rhogan wrote: > On 2017/02/24 at 04:00:33, eae wrote: > > LGTM for code change. This code is hard to unit test given the dependencies > sadly. We're fixing that with LayoutNG but in the current layout world it is > rather tricky. > > OK to land a manual test rather than a layout test? I'm OK with it. lgtm.
The CQ bit was checked by robhogan@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from wangxianzhu@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/2688413002/#ps40001 (title: "667245")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488023365491750,
"parent_rev": "220ff33573e9adaadc4264dab5b2bbbbe0d5e7f8", "commit_rev":
"38feb8be2c217b36fd46b228a69e31337fffe67e"}
Message was sent while issue was closed.
Description was changed from ========== Calculate glyph overflow when computing overflow outside of layout Compute glyph overflow on the fly when we don't have it from a layout pass. This ensures any glyph overflow is properly invalidated when we're doing an overflow-only recalc after a style change. BUG=667245 ========== to ========== Calculate glyph overflow when computing overflow outside of layout Compute glyph overflow on the fly when we don't have it from a layout pass. This ensures any glyph overflow is properly invalidated when we're doing an overflow-only recalc after a style change. BUG=667245 Review-Url: https://codereview.chromium.org/2688413002 Cr-Commit-Position: refs/heads/master@{#453087} Committed: https://chromium.googlesource.com/chromium/src/+/38feb8be2c217b36fd46b228a69e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/38feb8be2c217b36fd46b228a69e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
