[SPInvalidation] Fix fixed-position printing

[SPInvalidation] Fix fixed-position printing

Previously the fragments for fixed-position in non-transformed path
didn't have correct layerBounds and foregroundRect, causing nothing
painted.

Change collectPaintFragmentsForPaginatedFixedPosition() to
repeatFixedPositionObjectInPages(), and let the single-fragment
path generate the first fragment (which has appropriate layerBounds
and foregroundRect), and the new function repeats the fragment for
pages with adjusted paginationOffset and layerBounds.

BUG=646176
TEST=printing/fixed*.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/574be21bec113164af880b853f9f25209183ae90
Cr-Commit-Position: refs/heads/master@{#441235}

[Xianzhu, 2016-12-29 20:17:20]

Description was changed from

==========
[SPInvalidation] Fix fixed-position printing

Previously the fragments for fixed-position in non-transformed path
didn't have correct layerBounds and foregroundRect, causing nothing
painted.

Change collectPaintFragmentsForPaginatedFixedPosition() to
repeatFixedPositionObjectInPages(), and let the single-fragment
path generate the first fragment (which has appropriate layerBounds
and foregroundRect), and the new function repeats the fragment for
pages with adjusted paginationOffset and layerBounds.

BUG=646176
TEST=printing/fixed*.html
==========

to

==========
[SPInvalidation] Fix fixed-position printing

Previously the fragments for fixed-position in non-transformed path
didn't have correct layerBounds and foregroundRect, causing nothing
painted.

Change collectPaintFragmentsForPaginatedFixedPosition() to
repeatFixedPositionObjectInPages(), and let the single-fragment
path generate the first fragment (which has appropriate layerBounds
and foregroundRect), and the new function repeats the fragment for
pages with adjusted paginationOffset and layerBounds.

BUG=646176
TEST=printing/fixed*.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2016-12-29 20:17:22]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 20:17:37]

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

[Xianzhu, 2016-12-29 20:19:37]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org

[commit-bot: I haz the power, 2016-12-29 21:51:04]

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

[commit-bot: I haz the power, 2016-12-29 21:51:04]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Xianzhu, 2016-12-29 22:50:11]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 22:50:23]

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

[Xianzhu, 2016-12-29 22:55:12]

Uploaded Patch Set 2 with a small change fixing
virtual/spinvalidation/printing/fixed-positioned-scrolled.html failure of Patch
Set 1.

[chrishtr, 2016-12-30 00:10:50]

https://codereview.chromium.org/2605333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/2605333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:430:
repeatFixedPositionObjectInPages(paintingInfo, layerFragments);
It's only possible in single-fragment mode?

https://codereview.chromium.org/2605333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:628:
fragment.layerBounds.moveBy(pageOffset);
This didn't used to get set in the transformed path. Was it a bug there also?

[Xianzhu, 2016-12-30 00:52:41]

https://codereview.chromium.org/2605333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/2605333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:430:
repeatFixedPositionObjectInPages(paintingInfo, layerFragments);
On 2016/12/30 00:10:50, chrishtr wrote:
> It's only possible in single-fragment mode?

Now isFixedPositionObjectInPageMedia and single-fragment mode share the same
code between line 424 and 428. For isFixedPositionObjectInPageMedia, the shared
code generates the first fragment for the fixed-position object, then the above
function call repeats and adjusts the fragment for each page.

In paintLayerWithTransform path, this function is called with
ForceSingleFragment from the fragment iteration loop in paintLayerWithTransform,
and isFixedPositionObjectInPagedMedia variable is set to false, and we won't
generate fragments again.

https://codereview.chromium.org/2605333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:628:
fragment.layerBounds.moveBy(pageOffset);
On 2016/12/30 00:10:50, chrishtr wrote:
> This didn't used to get set in the transformed path. Was it a bug there also?

No. layerBounds and foregroundRect of these fragments are not used in
transformed path. This is why we only have had problem since we switched to
non-transformed path. Foreground clipping is applied in single-fragment mode
down the path later.

[commit-bot: I haz the power, 2016-12-30 01:25:31]

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

[commit-bot: I haz the power, 2016-12-30 01:25:32]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2017-01-03 18:25:09]

https://codereview.chromium.org/2605333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/2605333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:626:
PaintLayerFragment fragment = layerFragments[i - 1];
What guarantees that |layerFragments| has exactly the same length
as |pages|? The previous code didn't have this dependency...

https://codereview.chromium.org/2605333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:628:
fragment.layerBounds.moveBy(pageOffset);
On 2016/12/30 at 00:52:41, Xianzhu wrote:
> On 2016/12/30 00:10:50, chrishtr wrote:
> > This didn't used to get set in the transformed path. Was it a bug there
also?
> 
> No. layerBounds and foregroundRect of these fragments are not used in
transformed path. This is why we only have had problem since we switched to
non-transformed path. Foreground clipping is applied in single-fragment mode
down the path later.

I'm not sure what you mean. layerBounds is used in
atLeastOneFragmentIntersectsDamageRect and paintFragmentWithPhase.
Don't those get called in the transformed path?

[Xianzhu, 2017-01-03 18:34:38]

On 2017/01/03 18:25:09, chrishtr wrote:
>
https://codereview.chromium.org/2605333002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):
> 
>
https://codereview.chromium.org/2605333002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:626:
> PaintLayerFragment fragment = layerFragments[i - 1];
> What guarantees that |layerFragments| has exactly the same length
> as |pages|? The previous code didn't have this dependency...
> 
>
https://codereview.chromium.org/2605333002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:628:
> fragment.layerBounds.moveBy(pageOffset);
> On 2016/12/30 at 00:52:41, Xianzhu wrote:
> > On 2016/12/30 00:10:50, chrishtr wrote:
> > > This didn't used to get set in the transformed path. Was it a bug there
> also?
> > 
> > No. layerBounds and foregroundRect of these fragments are not used in
> transformed path. This is why we only have had problem since we switched to
> non-transformed path. Foreground clipping is applied in single-fragment mode
> down the path later.
> 
> I'm not sure what you mean. layerBounds is used in
> atLeastOneFragmentIntersectsDamageRect and paintFragmentWithPhase.
> Don't those get called in the transformed path?

The call stack of transformed path is like the following:
  paint() {
    paintLayerWithTransform() {
      fragments = collectFragments();
      for (fragment: fragments) {
        paintFragmentByApplyingTransform() {
          paintLayerContentsCompositingAllPhases(*ForceSingleFragment*) {
            paintLayerContents(*ForceSingleFragment*) {
              fragments = singleFragment(); // Because of ForceSingleFragment.
              if (atLeastOneFragmentIntersectsDamageRect(fragments)) {
                for (fragment: fragments)
                  paintFragmentWithPhase();
              }
            }
          }
        }
      }
    }
  }

layerBounds and foreground of the fragments in the outer loop are not used. We
use layerBounds of the single fragment in the inner loop.

[chrishtr, 2017-01-03 18:53:10]

https://codereview.chromium.org/2605333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/2605333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:626:
PaintLayerFragment fragment = layerFragments[i - 1];
On 2017/01/03 at 18:25:09, chrishtr wrote:
> What guarantees that |layerFragments| has exactly the same length
> as |pages|? The previous code didn't have this dependency...

oh I see now, nm.

https://codereview.chromium.org/2605333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:628:
fragment.layerBounds.moveBy(pageOffset);
On 2017/01/03 at 18:25:09, chrishtr wrote:
> On 2016/12/30 at 00:52:41, Xianzhu wrote:
> > On 2016/12/30 00:10:50, chrishtr wrote:
> > > This didn't used to get set in the transformed path. Was it a bug there
also?
> > 
> > No. layerBounds and foregroundRect of these fragments are not used in
transformed path. This is why we only have had problem since we switched to
non-transformed path. Foreground clipping is applied in single-fragment mode
down the path later.
> 
> I'm not sure what you mean. layerBounds is used in
atLeastOneFragmentIntersectsDamageRect and paintFragmentWithPhase.
> Don't those get called in the transformed path?

Right ok, now I see.

https://codereview.chromium.org/2605333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.h (right):

https://codereview.chromium.org/2605333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.h:60: void
repeatFixedPositionObjectInPages(const PaintLayerPaintingInfo&,
The input for repeatFixedPositionObjectInPages must be a single
fragment that is then repeated. How about changing the signature
to enforce that? e.g. input is PaintLayerFragment and output is
PaintLayerFragments.

Also, this fixed-pos code seems pretty hacky (not your fault, it
was like that before). It would be good to fold it into
collectFragments somehow, add a TODO?

[Xianzhu, 2017-01-03 19:54:42]

https://codereview.chromium.org/2605333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.h (right):

https://codereview.chromium.org/2605333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.h:60: void
repeatFixedPositionObjectInPages(const PaintLayerPaintingInfo&,
On 2017/01/03 18:53:10, chrishtr wrote:
> The input for repeatFixedPositionObjectInPages must be a single
> fragment that is then repeated. How about changing the signature
> to enforce that? e.g. input is PaintLayerFragment and output is
> PaintLayerFragments.
> 

Done.

> Also, this fixed-pos code seems pretty hacky (not your fault, it
> was like that before). It would be good to fold it into
> collectFragments somehow, add a TODO?

Done.

Actually I reviewed the original change adding the fixed-position hack. For now
the hack (instead of folded into collectFragment) it's still needed because of
the specialty of the transformed path.

[Xianzhu, 2017-01-03 19:55:09]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 19:55:29]

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

[chrishtr, 2017-01-03 20:13:38]

lgtm

https://codereview.chromium.org/2605333002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.h (right):

https://codereview.chromium.org/2605333002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.h:60: void
repeatFixedPositionObjectInPages(const PaintLayerFragment&,
Add a brief comment explaining the parameters and what happens.

[Xianzhu, 2017-01-03 20:19:20]

https://codereview.chromium.org/2605333002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.h (right):

https://codereview.chromium.org/2605333002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.h:60: void
repeatFixedPositionObjectInPages(const PaintLayerFragment&,
On 2017/01/03 20:13:38, chrishtr wrote:
> Add a brief comment explaining the parameters and what happens.

Done.

[Xianzhu, 2017-01-03 20:19:33]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-01-03 20:19:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2605333002/#ps60001
(title: "-")

[commit-bot: I haz the power, 2017-01-03 20:19:41]

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-01-03 22:57:04]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483474773563190,
"parent_rev": "1c0643e6e9c9654f153c0c4c5c0d37d4d71b50b4", "commit_rev":
"ceab63c1177fd1565191169a601876b1d630e7d8"}

[commit-bot: I haz the power, 2017-01-03 22:57:37]

Description was changed from

==========
[SPInvalidation] Fix fixed-position printing

Previously the fragments for fixed-position in non-transformed path
didn't have correct layerBounds and foregroundRect, causing nothing
painted.

Change collectPaintFragmentsForPaginatedFixedPosition() to
repeatFixedPositionObjectInPages(), and let the single-fragment
path generate the first fragment (which has appropriate layerBounds
and foregroundRect), and the new function repeats the fragment for
pages with adjusted paginationOffset and layerBounds.

BUG=646176
TEST=printing/fixed*.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[SPInvalidation] Fix fixed-position printing

Previously the fragments for fixed-position in non-transformed path
didn't have correct layerBounds and foregroundRect, causing nothing
painted.

Change collectPaintFragmentsForPaginatedFixedPosition() to
repeatFixedPositionObjectInPages(), and let the single-fragment
path generate the first fragment (which has appropriate layerBounds
and foregroundRect), and the new function repeats the fragment for
pages with adjusted paginationOffset and layerBounds.

BUG=646176
TEST=printing/fixed*.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[commit-bot: I haz the power, 2017-01-03 22:57:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2017-01-03 23:02:28]

Description was changed from

==========
[SPInvalidation] Fix fixed-position printing

Previously the fragments for fixed-position in non-transformed path
didn't have correct layerBounds and foregroundRect, causing nothing
painted.

Change collectPaintFragmentsForPaginatedFixedPosition() to
repeatFixedPositionObjectInPages(), and let the single-fragment
path generate the first fragment (which has appropriate layerBounds
and foregroundRect), and the new function repeats the fragment for
pages with adjusted paginationOffset and layerBounds.

BUG=646176
TEST=printing/fixed*.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

to

==========
[SPInvalidation] Fix fixed-position printing

Previously the fragments for fixed-position in non-transformed path
didn't have correct layerBounds and foregroundRect, causing nothing
painted.

Change collectPaintFragmentsForPaginatedFixedPosition() to
repeatFixedPositionObjectInPages(), and let the single-fragment
path generate the first fragment (which has appropriate layerBounds
and foregroundRect), and the new function repeats the fragment for
pages with adjusted paginationOffset and layerBounds.

BUG=646176
TEST=printing/fixed*.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/574be21bec113164af880b853f9f25209183ae90
Cr-Commit-Position: refs/heads/master@{#441235}
==========

[commit-bot: I haz the power, 2017-01-03 23:02:29]

Patchset 4 (id:??) landed as
https://crrev.com/574be21bec113164af880b853f9f25209183ae90
Cr-Commit-Position: refs/heads/master@{#441235}