Description was changed from
==========
Remove anonymous block wrapper when all children become inline
BUG=532382
==========
to
==========
Remove anonymous block wrapper when all children become inline
BUG=523382
==========
rhogan
Description was changed from ========== Remove anonymous block wrapper when all children become inline BUG=523382 ...
Description was changed from
==========
Remove anonymous block wrapper when all children become inline
BUG=523382
==========
to
==========
Remove anonymous block wrapper when all children become inline
BUG=532382
==========
rhogan
Description was changed from ========== Remove anonymous block wrapper when all children become inline BUG=532382 ...
Description was changed from
==========
Remove anonymous block wrapper when all children become inline
BUG=532382
==========
to
==========
Remove anonymous block wrapper when all children become inline
BUG=523282
==========
rhogan
On 2015/10/22 at 18:24:09, mstensho wrote: > Can you CC me on the bug report, ...
On 2015/10/22 at 19:19:58, mstensho wrote:
>
https://codereview.chromium.org/1423573002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutBlock.cpp:647: if
(!isLayoutBlockFlow() || isRubyRun())
> Where does the ruby check come from? Do we have a test that would fail without
it?
Yes, this one prompted me to introduce this check:
LayoutTests/fast/ruby/float-overhang-from-ruby-text.html. Without the check we
can end up trying to fold the RubyBase part of the ruby run into the
LayoutRubyText above it.
Addressed the rest of your comments in the new CL.
rhogan
Description was changed from ========== Remove anonymous block wrapper when all children become inline BUG=523282 ...
Description was changed from
==========
Remove anonymous block wrapper when all children become inline
BUG=523282
==========
to
==========
Remove anonymous block wrapper when all children become inline
BUG=523282,546849
==========
mstensho (USE GERRIT)
I don't have access to bug 546849. Can you CC me? https://codereview.chromium.org/1423573002/diff/20001/third_party/WebKit/Source/core/layout/LayoutBlock.cpp File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): ...
I don't have access to bug 546849. Can you CC me?
https://codereview.chromium.org/1423573002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):
https://codereview.chromium.org/1423573002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:649: void
LayoutBlock::makeChildrenInlineIfPossible()
Before doing anything (object type checks and establishing the Vector), how
about first checking if we have children at all? I.e. move the |child|
definition a few lines up and bail early if it's nullptr.
https://codereview.chromium.org/1423573002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:651: if
(!isLayoutBlockFlow() || isRubyRun())
I'm wondering if we need more checks here. What about continuations and
LayoutRubyBase? I'm looking at canMergeContiguousAnonymousBlocks()... Not sure
if it's possible to end up with anonymous ruby base siblings or anything, but...
rhogan
On 2015/10/26 at 13:42:46, mstensho wrote: > I don't have access to bug 546849. Can ...
On 2015/10/26 at 13:42:46, mstensho wrote:
> I don't have access to bug 546849. Can you CC me?
>
>
https://codereview.chromium.org/1423573002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):
>
>
https://codereview.chromium.org/1423573002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBlock.cpp:649: void
LayoutBlock::makeChildrenInlineIfPossible()
> Before doing anything (object type checks and establishing the Vector), how
about first checking if we have children at all? I.e. move the |child|
definition a few lines up and bail early if it's nullptr.
>
>
https://codereview.chromium.org/1423573002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBlock.cpp:651: if
(!isLayoutBlockFlow() || isRubyRun())
> I'm wondering if we need more checks here. What about continuations and
LayoutRubyBase? I'm looking at canMergeContiguousAnonymousBlocks()... Not sure
if it's possible to end up with anonymous ruby base siblings or anything, but...
Your questions prompted me to change my approach with the new CL to reuse and
rename removeAnonymousWrappers(). Think this is better.
mstensho (USE GERRIT)
lgtm, looking very good! https://codereview.chromium.org/1423573002/diff/80001/third_party/WebKit/Source/core/layout/LayoutBlock.cpp File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1423573002/diff/80001/third_party/WebKit/Source/core/layout/LayoutBlock.cpp#newcode272 third_party/WebKit/Source/core/layout/LayoutBlock.cpp:272: toLayoutBlock(parent())->makeChildrenInlineIfPossible(); Note: I think this ...
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1423573002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1423573002/80001
Issue 1423573002: Remove anonymous block wrapper when all children become inline
(Closed)
Created 3 years ago by rhogan
Modified 3 years ago
Reviewers: mstensho (USE GERRIT)
Base URL: https://chromium.googlesource.com/chromium/src.git@master
Comments: 9