Add a new document lifecycle; CompositingInputsClean

Add a new document lifecycle; CompositingInputsClean

The goal here is to be able to clean the compositing inputs without
running full compositing or starting animations. This is necessary for
properly computing compositing inputs-related properties such as sticky
(see the bug for details on that).

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

Review-Url: https://codereview.chromium.org/2795263002
Cr-Commit-Position: refs/heads/master@{#465248}
Committed: https://chromium.googlesource.com/chromium/src/+/79d50ca3b3326c45c7d041bc9cd8f5abd64b24cb

[smcgruer, 2017-04-04 15:18:57]

Description was changed from

==========
Add a new document lifecycle; CompositingInputsClean

BUG=672457
==========

to

==========
Add a new document lifecycle; CompositingInputsClean

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

[smcgruer, 2017-04-04 15:21:54]

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

[commit-bot: I haz the power, 2017-04-04 15:22:22]

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

[smcgruer, 2017-04-04 17:27:15]

smcgruer@chromium.org changed reviewers:
+ flackr@chromium.org

[smcgruer, 2017-04-04 17:27:16]

FYI: my first approach at this. Some refactoring of updateIfNeeded could be done
to make the code nicer, and obviously we need some tests but this is the basic
behavioral idea.

[commit-bot: I haz the power, 2017-04-04 18:13:29]

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

[commit-bot: I haz the power, 2017-04-04 18:13:30]

Dry run: This issue passed the CQ dry run.

[smcgruer, 2017-04-07 14:27:02]

This is now at the point where it still needs refactoring and clean-up, but I
believe it clearly communicates the planned change in behavior so PTAL :)

[flackr, 2017-04-11 20:46:28]

https://codereview.chromium.org/2795263002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2795263002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2924: // TODO(smcgruer):
DCHECK !RuntimeEnabledFeatures::slimmingPaintV2Enabled() ?
This is never called with slimming paint V2? Are the sticky constraints correct
if you run the same test with spv2 enabled?

https://codereview.chromium.org/2795263002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/2795263002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:424:
CompositingInputsUpdater::AssertNeedsCompositingInputsUpdateBitsCleared(
We should have this assert for CompositingInputsClean. In fact, if compositing
inputs are dirty the pending update type should be at least
kCompositingUpdateAfterCompositingInputChange (which seems to be the case in
UpdateWithoutAcceleratedCompositing) in which case you can use this code to
update the inputs and have the early exit here.

https://codereview.chromium.org/2795263002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositorTest.cpp
(right):

https://codereview.chromium.org/2795263002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositorTest.cpp:46:
GetDocument().Lifecycle().GetState());
Also check that the compositing inputs are clean on a box.

[smcgruer, 2017-04-12 15:33:05]

https://codereview.chromium.org/2795263002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2795263002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2924: // TODO(smcgruer):
DCHECK !RuntimeEnabledFeatures::slimmingPaintV2Enabled() ?
On 2017/04/11 20:46:28, flackr wrote:
> This is never called with slimming paint V2? Are the sticky constraints
correct
> if you run the same test with spv2 enabled?

This should never be called with spv2, as the entire compositing lifecycle
doesn't exist there. Instead, sticky constraints will be cleaned in PrePaint and
animations started in Paint, so we will just target PrePaint for sticky-subtree
offsetTop/etc in spv2.

https://codereview.chromium.org/2795263002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/2795263002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:424:
CompositingInputsUpdater::AssertNeedsCompositingInputsUpdateBitsCleared(
On 2017/04/11 20:46:28, flackr wrote:
> We should have this assert for CompositingInputsClean. In fact, if compositing
> inputs are dirty the pending update type should be at least
> kCompositingUpdateAfterCompositingInputChange (which seems to be the case in
> UpdateWithoutAcceleratedCompositing) in which case you can use this code to
> update the inputs and have the early exit here.

Done.

https://codereview.chromium.org/2795263002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositorTest.cpp
(right):

https://codereview.chromium.org/2795263002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositorTest.cpp:46:
GetDocument().Lifecycle().GetState());
On 2017/04/11 20:46:28, flackr wrote:
> Also check that the compositing inputs are clean on a box.

Done.

[flackr, 2017-04-12 15:50:15]

LGTM but let's have someone more familiar with the lifecycle take a look.

https://codereview.chromium.org/2795263002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2795263002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2924: // TODO(smcgruer):
DCHECK !RuntimeEnabledFeatures::slimmingPaintV2Enabled() ?
On 2017/04/12 15:33:04, smcgruer wrote:
> On 2017/04/11 20:46:28, flackr wrote:
> > This is never called with slimming paint V2? Are the sticky constraints
> correct
> > if you run the same test with spv2 enabled?
> 
> This should never be called with spv2, as the entire compositing lifecycle
> doesn't exist there. Instead, sticky constraints will be cleaned in PrePaint
and
> animations started in Paint, so we will just target PrePaint for
sticky-subtree
> offsetTop/etc in spv2.

Acknowledged.

https://codereview.chromium.org/2795263002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/2795263002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:455:
if (target_state == DocumentLifecycle::kCompositingInputsClean) {
I think this early exit would be cleaner at the top of the function before we
replace the pending_update_type_, i.e. if update_type <
kCompositingUpdateAfterCompositingInputChange.

[smcgruer, 2017-04-12 17:18:44]

https://codereview.chromium.org/2795263002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/2795263002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:455:
if (target_state == DocumentLifecycle::kCompositingInputsClean) {
On 2017/04/12 15:50:15, flackr wrote:
> I think this early exit would be cleaner at the top of the function before we
> replace the pending_update_type_, i.e. if update_type <
> kCompositingUpdateAfterCompositingInputChange.

Done.

[smcgruer, 2017-04-12 17:22:14]

Description was changed from

==========
Add a new document lifecycle; CompositingInputsClean

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

to

==========
Add a new document lifecycle; CompositingInputsClean

The goal here is to be able to clean the compositing inputs without
running full compositing or starting animations. This is necessary for
properly computing compositing inputs-related properties such as sticky
(see the bug for details on that).

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

[smcgruer, 2017-04-12 17:23:51]

+chrishtr for thoughts on the added lifecycle, +suzyh to make sure everything is
ok regarding animations.

[smcgruer, 2017-04-12 17:24:18]

smcgruer@chromium.org changed reviewers:
+ chrishtr@chromium.org, suzyh@chromium.org

[smcgruer, 2017-04-12 21:18:11]

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

[commit-bot: I haz the power, 2017-04-12 21:18:55]

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-04-12 21:25:51]

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

[commit-bot: I haz the power, 2017-04-12 21:25:51]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[smcgruer, 2017-04-12 22:57:08]

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

[commit-bot: I haz the power, 2017-04-12 22:57:48]

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-04-13 03:11:10]

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

[commit-bot: I haz the power, 2017-04-13 03:11:12]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2017-04-13 20:48:16]

lgtm

[suzyh_UTC10 (ex-contributor), 2017-04-18 06:09:25]

On 2017/04/12 at 17:23:51, smcgruer wrote:
> +chrishtr for thoughts on the added lifecycle, +suzyh to make sure everything
is ok regarding animations.

I find it difficult to review this from not having a good grasp of the overall
system. Is there a high-level design doc you could point me at? Alternatively,
is there a limited part of the CL that you were hoping for my thoughts on?

[smcgruer, 2017-04-18 13:21:19]

Sorry Suzy, I should have provided more context for you.  There is not a design
document, but my intention in adding you as a reviewer was an FYI/sanity-check
as the previous attempt to fix crbug.com/672457 resulted in me breaking the
animation lifecycle accidentally.

The next CL for this issue will be the one that actually has behavioral changes
(this CL just adds a new lifecycle step, no non-test code actually targets it
yet), so what I will do is:

1. Submit this.
2. Put together a background document explaining the context for the next CL.
3. Add you as a reviewer on that CL so you can help confirm I am not
accidentally breaking the animation lifecycle again :).

[smcgruer, 2017-04-18 13:29:36]

The CQ bit was checked by smcgruer@chromium.org

[smcgruer, 2017-04-18 13:29:36]

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

[commit-bot: I haz the power, 2017-04-18 13:30:02]

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-04-18 15:47:08]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492522176352570,
"parent_rev": "ebcb764059b9e3b054261e82fbead33cba9a14d7", "commit_rev":
"79d50ca3b3326c45c7d041bc9cd8f5abd64b24cb"}

[commit-bot: I haz the power, 2017-04-18 15:47:54]

Description was changed from

==========
Add a new document lifecycle; CompositingInputsClean

The goal here is to be able to clean the compositing inputs without
running full compositing or starting animations. This is necessary for
properly computing compositing inputs-related properties such as sticky
(see the bug for details on that).

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

to

==========
Add a new document lifecycle; CompositingInputsClean

The goal here is to be able to clean the compositing inputs without
running full compositing or starting animations. This is necessary for
properly computing compositing inputs-related properties such as sticky
(see the bug for details on that).

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

Review-Url: https://codereview.chromium.org/2795263002
Cr-Commit-Position: refs/heads/master@{#465248}
Committed:
https://chromium.googlesource.com/chromium/src/+/79d50ca3b3326c45c7d041bc9cd8...
==========

[commit-bot: I haz the power, 2017-04-18 15:47:55]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/79d50ca3b3326c45c7d041bc9cd8...

[suzyh_UTC10 (ex-contributor), 2017-04-18 23:34:22]

On 2017/04/18 at 13:21:19, smcgruer wrote:
> Sorry Suzy, I should have provided more context for you.  There is not a
design document, but my intention in adding you as a reviewer was an
FYI/sanity-check as the previous attempt to fix crbug.com/672457 resulted in me
breaking the animation lifecycle accidentally.
> 
> The next CL for this issue will be the one that actually has behavioral
changes (this CL just adds a new lifecycle step, no non-test code actually
targets it yet), so what I will do is:
> 
> 1. Submit this.
> 2. Put together a background document explaining the context for the next CL.
> 3. Add you as a reviewer on that CL so you can help confirm I am not
accidentally breaking the animation lifecycle again :).

SGTM, thanks.