Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2683553002:
Fix not autosizing ruby elements issue. (Closed)
|
Created:
3 years, 10 months ago by cathiechentx 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. |
DescriptionFix not autosizing ruby elements issue.
RubyRun is inline-block. When inflate ruby, skip rubyRun and inflate all rubyRun's inner blocks.
BUG=688561
Review-Url: https://codereview.chromium.org/2683553002
Cr-Commit-Position: refs/heads/master@{#450897}
Committed: https://chromium.googlesource.com/chromium/src/+/082b073b26e5f408462e4fd1f229d7614ace4364
Patch Set 1 #Patch Set 2 : delete unused include #
Total comments: 1
Patch Set 3 : update comments #
Total comments: 9
Patch Set 4 : add testcase and fix other issues #Patch Set 5 : Use MarkContainerChain for setPreferredLogicalWidthsDirty, when we skipped inline-block before #
Total comments: 10
Patch Set 6 : add testcase resizeRubyPage #Patch Set 7 : update testcase #
Total comments: 1
Patch Set 8 : 1. fix the dcheck fire when sreen ratated and add test case 2. format TextAutosizerTest #
Total comments: 4
Patch Set 9 : cr #
Messages
Total messages: 28 (9 generated)
Description was changed from ========== Fix not autosizing ruby elements issue. RubyRun is inline-block. When inflate ruby, skip rubyRun and inflate all ruby's inner blocks. BUG=688561 ========== to ========== Fix not autosizing ruby elements issue. RubyRun is inline-block. When inflate ruby, skip rubyRun and inflate all other ruby's inner blocks. BUG=688561 ==========
cathiechen@tencent.com changed reviewers: + jamesxwei@tencent.com, mukai@chromium.org, pdr@chromium.org, skobes@chromium.org
Description was changed from ========== Fix not autosizing ruby elements issue. RubyRun is inline-block. When inflate ruby, skip rubyRun and inflate all other ruby's inner blocks. BUG=688561 ========== to ========== Fix not autosizing ruby elements issue. RubyRun is inline-block. When inflate ruby, skip rubyRun and inflate all rubyRun's inner blocks. BUG=688561 ==========
https://codereview.chromium.org/2683553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2683553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:478: if (parent->isRuby()) { Cause ruby could be block or inline, deal with ruby's inner blocks in inflate(), to make sure both block and inline ruby elements be handled.
mstensho@opera.com changed reviewers: + mstensho@opera.com
This CL lacks a test. https://codereview.chromium.org/2683553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2683553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:33: #include <memory> Shouldn't have to move this up here. https://codereview.chromium.org/2683553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:404: // Skip ruby's inner blocks, cause these blocks already be inflated. "because these blocks are already inflated"? https://codereview.chromium.org/2683553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:481: if (parent->slowFirstChild() && parent->slowFirstChild()->isRubyRun()) { Might want to store parent->slowFirstChild() as a variable, instead of calling it three times.
This also looks reasonable to me, but please add a unittest https://codereview.chromium.org/2683553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2683553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:33: #include <memory> On 2017/02/07 at 10:32:36, mstensho wrote: > Shouldn't have to move this up here. I think it's okay, it seems to be the new clang formatter behavior: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/E_zrZQQCU3E/s2oAV... https://codereview.chromium.org/2683553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:486: (parent->childrenInline() || behavior == DescendToInnerBlocks)) style nit: because curly braces were added for the first conditional, please add curlies to the whole if-elseif-elseif chain: if (parent->isRuby()) { ... } else if (parent->isLayoutBlock() && (parent->childrenInline() || behavior == DescendToInnerBlocks)) { child = toLayoutInline(parent)->firstChild(); } else if (parent->isLayoutInline()) { child = toLayoutInline(parent)->firstChild(); } https://google.github.io/styleguide/cppguide.html#Conditionals
Thanks for reviewing :) Almost done with the issues. But I just found the position went wrong when screen rotated. Still working on this! https://codereview.chromium.org/2683553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2683553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:33: #include <memory> On 2017/02/08 04:45:33, pdr. wrote: > On 2017/02/07 at 10:32:36, mstensho wrote: > > Shouldn't have to move this up here. > > I think it's okay, it seems to be the new clang formatter behavior: > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/E_zrZQQCU3E/s2oAV... yes, "git cl format" did this:) https://codereview.chromium.org/2683553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:404: // Skip ruby's inner blocks, cause these blocks already be inflated. On 2017/02/07 10:32:36, mstensho wrote: > "because these blocks are already inflated"? Done. https://codereview.chromium.org/2683553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:481: if (parent->slowFirstChild() && parent->slowFirstChild()->isRubyRun()) { On 2017/02/07 10:32:36, mstensho wrote: > Might want to store parent->slowFirstChild() as a variable, instead of calling > it three times. Done. https://codereview.chromium.org/2683553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:486: (parent->childrenInline() || behavior == DescendToInnerBlocks)) On 2017/02/08 04:45:33, pdr. wrote: > style nit: because curly braces were added for the first conditional, please add > curlies to the whole if-elseif-elseif chain: > > if (parent->isRuby()) { > ... > } else if (parent->isLayoutBlock() && > (parent->childrenInline() || behavior == DescendToInnerBlocks)) { > child = toLayoutInline(parent)->firstChild(); > } else if (parent->isLayoutInline()) { > child = toLayoutInline(parent)->firstChild(); > } > > https://google.github.io/styleguide/cppguide.html#Conditionals Done.
https://codereview.chromium.org/2683553002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/2683553002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:1391: if (!preferredLogicalWidthsDirty()) { I think this dcheck should still work. Do you know why it needs the prefWidths check? https://codereview.chromium.org/2683553002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2683553002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:404: // Skip ruby's inner blocks, because these blocks already be inflated. nit: "...these blocks already are inflated." https://codereview.chromium.org/2683553002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:506: if (behavior == SkippedInlineBlockAndDescendToInnerBlocks) Nit: setPreferredLogicalWidthsDirty has lost its comment. I'd recommend something like: if (behavior == SkippedInlineBlockAndDescendToInnerBlocks) { child->setPreferredLogicalWidthsDirty(); } else if (parent->isLayoutInline()) { // FIXME: Investigate why MarkOnlyThis is sufficient. child->setPreferredLogicalWidthsDirty(MarkOnlyThis); } Btw: I looked into this a little and I think we might just be able to remove MarkOnlyThis. It was added in https://codereview.chromium.org/218863002 but no tests fail if it's removed. Would removing that and always using MarkContainerChain make this patch simpler? I.e., could you just set behavior = DescendToInnerBlocks for ruby instead of setting it to SkippedInlineBlockAndDescendToInnerBlocks?
https://codereview.chromium.org/2683553002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/2683553002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:1391: if (!preferredLogicalWidthsDirty()) { On 2017/02/10 04:10:46, pdr. wrote: > I think this dcheck should still work. Do you know why it needs the prefWidths > check? Hesitated to make this change too. The reason to fail this dcheck is: 1. After the first pass of layout, m_knownToHaveNoOverflowAndNoFallbackFonts is true. Resized. Triggle layout. 2. Layout ruby. When autosize ruby, we changed ruby's inner layouttext's style by setStyleInternal() in applyMultiplier(). setStyleInternal() won't change the state of m_knownToHaveNoOverflowAndNoFallbackFonts. So m_knownToHaveNoOverflowAndNoFallbackFonts remains true. 3. Layout rubyRun, rubyRun needs recalculate prefWidth. So it comes to here with "m_knownToHaveNoOverflowAndNoFallbackFonts==true". Maybe we could set m_knownToHaveNoOverflowAndNoFallbackFonts to false after we changed the style of layouttext by setStyleInternal()? https://codereview.chromium.org/2683553002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2683553002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:404: // Skip ruby's inner blocks, because these blocks already be inflated. On 2017/02/10 04:10:46, pdr. wrote: > nit: "...these blocks already are inflated." Got it. Thank you :) https://codereview.chromium.org/2683553002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:506: if (behavior == SkippedInlineBlockAndDescendToInnerBlocks) On 2017/02/10 04:10:46, pdr. wrote: > Nit: setPreferredLogicalWidthsDirty has lost its comment. I'd recommend > something like: > if (behavior == SkippedInlineBlockAndDescendToInnerBlocks) { > child->setPreferredLogicalWidthsDirty(); > } else if (parent->isLayoutInline()) { > // FIXME: Investigate why MarkOnlyThis is sufficient. > child->setPreferredLogicalWidthsDirty(MarkOnlyThis); > } > > Btw: I looked into this a little and I think we might just be able to remove > MarkOnlyThis. It was added in https://codereview.chromium.org/218863002 but no > tests fail if it's removed. Would removing that and always using > MarkContainerChain make this patch simpler? I.e., could you just set behavior = > DescendToInnerBlocks for ruby instead of setting it to > SkippedInlineBlockAndDescendToInnerBlocks? yes, it's much simpler. But rubyText is a block;( rubyrun rubyBase(block) inline(<rb>) text rubyText(block) text(parent is block) How about this? if (behavior == DescendToInnerBlocks) child->setPreferredLogicalWidthsDirty(); else if (parent->isLayoutInline()) child->setPreferredLogicalWidthsDirty(MarkOnlyThis); It will make table stuff setPreferredLogicalWidthsDirty too, but I think it's reasonable.
https://codereview.chromium.org/2683553002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/2683553002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:1391: if (!preferredLogicalWidthsDirty()) { On 2017/02/10 at 05:17:47, cathiechentx wrote: > On 2017/02/10 04:10:46, pdr. wrote: > > I think this dcheck should still work. Do you know why it needs the prefWidths > > check? > > Hesitated to make this change too. The reason to fail this dcheck is: > > 1. After the first pass of layout, m_knownToHaveNoOverflowAndNoFallbackFonts is true. Resized. Triggle layout. > > 2. Layout ruby. When autosize ruby, we changed ruby's inner layouttext's style by setStyleInternal() in applyMultiplier(). setStyleInternal() won't change the state of m_knownToHaveNoOverflowAndNoFallbackFonts. So m_knownToHaveNoOverflowAndNoFallbackFonts remains true. > > 3. Layout rubyRun, rubyRun needs recalculate prefWidth. So it comes to here with "m_knownToHaveNoOverflowAndNoFallbackFonts==true". > > Maybe we could set m_knownToHaveNoOverflowAndNoFallbackFonts to false after we changed the style of layouttext by setStyleInternal()? Can you add a test for this? Just naively removing the conditional doesn't make any tests hit this assert, but I see what you mean about how it can be hit. If this dcheck is firing, is it because we have glyphOverflow when we didn't before? I wonder if we need to force the entire ruby run to be recalculated (maybe using setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation instead of setNeedsLayoutAndFullPaintInvalidation)? https://codereview.chromium.org/2683553002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right): https://codereview.chromium.org/2683553002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/TextAutosizer.cpp:506: if (behavior == SkippedInlineBlockAndDescendToInnerBlocks) On 2017/02/10 at 05:17:47, cathiechentx wrote: > On 2017/02/10 04:10:46, pdr. wrote: > > Nit: setPreferredLogicalWidthsDirty has lost its comment. I'd recommend > > something like: > > if (behavior == SkippedInlineBlockAndDescendToInnerBlocks) { > > child->setPreferredLogicalWidthsDirty(); > > } else if (parent->isLayoutInline()) { > > // FIXME: Investigate why MarkOnlyThis is sufficient. > > child->setPreferredLogicalWidthsDirty(MarkOnlyThis); > > } > > > > Btw: I looked into this a little and I think we might just be able to remove > > MarkOnlyThis. It was added in https://codereview.chromium.org/218863002 but no > > tests fail if it's removed. Would removing that and always using > > MarkContainerChain make this patch simpler? I.e., could you just set behavior = > > DescendToInnerBlocks for ruby instead of setting it to > > SkippedInlineBlockAndDescendToInnerBlocks? > > yes, it's much simpler. But rubyText is a block;( > rubyrun > rubyBase(block) > inline(<rb>) > text > rubyText(block) > text(parent is block) > > How about this? > if (behavior == DescendToInnerBlocks) > child->setPreferredLogicalWidthsDirty(); > else if (parent->isLayoutInline()) > child->setPreferredLogicalWidthsDirty(MarkOnlyThis); > > It will make table stuff setPreferredLogicalWidthsDirty too, but I think it's reasonable. Yeah, something like that seems good (just don't forget to keep the comment "// FIXME: Investigate why MarkOnlyThis is sufficient.")
https://codereview.chromium.org/2683553002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/2683553002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:1391: if (!preferredLogicalWidthsDirty()) { On 2017/02/10 19:27:09, pdr. wrote: > On 2017/02/10 at 05:17:47, cathiechentx wrote: > > On 2017/02/10 04:10:46, pdr. wrote: > > > I think this dcheck should still work. Do you know why it needs the > prefWidths > > > check? > > > > Hesitated to make this change too. The reason to fail this dcheck is: > > > > 1. After the first pass of layout, m_knownToHaveNoOverflowAndNoFallbackFonts > is true. Resized. Triggle layout. > > > > 2. Layout ruby. When autosize ruby, we changed ruby's inner layouttext's style > by setStyleInternal() in applyMultiplier(). setStyleInternal() won't change the > state of m_knownToHaveNoOverflowAndNoFallbackFonts. So > m_knownToHaveNoOverflowAndNoFallbackFonts remains true. > > > > 3. Layout rubyRun, rubyRun needs recalculate prefWidth. So it comes to here > with "m_knownToHaveNoOverflowAndNoFallbackFonts==true". > > > > Maybe we could set m_knownToHaveNoOverflowAndNoFallbackFonts to false after we > changed the style of layouttext by setStyleInternal()? > > Can you add a test for this? Just naively removing the conditional doesn't make > any tests hit this assert, but I see what you mean about how it can be hit. > > If this dcheck is firing, is it because we have glyphOverflow when we didn't > before? I wonder if we need to force the entire ruby run to be recalculated > (maybe using setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation instead > of setNeedsLayoutAndFullPaintInvalidation)? fallbackFonts is not empty... https://codereview.chromium.org/2683553002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp (right): https://codereview.chromium.org/2683553002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TextAutosizerTest.cpp:717: TEST_F(TextAutosizerTest, ResizeRubyPage) { To produce DCHECK fail in LayoutText, we should use these exact numbers. "360 * 640" which is from the window size of Sumsung Galoxy S4, and the body layout size must be 784px(800 - 16)... I'm afraid my analysis before goes wrong. Keep on looking into this tomorrow:)
https://codereview.chromium.org/2683553002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/2683553002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutText.cpp:1391: if (!preferredLogicalWidthsDirty()) { On 2017/02/13 14:53:56, cathiechentx wrote: > On 2017/02/10 19:27:09, pdr. wrote: > > On 2017/02/10 at 05:17:47, cathiechentx wrote: > > > On 2017/02/10 04:10:46, pdr. wrote: > > > > I think this dcheck should still work. Do you know why it needs the > > prefWidths > > > > check? > > > > > > Hesitated to make this change too. The reason to fail this dcheck is: > > > > > > 1. After the first pass of layout, m_knownToHaveNoOverflowAndNoFallbackFonts > > is true. Resized. Triggle layout. > > > > > > 2. Layout ruby. When autosize ruby, we changed ruby's inner layouttext's > style > > by setStyleInternal() in applyMultiplier(). setStyleInternal() won't change > the > > state of m_knownToHaveNoOverflowAndNoFallbackFonts. So > > m_knownToHaveNoOverflowAndNoFallbackFonts remains true. > > > > > > 3. Layout rubyRun, rubyRun needs recalculate prefWidth. So it comes to here > > with "m_knownToHaveNoOverflowAndNoFallbackFonts==true". > > > > > > Maybe we could set m_knownToHaveNoOverflowAndNoFallbackFonts to false after > we > > changed the style of layouttext by setStyleInternal()? > > > > Can you add a test for this? Just naively removing the conditional doesn't > make > > any tests hit this assert, but I see what you mean about how it can be hit. > > > > If this dcheck is firing, is it because we have glyphOverflow when we didn't > > before? I wonder if we need to force the entire ruby run to be recalculated > > (maybe using setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation instead > > of setNeedsLayoutAndFullPaintInvalidation)? > Update:) Actuall fallbackFonts is empty. "If this dcheck is firing, is it because we have glyphOverflow when we didn't before?" Yes! The glyphOverflow of some font-family is different when font-size is changed. Analysis: The below is the log of glyphOverflow printing before that 'DCHECK'. They are the glyphOverflow of 'n' with 'font-family:Times New Roman;' and font-size from 12px to 30px. They are printed in SumSung Glaxy S4. As we can see glyphOverflow is different when font-size is changed. As a result of autosize the font-size could be changed when screen rotated. It could change from glyphOverflow.isApproximatelyZero==true to glyphOverflow.isApproximatelyZero==false. So m_knownToHaveNoOverflowAndNoFallbackFonts could change from true to false after screen rotated. The test case 'ResizeAndGlyphOverflowChanged' could reproduce this issue. Solution: I think its reasonable to set 'm_knownToHaveNoOverflowAndNoFallbackFonts' false of layoutText after this layoutText's computeSize is changed in applyMultiplier() by setStyleInternal(). We do not know if there any change of glyphOverflow. Log: glyphOverflow=(0.000000, 0.259766, 0.000000, 0.000000) glyphOverflow=(0.000000, 0.614746, 0.000000, 0.000000) glyphOverflow=(0.000000, 0.000000, 0.000000, 0.000000) glyphOverflow=(0.000000, 0.324707, 0.000000, 0.000000) glyphOverflow=(0.000000, 0.000000, 0.000000, 0.000000) glyphOverflow=(0.000000, 0.034668, 0.000000, 0.000000) glyphOverflow=(0.000000, 0.389648, 0.000000, 0.000000) glyphOverflow=(0.000000, 0.000000, 0.000000, 0.000000) glyphOverflow=(0.000000, 0.099609, 0.000000, 0.000000) glyphOverflow=(0.000000, 0.000000, 0.000000, 0.000000) glyphOverflow=(0.000000, 0.000000, 0.000000, 0.000000) glyphOverflow=(0.000000, 0.164551, 0.000000, 0.000000) glyphOverflow=(0.000000, 0.000000, 0.000000, 0.000000) glyphOverflow=(0.000000, 0.000000, 0.000000, 0.000000) glyphOverflow=(0.000000, 0.229492, 0.000000, 0.000000) glyphOverflow=(0.000000, 0.000000, 0.000000, 0.000000) glyphOverflow=(0.000000, 0.000000, 0.000000, 0.000000) glyphOverflow=(0.000000, 0.000000, 0.000000, 0.000000) glyphOverflow=(0.000000, 0.000000, 0.000000, 0.000000)
lgtm Thanks, this looks reasonable. I agree with your analysis about m_knownToHaveNoOverflowAndNoFallbackFonts. https://codereview.chromium.org/2683553002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/2683553002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutText.cpp:1391: DCHECK(!m_knownToHaveNoOverflowAndNoFallbackFonts || If you're changing this to DCHECK can you also change the other ASSERTs in this file? https://codereview.chromium.org/2683553002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutText.h (right): https://codereview.chromium.org/2683553002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutText.h:220: void setKnownToHaveNoOverflowAndNoFallbackFonts(bool known) { I'd write this as: void autosizingMultiplerChanged() { m_knownToHaveNoOverflowAndNoFallbackFonts = false; }
On 2017/02/16 02:48:37, skobes wrote: > lgtm > > Thanks, this looks reasonable. I agree with your analysis about > m_knownToHaveNoOverflowAndNoFallbackFonts. > Thanks for reviewing! After the change to autosize Ruby, the assert issue can be reproduced every time when I rotated the screen. I thought it was introduced by the implementation of Ruby. After looking into it, found these are two different issues... Should I part this patch into two?
https://codereview.chromium.org/2683553002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutText.cpp (right): https://codereview.chromium.org/2683553002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutText.cpp:1391: DCHECK(!m_knownToHaveNoOverflowAndNoFallbackFonts || On 2017/02/16 02:48:37, skobes wrote: > If you're changing this to DCHECK can you also change the other ASSERTs in this > file? I'd like to roll it back to ASSERT ;) https://codereview.chromium.org/2683553002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutText.h (right): https://codereview.chromium.org/2683553002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutText.h:220: void setKnownToHaveNoOverflowAndNoFallbackFonts(bool known) { On 2017/02/16 02:48:37, skobes wrote: > I'd write this as: > > void autosizingMultiplerChanged() { > m_knownToHaveNoOverflowAndNoFallbackFonts = false; > } Done.
On 2017/02/16 03:17:35, cathiechentx wrote: > After the change to autosize Ruby, the assert issue can be reproduced every time > when I rotated the screen. > I thought it was introduced by the implementation of Ruby. > After looking into it, found these are two different issues... > Should I part this patch into two? The change is small enough that I don't think it's important to split it up. Please wait for lgtm from pdr@ before landing.
On 2017/02/16 at 03:50:24, skobes wrote: > On 2017/02/16 03:17:35, cathiechentx wrote: > > After the change to autosize Ruby, the assert issue can be reproduced every time > > when I rotated the screen. > > I thought it was introduced by the implementation of Ruby. > > After looking into it, found these are two different issues... > > Should I part this patch into two? > > The change is small enough that I don't think it's important to split it up. > > Please wait for lgtm from pdr@ before landing. LGTM 2!
On 2017/02/16 04:10:21, pdr. wrote: > On 2017/02/16 at 03:50:24, skobes wrote: > > On 2017/02/16 03:17:35, cathiechentx wrote: > > > After the change to autosize Ruby, the assert issue can be reproduced every > time > > > when I rotated the screen. > > > I thought it was introduced by the implementation of Ruby. > > > After looking into it, found these are two different issues... > > > Should I part this patch into two? > > > > The change is small enough that I don't think it's important to split it up. > > > > Please wait for lgtm from pdr@ before landing. > > LGTM 2! Thanks;)
The CQ bit was checked by cathiechen@tencent.com
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
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by cathiechen@tencent.com
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": 160001, "attempt_start_ts": 1487226334902860,
"parent_rev": "3c9f07fc10a86242a0f38221ae73fd503dfcfd54", "commit_rev":
"082b073b26e5f408462e4fd1f229d7614ace4364"}
Message was sent while issue was closed.
Description was changed from ========== Fix not autosizing ruby elements issue. RubyRun is inline-block. When inflate ruby, skip rubyRun and inflate all rubyRun's inner blocks. BUG=688561 ========== to ========== Fix not autosizing ruby elements issue. RubyRun is inline-block. When inflate ruby, skip rubyRun and inflate all rubyRun's inner blocks. BUG=688561 Review-Url: https://codereview.chromium.org/2683553002 Cr-Commit-Position: refs/heads/master@{#450897} Committed: https://chromium.googlesource.com/chromium/src/+/082b073b26e5f408462e4fd1f229... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/082b073b26e5f408462e4fd1f229... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
