Re: Complete layout even if a block needs relayout due to widows or column balancing. (issue 2471623003 by mstensho@opera.com)

mste...@opera.com

unread,

Dec 1, 2016, 7:49:57 AM12/1/16

to sza...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

On 2016/11/02 22:50:03, mstensho wrote:
> I just realized that I should clean up the pagination code in this area before
> attempting to refactor it. I've landed some deep layout avoidance
optimizations
> for pagination recently. Among other things, we no longer need to store
whether
> page logical height changed or not in LayoutState. See
> https://codereview.chromium.org/2467353003/

@szager: I've now landed a few patches that clean up layoutBlock() and
layoutBlockFlow() a bit. There's more work to be done, but I'd like to land this
CL separately first (without any further refactoring), since the final
refactoring CL inevitably is going to fix something related to
PaintLayerScrollableArea::FreezeScrollbarsScope in combination with
fragmentation. And that deserves a CL (and a test) on its own, I should think.

https://codereview.chromium.org/2471623003/

sza...@chromium.org

unread,

Dec 1, 2016, 12:43:25 PM12/1/16

to mste...@opera.com, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

I love this!

lgtm with nit.

https://codereview.chromium.org/2471623003/diff/40001/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2471623003/diff/40001/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp#newcode518
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:518:
layoutBlockFlow(relayoutChildren, layoutScope);
Please add:

DCHECK(preferredLogicalWidthsWereDirty ||
!preferredLogicalWidthsDirty());

https://codereview.chromium.org/2471623003/

mste...@opera.com

unread,

Dec 1, 2016, 1:11:47 PM12/1/16

to sza...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

https://codereview.chromium.org/2471623003/diff/40001/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2471623003/diff/40001/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp#newcode518
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:518:
layoutBlockFlow(relayoutChildren, layoutScope);

On 2016/12/01 17:43:24, szager1 wrote:
> Please add:
>
> DCHECK(preferredLogicalWidthsWereDirty ||
!preferredLogicalWidthsDirty());

We can be pretty sure that !preferredLogicalWidthsWereDirty at this
point. So I suppose it's sufficient to
DCHECK(!preferredLogicalWidthsDirty()), right?

https://codereview.chromium.org/2471623003/

mste...@opera.com

unread,

Dec 1, 2016, 2:15:35 PM12/1/16

to sza...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

https://codereview.chromium.org/2471623003/diff/40001/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2471623003/diff/40001/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp#newcode518
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:518:
layoutBlockFlow(relayoutChildren, layoutScope);

On 2016/12/01 18:11:47, mstensho wrote:
> On 2016/12/01 17:43:24, szager1 wrote:
> > Please add:
> >
> > DCHECK(preferredLogicalWidthsWereDirty ||
!preferredLogicalWidthsDirty());
>
> We can be pretty sure that !preferredLogicalWidthsWereDirty at this
point. So I
> suppose it's sufficient to DCHECK(!preferredLogicalWidthsDirty()),
right?

Hmm... scrollbars/overflow-auto-infinite-loop.html crashes then. Maybe
deal with this separately?

https://codereview.chromium.org/2471623003/

sza...@chromium.org

unread,

Dec 1, 2016, 3:30:12 PM12/1/16

to mste...@opera.com, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

https://codereview.chromium.org/2471623003/diff/40001/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2471623003/diff/40001/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp#newcode518
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:518:
layoutBlockFlow(relayoutChildren, layoutScope);
On 2016/12/01 18:11:47, mstensho wrote:
> On 2016/12/01 17:43:24, szager1 wrote:
> > Please add:
> >
> > DCHECK(preferredLogicalWidthsWereDirty ||
!preferredLogicalWidthsDirty());
>
> We can be pretty sure that !preferredLogicalWidthsWereDirty at this
point. So I
> suppose it's sufficient to DCHECK(!preferredLogicalWidthsDirty()),
right?

There's a reason why I added preferredLogicalWidthsWereDirty, rather
than just checking preferredLogicalWidthsDirty() after
layoutBlockChildren. There are some legitimate cases where this code
runs while preferred widths are dirty (I don't remember exactly why).
So, we only want to flag the case where preferred widths were clean and
became dirty.

https://codereview.chromium.org/2471623003/

mste...@opera.com

unread,

Dec 1, 2016, 3:36:11 PM12/1/16

to sza...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

https://codereview.chromium.org/2471623003/diff/40001/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2471623003/diff/40001/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp#newcode518
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:518:
layoutBlockFlow(relayoutChildren, layoutScope);

On 2016/12/01 20:30:12, szager1 wrote:
> On 2016/12/01 18:11:47, mstensho wrote:
> > On 2016/12/01 17:43:24, szager1 wrote:
> > > Please add:
> > >
> > > DCHECK(preferredLogicalWidthsWereDirty ||
!preferredLogicalWidthsDirty());
> >
> > We can be pretty sure that !preferredLogicalWidthsWereDirty at this
point. So
> I
> > suppose it's sufficient to DCHECK(!preferredLogicalWidthsDirty()),
right?
>
> There's a reason why I added preferredLogicalWidthsWereDirty, rather
than just
> checking preferredLogicalWidthsDirty() after layoutBlockChildren.
There are
> some legitimate cases where this code runs while preferred widths are
dirty (I
> don't remember exactly why). So, we only want to flag the case where
preferred
> widths were clean and became dirty.

But we only get here if preferredLogicalWidthsBecameDirty, i.e. if
!preferredLogicalWidthsWereDirty && preferredLogicalWidthsDirty(). So I
don't understand how preferredLogicalWidthsWereDirty can be true here.

(preferred widths are dirty and remain dirty on any object that doesn't
need its min/max widths calculated, but maybe you had something more
specific in mind)

https://codereview.chromium.org/2471623003/

mste...@opera.com

unread,

Dec 2, 2016, 7:11:44 AM12/2/16

to sza...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

@szager Can I land this without your suggested addition? It fails in
scrollbars/overflow-auto-infinite-loop.html . This also happens if I add a
similar DCHECK to the code without this CL applied.

https://codereview.chromium.org/2471623003/

sza...@chromium.org

unread,

Dec 2, 2016, 7:03:04 PM12/2/16

to mste...@opera.com, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

Yeah, that's fine.

https://codereview.chromium.org/2471623003/

commit-bot@chromium.org via codereview.chromium.org

unread,

Dec 4, 2016, 3:54:40 AM12/4/16

to mste...@opera.com, sza...@chromium.org, commi...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2471623003/40001

https://codereview.chromium.org/2471623003/

commit-bot@chromium.org via codereview.chromium.org

unread,

Dec 4, 2016, 5:58:39 AM12/4/16

to mste...@opera.com, sza...@chromium.org, commi...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

Committed patchset #3 (id:40001)

https://codereview.chromium.org/2471623003/

commit-bot@chromium.org via codereview.chromium.org

unread,

Dec 4, 2016, 6:00:27 AM12/4/16

to mste...@opera.com, sza...@chromium.org, commi...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org

Patchset 3 (id:??) landed as
https://crrev.com/0740b0dfd20e2f99cddc773e0fe1460ef05da508
Cr-Commit-Position: refs/heads/master@{#436192}

https://codereview.chromium.org/2471623003/

Reply all

Reply to author

Forward