Only the first layout pass needs to go deep when pagination state changes.

Only the first layout pass needs to go deep when pagination state changes.

Since we now re-use the LayoutState object in multipass layout (caused by
either fragmentation or the PaintLayerScrollableArea::FreezeScrollbarsScope
mechanism), we need to notify the LayoutState object when we have performed the
necessary deep layout pass, so that not all subsequent passes also go deep
needlessly.

BUG=672088
Committed: https://crrev.com/aa1f4b1278d33e302c40a6879614ec2c418aaf33
Cr-Commit-Position: refs/heads/master@{#438353}

[mstensho (USE GERRIT), 2016-12-12 23:02:59]

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

[commit-bot: I haz the power, 2016-12-12 23:05:37]

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-13 01:19:40]

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

[commit-bot: I haz the power, 2016-12-13 01:19:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mstensho (USE GERRIT), 2016-12-13 07:45:10]

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

[mstensho (USE GERRIT), 2016-12-13 07:45:10]

Not very pretty, but it does what it says. :) Another option is of course to
just live with the performance regression and WontFix it.

[szager1, 2016-12-13 21:25:42]

I'm not entirely comfortable with modifying a LayoutState in that way (although
it's probably worth doing for the performance benefit).  Here's an alternative,
tell me what you think:

LayoutBlockFlow::layoutBlock(...) {
  ...
  // This line would be removed:
  // LayoutState state(*this, logicalWidthChanged);
  ...
  do {
    LayoutState state(*this, logicalWidthChanged);
    if (m_paginationStateChanged) {
      m_paginationStateChanged = false;
      state.setPaginationStateChanged();
    }

    layoutChildren(...);
    ...
  } while (true);

  LayoutState state(*this, logicalWidthChanged);
  ...
}

[mstensho (USE GERRIT), 2016-12-13 22:20:37]

On 2016/12/13 21:25:42, szager1 wrote:
> I'm not entirely comfortable with modifying a LayoutState in that way
(although
> it's probably worth doing for the performance benefit).  Here's an
alternative,
> tell me what you think:
> 
> LayoutBlockFlow::layoutBlock(...) {
>   ...
>   // This line would be removed:
>   // LayoutState state(*this, logicalWidthChanged);
>   ...
>   do {
>     LayoutState state(*this, logicalWidthChanged);
>     if (m_paginationStateChanged) {
>       m_paginationStateChanged = false;
>       state.setPaginationStateChanged();
>     }
> 
>     layoutChildren(...);
>     ...
>   } while (true);
> 
>   LayoutState state(*this, logicalWidthChanged);
>   ...
> }

Yes, I considered that alternative too (but I wasn't sure which one I preferred
- it kind of felt exotic to establish LayoutState at two locations).
Note that we also need setPaginationStateChanged() for the second LayoutState
too, though. I've uploaded a new patch set. Please take a look.

[szager1, 2016-12-13 22:28:47]

I like this one better.

lgtm

[mstensho (USE GERRIT), 2016-12-13 22:37:32]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2016-12-13 22:38:19]

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-14 00:17:41]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481668652287450,
"parent_rev": "ad5ea1f92522b9bd96180b41c9bf18452b4df94b", "commit_rev":
"be29bd963db8903a7725e390b3b17fb31dc178c6"}

[commit-bot: I haz the power, 2016-12-14 00:18:10]

Description was changed from

==========
Only the first layout pass needs to go deep when pagination state changes.

Since we now re-use the LayoutState object in multipass layout (caused by
either fragmentation or the PaintLayerScrollableArea::FreezeScrollbarsScope
mechanism), we need to notify the LayoutState object when we have performed the
necessary deep layout pass, so that not all subsequent passes also go deep
needlessly.

BUG=672088
==========

to

==========
Only the first layout pass needs to go deep when pagination state changes.

Since we now re-use the LayoutState object in multipass layout (caused by
either fragmentation or the PaintLayerScrollableArea::FreezeScrollbarsScope
mechanism), we need to notify the LayoutState object when we have performed the
necessary deep layout pass, so that not all subsequent passes also go deep
needlessly.

BUG=672088

Review-Url: https://codereview.chromium.org/2570643002
==========

[commit-bot: I haz the power, 2016-12-14 00:18:11]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-14 00:21:02]

Description was changed from

==========
Only the first layout pass needs to go deep when pagination state changes.

Since we now re-use the LayoutState object in multipass layout (caused by
either fragmentation or the PaintLayerScrollableArea::FreezeScrollbarsScope
mechanism), we need to notify the LayoutState object when we have performed the
necessary deep layout pass, so that not all subsequent passes also go deep
needlessly.

BUG=672088

Review-Url: https://codereview.chromium.org/2570643002
==========

to

==========
Only the first layout pass needs to go deep when pagination state changes.

Since we now re-use the LayoutState object in multipass layout (caused by
either fragmentation or the PaintLayerScrollableArea::FreezeScrollbarsScope
mechanism), we need to notify the LayoutState object when we have performed the
necessary deep layout pass, so that not all subsequent passes also go deep
needlessly.

BUG=672088

Committed: https://crrev.com/aa1f4b1278d33e302c40a6879614ec2c418aaf33
Cr-Commit-Position: refs/heads/master@{#438353}
==========

[commit-bot: I haz the power, 2016-12-14 00:21:03]

Patchset 2 (id:??) landed as
https://crrev.com/aa1f4b1278d33e302c40a6879614ec2c418aaf33
Cr-Commit-Position: refs/heads/master@{#438353}