Fix gradient background invalidation when HTML size changes

Fix viewport background invalidation when HTML size changes

Non-fixed-attachment viewport background uses root element as the
background positioning area. If HTML size changes, and the background
needs paint invalidation on the resize, we should invalidate the
LayoutView.

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

Review-Url: https://codereview.chromium.org/2699463004
Cr-Commit-Position: refs/heads/master@{#451265}
Committed: https://chromium.googlesource.com/chromium/src/+/d34bb14bf30e4885f704336b0658cd32ba01e108

[Xianzhu, 2017-02-16 00:52:59]

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

[commit-bot: I haz the power, 2017-02-16 00:54:02]

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-02-16 02:51:44]

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

[commit-bot: I haz the power, 2017-02-16 02:51:45]

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

[Xianzhu, 2017-02-16 06:43:06]

Description was changed from

==========
Fix gradient background invalidation when HTML size changes

BUG=688596
==========

to

==========
Fix gradient background invalidation when HTML size changes

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

[Xianzhu, 2017-02-16 06:43:10]

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

[commit-bot: I haz the power, 2017-02-16 06:44:05]

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-02-16 06:46:43]

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

[commit-bot: I haz the power, 2017-02-16 06:46:44]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Xianzhu, 2017-02-16 07:00:51]

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

[commit-bot: I haz the power, 2017-02-16 07:01:25]

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-02-16 08:45:01]

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

[commit-bot: I haz the power, 2017-02-16 08:45:02]

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

[Xianzhu, 2017-02-16 17:08:53]

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

[Xianzhu, 2017-02-16 17:08:54]

Will rebaseline the failing layout tests by removing the extra HTML paint
invalidations.

[chrishtr, 2017-02-16 18:39:37]

Why can't this case be handled with something like the already-existing code in
BoxPaintInvalidator::shouldFullyInvalidateBackgroundOnLayoutOverflowChange?

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

https://codereview.chromium.org/2699463004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutView.cpp:296: if
(backgroundPositioningObject && backgroundPositioningObject->isBox()) {
could the layout between the code you added above and here invalidate
backgroundPositioningObject?

[Xianzhu, 2017-02-16 20:45:33]

> Why can't this case be handled with something like the already-existing code
in
> BoxPaintInvalidator::shouldFullyInvalidateBackgroundOnLayoutOverflowChange?
>

shouldFullyInvalidateBackgroundOnLayoutOverflowChange() applies only when the
background is affected by layout overflow.
viewport's non-fixed-attachment gradient background is affected by size of the
HTML element.

Have refactored to CL to make the new logic in the new ViewPaintInvalidator
class.

[Xianzhu, 2017-02-16 20:47:09]

https://codereview.chromium.org/2699463004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BoxPaintInvalidator.h (right):

https://codereview.chromium.org/2699463004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.h:30: static LayoutSize
previousBorderBoxSize(const LayoutBox&);
I will follow-up to try adding LayoutBox::m_previousSize which will increase
size of LayoutBox slightly but can simply logic here.

[chrishtr, 2017-02-16 21:11:14]

I'm still not sure what is special about the LayoutView, even with
--root-layer-scrolls.
Why can't this gradient bug happen for regular boxes, in other words?
Is it the weird rule about stealing the background from the body, and so
LayoutView depends
on another layout object, namely the HTML element?

https://codereview.chromium.org/2699463004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp (right):

https://codereview.chromium.org/2699463004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:317: // know its
previous border box size.
Even if empty? I wonder in which case.

https://codereview.chromium.org/2699463004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:320: // full
invalidate once the paint rect becomes non-empty.
drive-by nit: s/full/fully/. also "geometries" instead of "Geometries"

https://codereview.chromium.org/2699463004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:382: return
previousBorderBoxSizeInternal(m_box, m_context.oldVisualRect.size());
Wh oldVisualRect() here but visualRect() in the other function variant?

[Xianzhu, 2017-02-16 21:51:15]

On 2017/02/16 21:11:14, chrishtr wrote:
> I'm still not sure what is special about the LayoutView, even with
> --root-layer-scrolls.
> Why can't this gradient bug happen for regular boxes, in other words?
> Is it the weird rule about stealing the background from the body, and so
> LayoutView depends
> on another layout object, namely the HTML element?

Yes, non-fixed-attachment view background is special because it always uses the
HTML element as the positioning area, even if the background is from the body:
https://drafts.csswg.org/css-backgrounds-3/#root-background. Firefox also works
like this.

This won't change with --root-layer-scrolls.

[Xianzhu, 2017-02-16 22:01:58]

https://codereview.chromium.org/2699463004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp (right):

https://codereview.chromium.org/2699463004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:317: // know its
previous border box size.
On 2017/02/16 21:11:14, chrishtr wrote:
> Even if empty? I wonder in which case.

The size of HTML element affects viewport background even if the HTML element
has no painted output (e.g. it has visibility:hidden which causes empty
paintInvalidationSize).

If both paintInvalidationSize and borderSize are empty, we'll still return false
at the end of this function.

Updated comments.

https://codereview.chromium.org/2699463004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:320: // full
invalidate once the paint rect becomes non-empty.
On 2017/02/16 21:11:14, chrishtr wrote:
> drive-by nit: s/full/fully/. also "geometries" instead of "Geometries"

Done.

https://codereview.chromium.org/2699463004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:382: return
previousBorderBoxSizeInternal(m_box, m_context.oldVisualRect.size());
On 2017/02/16 21:11:14, chrishtr wrote:
> Wh oldVisualRect() here but visualRect() in the other function variant?

Added comments:
// This is used during paint invalidation of the m_box. PaintInvalidator has
// already updated m_box.visualRect() to the current value, so we need to use
// context.oldVisualRect as the default value.

[Xianzhu, 2017-02-16 22:40:22]

Description was changed from

==========
Fix gradient background invalidation when HTML size changes

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

to

==========
Fix viewport background invalidation when HTML size changes

Non-fixed-attachment viewport background uses root element as the
background positioning area. If HTML size changes, and the background
needs paint invalidation on the resize, we should invalidate the
LayoutView.

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

[Xianzhu, 2017-02-16 22:40:44]

Updated CL description to include more information.

[trchen, 2017-02-16 22:50:44]

On 2017/02/16 21:11:14, chrishtr wrote:
> I'm still not sure what is special about the LayoutView, even with
> --root-layer-scrolls.
> Why can't this gradient bug happen for regular boxes, in other words?
> Is it the weird rule about stealing the background from the body, and so

There are two weird rules, <html> steals background from <body> is resolved
during style computation, and is irrelevant to this CL. The other weird rule
explained below.

> LayoutView depends
> on another layout object, namely the HTML element?

Root background painting is very special:
1. The root background always fill the whole frame, regardless the box of the
root element.
2. The root background does not belong to the root stacking context, but paint
below the root stacking context, thus we let LayoutView paint it.
3. However it uses the box of root element for layout. e.g. respect <html>
transform, using <html> box size for scalable images / scaling / tiling.

Therefore layout changes on the root element will potentially invalidate
LayoutView background. Some of the invalidation were missing or done incorrectly
prior to this CL.

The code lgtm, just needs a comprehensive CL description. :D

[chrishtr, 2017-02-16 23:11:25]

https://codereview.chromium.org/2699463004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp (right):

https://codereview.chromium.org/2699463004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:380: LayoutSize
BoxPaintInvalidator::previousBorderBoxSize(const LayoutBox& box) {
It's weird that there are two methods with the same name that use a different
implicit rect. How about just removing these non-static methods and making
everyone pass two args to previousBorderBoxSizeInternal? (which would be
renamed).

https://codereview.chromium.org/2699463004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/ViewPaintInvalidator.cpp (right):

https://codereview.chromium.org/2699463004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/ViewPaintInvalidator.cpp:13:
PaintInvalidationReason ViewPaintInvalidator::invalidatePaintIfNeeded() {
Indentation nit:

if (m_view.styleRef().hasEntirelyFixedBackground())
  return;
...

https://codereview.chromium.org/2699463004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/ViewPaintInvalidator.cpp:16: // document
element.
Please link to the spec here, for reference.

https://codereview.chromium.org/2699463004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/ViewPaintInvalidator.cpp:26:
LayoutBox::mustInvalidateFillLayersPaintOnWidthChange(
This looks like mostly the same code as
LayoutView::setShouldDoFullPaintInvalidationOnResizeIfNeeded. Re-use/
refactor to re-use?

[Xianzhu, 2017-02-16 23:39:35]

https://codereview.chromium.org/2699463004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp (right):

https://codereview.chromium.org/2699463004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp:380: LayoutSize
BoxPaintInvalidator::previousBorderBoxSize(const LayoutBox& box) {
On 2017/02/16 23:11:25, chrishtr wrote:
> It's weird that there are two methods with the same name that use a different
> implicit rect. How about just removing these non-static methods and making
> everyone pass two args to previousBorderBoxSizeInternal? (which would be
> renamed).

Done.

https://codereview.chromium.org/2699463004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/ViewPaintInvalidator.cpp (right):

https://codereview.chromium.org/2699463004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/ViewPaintInvalidator.cpp:13:
PaintInvalidationReason ViewPaintInvalidator::invalidatePaintIfNeeded() {
On 2017/02/16 23:11:25, chrishtr wrote:
> Indentation nit:
> 
> if (m_view.styleRef().hasEntirelyFixedBackground())
>   return;
> ...

Done.

https://codereview.chromium.org/2699463004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/ViewPaintInvalidator.cpp:16: // document
element.
On 2017/02/16 23:11:25, chrishtr wrote:
> Please link to the spec here, for reference.

Done.

https://codereview.chromium.org/2699463004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/ViewPaintInvalidator.cpp:26:
LayoutBox::mustInvalidateFillLayersPaintOnWidthChange(
On 2017/02/16 23:11:25, chrishtr wrote:
> This looks like mostly the same code as
> LayoutView::setShouldDoFullPaintInvalidationOnResizeIfNeeded. Re-use/
> refactor to re-use?

Currently we can only reuse 4 lines of code because other conditions are
different. Now we still need the fixed-attachment code in LayoutView because we
don't have the previous view size here. Will move the fixed-attachment code here
when we enable rootLayerScrolling permanently (when we can use
previousBorderBoxSize as the viewport size).

[chrishtr, 2017-02-16 23:49:46]

lgtm

[Xianzhu, 2017-02-17 00:14:46]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-02-17 00:14:47]

The patchset sent to the CQ was uploaded after l-g-t-m from trchen@chromium.org,
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2699463004/#ps130001
(title: "Rebaseline tests")

[commit-bot: I haz the power, 2017-02-17 00:16: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, 2017-02-17 02:19:24]

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

[commit-bot: I haz the power, 2017-02-17 02:19:26]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Xianzhu, 2017-02-17 02:26:18]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2017-02-17 02:27:52]

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

[Xianzhu, 2017-02-17 03:50:26]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-02-17 03:50:26]

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

[commit-bot: I haz the power, 2017-02-17 03:51:50]

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-02-17 05:53:43]

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

[commit-bot: I haz the power, 2017-02-17 05:53:45]

Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Xianzhu, 2017-02-17 05:54:20]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2017-02-17 05:55:05]

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-02-17 07:48:16]

CQ is committing da patch.

Bot data: {"patchset_id": 150001, "attempt_start_ts": 1487310860032490,
"parent_rev": "ddd53c2ef164f4e22c55d3d69b9c1fb386119375", "commit_rev":
"d34bb14bf30e4885f704336b0658cd32ba01e108"}

[commit-bot: I haz the power, 2017-02-17 07:48:50]

Description was changed from

==========
Fix viewport background invalidation when HTML size changes

Non-fixed-attachment viewport background uses root element as the
background positioning area. If HTML size changes, and the background
needs paint invalidation on the resize, we should invalidate the
LayoutView.

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

to

==========
Fix viewport background invalidation when HTML size changes

Non-fixed-attachment viewport background uses root element as the
background positioning area. If HTML size changes, and the background
needs paint invalidation on the resize, we should invalidate the
LayoutView.

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

Review-Url: https://codereview.chromium.org/2699463004
Cr-Commit-Position: refs/heads/master@{#451265}
Committed:
https://chromium.googlesource.com/chromium/src/+/d34bb14bf30e4885f704336b0658...
==========

[commit-bot: I haz the power, 2017-02-17 07:48:56]

Committed patchset #9 (id:150001) as
https://chromium.googlesource.com/chromium/src/+/d34bb14bf30e4885f704336b0658...