Disable scroll anchoring when in printing mode

Disable scroll anchoring when in printing mode

BUG=651020
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/32628627dac02e25eb29924fa9b39b399d694547
Cr-Commit-Position: refs/heads/master@{#425756}

[ymalik, 2016-10-12 22:17:35]

Description was changed from

==========
Disable scroll anchoring when in printing mode

BUG=651020
==========

to

==========
Disable scroll anchoring when in printing mode

BUG=651020
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[ymalik, 2016-10-12 22:20:51]

Description was changed from

==========
Disable scroll anchoring when in printing mode

BUG=651020
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Disable scroll anchoring when in printing mode

BUG=651020
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[ymalik, 2016-10-12 22:20:51]

ymalik@chromium.org changed reviewers:
+ skobes@chromium.org

[ymalik, 2016-10-12 22:21:01]

https://codereview.chromium.org/2417683002/diff/1/components/printing/rendere...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/2417683002/diff/1/components/printing/rendere...
components/printing/renderer/print_web_view_helper.cc:810: frame->printEnd();
This is needed because RestoreSize would trigger layout which would trigger
scroll anchoring. Moving the call to printEnd ensures that anchoring remains
disabled from RestoreSize.

Also, when we do printStart then resize, so it makes sense to restore size then
printEnd.

[skobes, 2016-10-12 22:40:31]

https://codereview.chromium.org/2417683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2417683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.h:1069: // from blink::Document
because it's reset before the set of layouts caused
This seems wrong.  Isn't Document::m_printing cleared by PrintContext::end() at
the same time you're clearing this new bit?

[ymalik, 2016-10-13 13:48:04]

https://codereview.chromium.org/2417683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2417683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.h:1069: // from blink::Document
because it's reset before the set of layouts caused
On 2016/10/12 22:40:31, skobes wrote:
> Isn't Document::m_printing cleared by PrintContext::end() at
> the same time you're clearing this new bit?

Not quite. Document::m_printing is cleared from LocalFrame::setPrinting, before
the layout that trigger unnecessary adjustments. 

See
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Loc...

Another potential fix is clearing the bit at the end of the
LocalFrame::setPrinting function, but I am afraid it has a lot of users and its
not clear to me whether the order matters.

[skobes, 2016-10-13 16:24:00]

https://codereview.chromium.org/2417683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2417683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.h:1069: // from blink::Document
because it's reset before the set of layouts caused
On 2016/10/13 13:48:04, ymalik wrote:
> Another potential fix is clearing the bit at the end of the
> LocalFrame::setPrinting function, but I am afraid it has a lot of users and
its
> not clear to me whether the order matters.

Ah, I suspect LocalFrame::setPrinting does a layout after clearing m_printing in
order to reset the layout tree to its original state, and the anchor node moves
during this layout.

I'd rather turn Document::m_printing into an enum {NotPrinting, Printing,
ExitingPrinting} than add a new bit on FrameView for this.  But I also see we
have Document::wasPrinting() added to suppress CSS transitions in
http://crbug.com/412281.  Is it feasible to use that?

[ymalik, 2016-10-14 16:30:56]

@skobes, PTAL :)

https://codereview.chromium.org/2417683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2417683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.h:1069: // from blink::Document
because it's reset before the set of layouts caused
On 2016/10/13 16:24:00, skobes wrote:
> On 2016/10/13 13:48:04, ymalik wrote:
> > Another potential fix is clearing the bit at the end of the
> > LocalFrame::setPrinting function, but I am afraid it has a lot of users and
> its
> > not clear to me whether the order matters.
> 
> Ah, I suspect LocalFrame::setPrinting does a layout after clearing m_printing
in
> order to reset the layout tree to its original state, and the anchor node
moves
> during this layout.
> 
> I'd rather turn Document::m_printing into an enum {NotPrinting, Printing,
> ExitingPrinting} than add a new bit on FrameView for this.  But I also see we
> have Document::wasPrinting() added to suppress CSS transitions in
> http://crbug.com/412281.  Is it feasible to use that?

Yes an enum is cleaner in this case. It also allows us to remove
Document::wasPrinting()

Document::wasPrinting is used to disable css transitions in PrintMode. Basically
the call stack is as follows:
PrintContext::Begin
Update CSS Animation (m_printing = 1, m_wasPrinting = 0) - Don't update
transition
Document::UpdateState (m_wasPrinting = 1)
layout for printing mode
...
PrintContext::End
Update CSS Animation (m_printing = 0, m_wasPrinting = 1) - Don't update
transition
Document::UpdateState (m_wasPrinting = 0)
layout to restore original state

We couldn't use m_wasPrinting in the case of scroll anchoring because it was
reset before layout. Using this enum, we can cover both the transition case and
scroll anchoring case. I verified locally that this works.

[skobes, 2016-10-14 16:36:24]

Thanks, this looks much better.  LGTM w/ nit

https://codereview.chromium.org/2417683002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2417683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:631: enum PrintingState {
NotPrinting, Printing, FinishingPrinting };
Add a comment that explains what FinishingPrinting means (as I understand it,
something like "restoring the non-printing layout state").

[ymalik, 2016-10-14 16:51:04]

ymalik@chromium.org changed reviewers:
+ skobes thestig@chromium.org
- skobes@chromium.org

[ymalik, 2016-10-14 16:51:04]

+thestig for print_web_view_helper.cc

[ymalik, 2016-10-14 16:51:23]

ymalik@chromium.org changed reviewers:
+ skobes@chromium.org, thestig@chromium.org
- skobes thestig@chromium.org

[Lei Zhang, 2016-10-14 18:17:29]

thestig@chromium.org changed reviewers:
+ nainar@chromium.org

[Lei Zhang, 2016-10-14 18:17:30]

+nainar who looked at some elements of Blink + printing recently.

[nainar, 2016-10-17 16:26:04]

the third_party/WebKit changes lgtm. 

the change to print_web_view_helper.cc lgtm too, only ever been in that file
once though.

[Lei Zhang, 2016-10-17 17:32:58]

lgtm

[ymalik, 2016-10-17 18:12:48]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-10-17 18:12:49]

The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/2417683002/#ps40001
(title: "Add comment")

[commit-bot: I haz the power, 2016-10-17 18:13:03]

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-10-17 19:51:20]

Description was changed from

==========
Disable scroll anchoring when in printing mode

BUG=651020
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Disable scroll anchoring when in printing mode

BUG=651020
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-10-17 19:51:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-17 19:54:50]

Description was changed from

==========
Disable scroll anchoring when in printing mode

BUG=651020
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Disable scroll anchoring when in printing mode

BUG=651020
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/32628627dac02e25eb29924fa9b39b399d694547
Cr-Commit-Position: refs/heads/master@{#425756}
==========

[commit-bot: I haz the power, 2016-10-17 19:54:51]

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