Complete layout even if a block needs relayout due to widows or column balancing.

Complete layout even if a block needs relayout due to widows or column balancing.

We cannot just abort in the middle of layoutBlockFlow() when we detect that we
need another layout pass (due to new column height or because we want an
earlier break to satisfy the widows requirement). We might miss our only
opportunity to detect size changes that way, and thus skip necessary layout and
repositioning of absolutely positioned descendants.

BUG=591637
Committed: https://crrev.com/0740b0dfd20e2f99cddc773e0fe1460ef05da508
Cr-Commit-Position: refs/heads/master@{#436192}

[mstensho (USE GERRIT), 2016-11-02 14:02:36]

The CQ bit was checked by mstensho@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 14:02:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 15:36:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 15:36:53]

Dry run: This issue passed the CQ dry run.

[mstensho (USE GERRIT), 2016-11-02 16:03:39]

mstensho@opera.com changed reviewers:
+ szager@chromium.org

[mstensho (USE GERRIT), 2016-11-02 16:03:40]

I tried to figure out if we should do something about the
preferredLogicalWidthsBecameDirty mechanism as well, but that one seems safe,
since it calls layoutBlockFlow() recursively, and, more crucially, it passes the
value of relayoutChildren (which will be set to true if size changes were
detected).

[szager1, 2016-11-02 21:35:41]

Would it work to move the height calculation and positioned object layout out of
layoutBlockFlow, and run that stuff only once after layoutBlockFlow has
succeeded?

[szager1, 2016-11-02 21:39:00]

On 2016/11/02 21:35:41, szager1 wrote:
> Would it work to move the height calculation and positioned object layout out
of
> layoutBlockFlow, and run that stuff only once after layoutBlockFlow has
> succeeded?

Something like:

do {
  layoutBlockFlow(...);

  if (column heights changed)
    continue;

  if (should break at line to avoid window)
    continue;

  break;
} while (true);

updateLogicalHeightAndLayoutPositionedObjectsAndComputeOverflow();

[mstensho (USE GERRIT), 2016-11-02 21:42:44]

On 2016/11/02 21:35:41, szager1 wrote:
> Would it work to move the height calculation and positioned object layout out
of
> layoutBlockFlow, and run that stuff only once after layoutBlockFlow has
> succeeded?

Only if layoutBlockFlow() has some means of informing the caller that size has
changed. I suppose it could take bool& relayoutChildren instead of bool
relayoutChildren. Or maybe just let it return a bool that tells whether size
changed or not.

The distribution of responsibilities between layoutBlock() and layoutBlockFlow()
is rather unclear to me.

[szager1, 2016-11-02 21:58:41]

On 2016/11/02 21:42:44, mstensho wrote:
> On 2016/11/02 21:35:41, szager1 wrote:
> > Would it work to move the height calculation and positioned object layout
out
> of
> > layoutBlockFlow, and run that stuff only once after layoutBlockFlow has
> > succeeded?
> 
> Only if layoutBlockFlow() has some means of informing the caller that size has
> changed. I suppose it could take bool& relayoutChildren instead of bool
> relayoutChildren. Or maybe just let it return a bool that tells whether size
> changed or not.
> 
> The distribution of responsibilities between layoutBlock() and
layoutBlockFlow()
> is rather unclear to me.

It seems like it should be possible to refactor so that logical height
computation, overhanging floats, and overflow computation are only ever done
once.  That could be in a different CL, but since you're already refactoring
stuff out of layoutBlockFlow and changing the loop logic, it seems reasonable to
do it here.

As for the separation of logic: *shrug*.

[mstensho (USE GERRIT), 2016-11-02 22:05:37]

On 2016/11/02 21:58:41, szager1 wrote:
> On 2016/11/02 21:42:44, mstensho wrote:
> > On 2016/11/02 21:35:41, szager1 wrote:
> > > Would it work to move the height calculation and positioned object layout
> out
> > of
> > > layoutBlockFlow, and run that stuff only once after layoutBlockFlow has
> > > succeeded?
> > 
> > Only if layoutBlockFlow() has some means of informing the caller that size
has
> > changed. I suppose it could take bool& relayoutChildren instead of bool
> > relayoutChildren. Or maybe just let it return a bool that tells whether size
> > changed or not.
> > 
> > The distribution of responsibilities between layoutBlock() and
> layoutBlockFlow()
> > is rather unclear to me.
> 
> It seems like it should be possible to refactor so that logical height
> computation, overhanging floats, and overflow computation are only ever done
> once.  That could be in a different CL, but since you're already refactoring
> stuff out of layoutBlockFlow and changing the loop logic, it seems reasonable
to
> do it here.

Agreed. Let me see what I can do.

> As for the separation of logic: *shrug*.

Ah - here: https://codereview.chromium.org/147233002

[mstensho (USE GERRIT), 2016-11-02 22:50:03]

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/

[mstensho (USE GERRIT), 2016-12-01 09:50:16]

The CQ bit was checked by mstensho@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 09:50:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 11:20:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 11:20:54]

Dry run: This issue passed the CQ dry run.

[mstensho (USE GERRIT), 2016-12-01 12:49:56]

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.

[szager1, 2016-12-01 17:43:24]

I love this!

lgtm with nit.

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

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

DCHECK(preferredLogicalWidthsWereDirty || !preferredLogicalWidthsDirty());

[mstensho (USE GERRIT), 2016-12-01 18:11:47]

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

https://codereview.chromium.org/2471623003/diff/40001/third_party/WebKit/Sour...
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?

[mstensho (USE GERRIT), 2016-12-01 19:15:34]

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

https://codereview.chromium.org/2471623003/diff/40001/third_party/WebKit/Sour...
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?

[szager1, 2016-12-01 20:30:12]

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

https://codereview.chromium.org/2471623003/diff/40001/third_party/WebKit/Sour...
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.

[mstensho (USE GERRIT), 2016-12-01 20:36:10]

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

https://codereview.chromium.org/2471623003/diff/40001/third_party/WebKit/Sour...
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)

[mstensho (USE GERRIT), 2016-12-02 12:11:43]

On 2016/12/01 20:36:10, mstensho wrote:
>
https://codereview.chromium.org/2471623003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):
> 
>
https://codereview.chromium.org/2471623003/diff/40001/third_party/WebKit/Sour...
> 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)

@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.

[szager1, 2016-12-03 00:03:03]

On 2016/12/02 12:11:43, mstensho wrote:
> On 2016/12/01 20:36:10, mstensho wrote:
> >
>
https://codereview.chromium.org/2471623003/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2471623003/diff/40001/third_party/WebKit/Sour...
> > 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)
> 
> @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.

Yeah, that's fine.

[mstensho (USE GERRIT), 2016-12-04 08:54:31]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2016-12-04 08:54:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-04 10:58:11]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480841671460850,
"parent_rev": "9884111c46b39236765e08aba101a2b12e561b33", "commit_rev":
"9412545d84d215c11ef78ba48a52e1fe5839a3ee"}

[commit-bot: I haz the power, 2016-12-04 10:58:39]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-04 11:00:25]

Description was changed from

==========
Complete layout even if a block needs relayout due to widows or column
balancing.

We cannot just abort in the middle of layoutBlockFlow() when we detect that we
need another layout pass (due to new column height or because we want an
earlier break to satisfy the widows requirement). We might miss our only
opportunity to detect size changes that way, and thus skip necessary layout and
repositioning of absolutely positioned descendants.

BUG=591637
==========

to

==========
Complete layout even if a block needs relayout due to widows or column
balancing.

We cannot just abort in the middle of layoutBlockFlow() when we detect that we
need another layout pass (due to new column height or because we want an
earlier break to satisfy the widows requirement). We might miss our only
opportunity to detect size changes that way, and thus skip necessary layout and
repositioning of absolutely positioned descendants.

BUG=591637

Committed: https://crrev.com/0740b0dfd20e2f99cddc773e0fe1460ef05da508
Cr-Commit-Position: refs/heads/master@{#436192}
==========

[commit-bot: I haz the power, 2016-12-04 11:00:27]

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