Avoid clearing a page's layout overflow after paginated layout

Avoid clearing a page's layout overflow after paginated layout

This was flushed out by https://codereview.chromium.org/2089373002.

If we've forced a layout on paged media and find that our width exceeds
the page's then we may end up needing to clip some of the horizontal overflow.
We do this by calculating it manually and storing it as layout overflow.
However if our scrollbars are changing we will force another layout on the
frame that overwrites this good work. So make sure we preserve any clipped
overflow on the frame.

It's very hard to reproduce print preview issues reliably but fortunately
this specific issue can be recreated during layout testing by using
@media page rules so that's how we reproduce it here. I was going to revert
the CL in 2089373002 but it's very hard to prove either way that that is
achieving anything - this change on the other hand has an intelligible 
rationale and does ensure the expected clipping of width.


Related bugs potentially fixed by this are 660058 and 690621.

BUG=664235
Review-Url: https://codereview.chromium.org/2747623002
Cr-Commit-Position: refs/heads/master@{#460292}
Committed: https://chromium.googlesource.com/chromium/src/+/593eebedc4013ea212b05c21dba300d41a6d7977

[rhogan, 2017-03-12 15:19:27]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-12 15:19:36]

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

[rhogan, 2017-03-12 15:22:38]

Description was changed from

==========
Revert to sizing page before printing
BUG=664235
==========

to

==========
Revert to sizing page before printing

This is a revert of https://codereview.chromium.org/2089373002.

I've added a test or two to catch the regressions, and I'm leaving
the tests it added in place. I can't reproduce the issues noted in
crbug.com/567317 with CL 2089373002 reverted, so it looks like the
order in which we called ResizeForPrinting() wasn't the real cause.

BUG=664235, 660058, 567317
==========

[commit-bot: I haz the power, 2017-03-12 15:58:54]

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

[commit-bot: I haz the power, 2017-03-12 15:58:55]

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

[rhogan, 2017-03-13 21:12:30]

robhogan@gmail.com changed reviewers:
+ esprehn@chromium.org, nainar@chromium.org, thestig@chromium.org

[rhogan, 2017-03-13 21:12:31]

Hi guys - haven't managed to get a working test going yet, but what do you think
about moving ResizeForPrinting() back to where it was. Testing manually, I can
recreate any of the issues from the CL's original bug - so I think we can move
it bug and squash the regressions it created.

[Lei Zhang, 2017-03-15 01:48:38]

I defer to nainar and will stamp whatever solution y'all agree on.

[nainar, 2017-03-15 06:28:41]

lgtm

[esprehn, 2017-03-15 06:34:20]

Can you explain in the description why this fixes all those linked bugs?

[rhogan, 2017-03-23 11:45:18]

On 2017/03/15 at 06:34:20, esprehn wrote:
> Can you explain in the description why this fixes all those linked bugs?

I set out to fix 690621 and discovered these other bugs were related and traced
it back to 567317 as the source of the regression.

Now I can fix 690621 and 664235 in forceLayoutForPagination() - it sets layout
overflow that inadvertently gets cleared again by adjustViewSizeAndLayout(). But
when I revert to ResizeForPrinting() before printBegin() I can't recreate any of
the issues in 567317 so it seems like it isn't necessary.

I'm happy to fix this in forceLayoutForPagination() - I'm not clear why it was
correct in principle to move ResizeForPrinting() and I can't find a test that
demonstrates the need to do it.

[esprehn, 2017-03-24 01:57:49]

On 2017/03/23 at 11:45:18, robhogan wrote:
> On 2017/03/15 at 06:34:20, esprehn wrote:
> > Can you explain in the description why this fixes all those linked bugs?
> 
> I set out to fix 690621 and discovered these other bugs were related and
traced it back to 567317 as the source of the regression.
> 
> Now I can fix 690621 and 664235 in forceLayoutForPagination() - it sets layout
overflow that inadvertently gets cleared again by adjustViewSizeAndLayout(). But
when I revert to ResizeForPrinting() before printBegin() I can't recreate any of
the issues in 567317 so it seems like it isn't necessary.
> 
> I'm happy to fix this in forceLayoutForPagination() - I'm not clear why it was
correct in principle to move ResizeForPrinting() and I can't find a test that
demonstrates the need to do it.

Okay sgtm.

[rhogan, 2017-03-24 20:59:54]

On 2017/03/24 at 01:57:49, esprehn wrote:
> Okay sgtm.

Fix a subset of the regressions by patching forceLayoutForPagination()?

[esprehn, 2017-03-24 21:56:24]

On 2017/03/24 at 20:59:54, robhogan wrote:
> On 2017/03/24 at 01:57:49, esprehn wrote:
> > Okay sgtm.
> 
> Fix a subset of the regressions by patching forceLayoutForPagination()?

I meant this patch, though I see you've got a bunch of red bots.

[rhogan, 2017-03-27 20:57:19]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 20:57:52]

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, 2017-03-27 21:47:41]

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

[commit-bot: I haz the power, 2017-03-27 21:47:42]

Dry run: This issue passed the CQ dry run.

[rhogan, 2017-03-28 18:07:41]

Description was changed from

==========
Revert to sizing page before printing

This is a revert of https://codereview.chromium.org/2089373002.

I've added a test or two to catch the regressions, and I'm leaving
the tests it added in place. I can't reproduce the issues noted in
crbug.com/567317 with CL 2089373002 reverted, so it looks like the
order in which we called ResizeForPrinting() wasn't the real cause.

BUG=664235, 660058, 567317
==========

to

==========
Avoid clearing a page's layout overflow after paginated layout

This was flushed out by https://codereview.chromium.org/2089373002.

If we've forced a layout on paged media and find that our width exceeds
the page's then we may end up needing to clip some of the horizontal overflow.
We do this by calculating it manually and storing it as layout overflow.
However if our scrollbars are changing we will force another layout on the
frame that overwrites this good work. So make sure we preserve any clipped
overflow on the frame.

It's very hard to reproduce print preview issues reliably but fortunately
this specific issue can be recreated during layout testing by using
@media page rules so that's how we reproduce it here. I was going to revert
the CL in 2089373002 but it's very hard to prove either way that that is
achieving anything - this change on the other hand has an intelligible 
rationale and does ensure the expected clipping of width.


Related bugs potentially fixed by this are 660058 and 690621.

BUG=664235
==========

[rhogan, 2017-03-28 18:07:47]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 18:08:11]

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

[rhogan, 2017-03-28 18:09:23]

On 2017/03/24 at 21:56:24, esprehn wrote:
> On 2017/03/24 at 20:59:54, robhogan wrote:
> > On 2017/03/24 at 01:57:49, esprehn wrote:
> > > Okay sgtm.
> > 
> > Fix a subset of the regressions by patching forceLayoutForPagination()?
> 
> I meant this patch, though I see you've got a bunch of red bots.

Decided to go with the forceLayoutForPagination() route as it makes sense to me
and I can test for it. I'm unable to come up with a way of layout or unit
testing the correct position of ResizeForPrinting() so don't have much
confidence that it's right to move it anymore.

[commit-bot: I haz the power, 2017-03-28 19:38:10]

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

[commit-bot: I haz the power, 2017-03-28 19:38:11]

Dry run: This issue passed the CQ dry run.

[esprehn, 2017-03-28 21:34:27]

lgtm

[rhogan, 2017-03-29 05:39:52]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2017-03-29 05:39:52]

The patchset sent to the CQ was uploaded after l-g-t-m from nainar@chromium.org
Link to the patchset: https://codereview.chromium.org/2747623002/#ps40001
(title: "bug 664235")

[commit-bot: I haz the power, 2017-03-29 05:40: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, 2017-03-29 05:45:28]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490765992182960,
"parent_rev": "82460c48b74229f08ddb8b19982a6a5076787466", "commit_rev":
"593eebedc4013ea212b05c21dba300d41a6d7977"}

[commit-bot: I haz the power, 2017-03-29 05:46:10]

Description was changed from

==========
Avoid clearing a page's layout overflow after paginated layout

This was flushed out by https://codereview.chromium.org/2089373002.

If we've forced a layout on paged media and find that our width exceeds
the page's then we may end up needing to clip some of the horizontal overflow.
We do this by calculating it manually and storing it as layout overflow.
However if our scrollbars are changing we will force another layout on the
frame that overwrites this good work. So make sure we preserve any clipped
overflow on the frame.

It's very hard to reproduce print preview issues reliably but fortunately
this specific issue can be recreated during layout testing by using
@media page rules so that's how we reproduce it here. I was going to revert
the CL in 2089373002 but it's very hard to prove either way that that is
achieving anything - this change on the other hand has an intelligible 
rationale and does ensure the expected clipping of width.


Related bugs potentially fixed by this are 660058 and 690621.

BUG=664235
==========

to

==========
Avoid clearing a page's layout overflow after paginated layout

This was flushed out by https://codereview.chromium.org/2089373002.

If we've forced a layout on paged media and find that our width exceeds
the page's then we may end up needing to clip some of the horizontal overflow.
We do this by calculating it manually and storing it as layout overflow.
However if our scrollbars are changing we will force another layout on the
frame that overwrites this good work. So make sure we preserve any clipped
overflow on the frame.

It's very hard to reproduce print preview issues reliably but fortunately
this specific issue can be recreated during layout testing by using
@media page rules so that's how we reproduce it here. I was going to revert
the CL in 2089373002 but it's very hard to prove either way that that is
achieving anything - this change on the other hand has an intelligible 
rationale and does ensure the expected clipping of width.


Related bugs potentially fixed by this are 660058 and 690621.

BUG=664235

Review-Url: https://codereview.chromium.org/2747623002
Cr-Commit-Position: refs/heads/master@{#460292}
Committed:
https://chromium.googlesource.com/chromium/src/+/593eebedc4013ea212b05c21dba3...
==========

[commit-bot: I haz the power, 2017-03-29 05:46:11]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/593eebedc4013ea212b05c21dba3...