Don't skip logic in in UpdateLifecyclePhasesInternal for kCompositingInputsClean

Don't skip logic in in UpdateLifecyclePhasesInternal for kCompositingInputsClean

This was an oversight when I originally implemented kCompositingInputsUpdate;
the early-exit from UpdateLifecyclePhasesInternal meant that some necessary
logic was being skipped accidentally.

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

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

[smcgruer, 2017-04-27 20:27:58]

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

[commit-bot: I haz the power, 2017-04-27 20:28:27]

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-27 20:30:51]

Description was changed from

==========
Reset allows_layout_invalidation_after_layout_clean_ for
kCompositingInputsUpdate

This was an oversight when I originally implemented kCompositingInputsUpdate;
the early-exit from UpdateLifecyclePhasesInternal meant that
allows_layout_invalidation_after_layout_clean_ was not reset when targeting
kCompositingInputsUpdate.

BUG=716150
==========

to

==========
Don't skip logic in in UpdateLifecyclePhasesInternal for kCompositingInputsClean

This was an oversight when I originally implemented kCompositingInputsUpdate;
the early-exit from UpdateLifecyclePhasesInternal meant that some necessary
logic was being skipped accidentally.

BUG=716150
==========

[smcgruer, 2017-04-27 20:31:21]

Description was changed from

==========
Don't skip logic in in UpdateLifecyclePhasesInternal for kCompositingInputsClean

This was an oversight when I originally implemented kCompositingInputsUpdate;
the early-exit from UpdateLifecyclePhasesInternal meant that some necessary
logic was being skipped accidentally.

BUG=716150
==========

to

==========
Don't skip logic in in UpdateLifecyclePhasesInternal for kCompositingInputsClean

This was an oversight when I originally implemented kCompositingInputsUpdate;
the early-exit from UpdateLifecyclePhasesInternal meant that some necessary
logic was being skipped accidentally.

BUG=716150
==========

[commit-bot: I haz the power, 2017-04-27 21:00:42]

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

[commit-bot: I haz the power, 2017-04-27 21:00:44]

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_...)

[smcgruer, 2017-04-28 01:55:21]

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

[commit-bot: I haz the power, 2017-04-28 01:56:40]

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-28 02:46:24]

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

[commit-bot: I haz the power, 2017-04-28 02:46:25]

Dry run: This issue passed the CQ dry run.

[smcgruer, 2017-04-28 13:24:28]

Description was changed from

==========
Don't skip logic in in UpdateLifecyclePhasesInternal for kCompositingInputsClean

This was an oversight when I originally implemented kCompositingInputsUpdate;
the early-exit from UpdateLifecyclePhasesInternal meant that some necessary
logic was being skipped accidentally.

BUG=716150
==========

to

==========
Don't skip logic in in UpdateLifecyclePhasesInternal for kCompositingInputsClean

This was an oversight when I originally implemented kCompositingInputsUpdate;
the early-exit from UpdateLifecyclePhasesInternal meant that some necessary
logic was being skipped accidentally.

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

[smcgruer, 2017-04-28 13:26:34]

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

[smcgruer, 2017-04-28 13:26:39]

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

[commit-bot: I haz the power, 2017-04-28 13:27:04]

Dry run: CQ is trying da patch.

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

[flackr, 2017-04-28 13:38:28]

LGTM

[commit-bot: I haz the power, 2017-04-28 15:32:58]

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

[commit-bot: I haz the power, 2017-04-28 15:32:58]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2017-04-28 15:35:51]

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

https://codereview.chromium.org/2844993004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3128: void
FrameView::UpdateLayerTree(
I don't think UpdateLayerTree means much of anything - prefer
to keep the logic inlined in the calling method.

[smcgruer, 2017-04-28 15:40:35]

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

https://codereview.chromium.org/2844993004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3128: void
FrameView::UpdateLayerTree(
On 2017/04/28 15:35:51, chrishtr wrote:
> I don't think UpdateLayerTree means much of anything - prefer
> to keep the logic inlined in the calling method.

Name was taken from the TRACE_EVENT1 that this entire logic is considered 'part
of'.

I can inline it back into the caller, but that makes handling the early-exit
uglier. Would have to add another layer of 'if' nesting for target_state >
DocumentLifecycle::kCompositingInputsClean, unless either yourself or flackr
have a better idea?

[smcgruer, 2017-04-28 15:40:35]

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

https://codereview.chromium.org/2844993004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3128: void
FrameView::UpdateLayerTree(
On 2017/04/28 15:35:51, chrishtr wrote:
> I don't think UpdateLayerTree means much of anything - prefer
> to keep the logic inlined in the calling method.

Name was taken from the TRACE_EVENT1 that this entire logic is considered 'part
of'.

I can inline it back into the caller, but that makes handling the early-exit
uglier. Would have to add another layer of 'if' nesting for target_state >
DocumentLifecycle::kCompositingInputsClean, unless either yourself or flackr
have a better idea?

[flackr, 2017-04-28 15:42:24]

On 2017/04/28 15:40:35, smcgruer wrote:
>
https://codereview.chromium.org/2844993004/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/2844993004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/FrameView.cpp:3128: void
> FrameView::UpdateLayerTree(
> On 2017/04/28 15:35:51, chrishtr wrote:
> > I don't think UpdateLayerTree means much of anything - prefer
> > to keep the logic inlined in the calling method.
> 
> Name was taken from the TRACE_EVENT1 that this entire logic is considered
'part
> of'.
> 
> I can inline it back into the caller, but that makes handling the early-exit
> uglier. Would have to add another layer of 'if' nesting for target_state >
> DocumentLifecycle::kCompositingInputsClean, unless either yourself or flackr
> have a better idea?

We could wrap the work that needs to be done before returning into a scoped
class destructor.

[chrishtr, 2017-04-28 18:28:56]

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

https://codereview.chromium.org/2844993004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3128: void
FrameView::UpdateLayerTree(
On 2017/04/28 at 15:40:35, smcgruer wrote:
> On 2017/04/28 15:35:51, chrishtr wrote:
> > I don't think UpdateLayerTree means much of anything - prefer
> > to keep the logic inlined in the calling method.
> 
> Name was taken from the TRACE_EVENT1 that this entire logic is considered
'part of'.

That trace event has a bad name, unfortuantely. Layer is a very overloaded
concept. The
reason we didn't change it already is that Catapult and DevTools have
dependencies on that
name.

> 
> I can inline it back into the caller, but that makes handling the early-exit
uglier. Would have to add another layer of 'if' nesting for target_state >
DocumentLifecycle::kCompositingInputsClean, unless either yourself or flackr
have a better idea?

goto? LOL

Another idea: wrap this code:

if (target_state >= kCompositingClean) {
ScrollContentsIfNeededRecursive();
DCHECK(RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled() ||
             Lifecycle().GetState() >= DocumentLifecycle::kCompositingClean);
frame_->GetPage()->GlobalRootScrollerController().DidUpdateCompositing();

}


Then the whole function would be structured like:

if (state >= state1) {
  Do state 1 stuff
}

if (state >= state2) {
 Do State 2 stuff
}

etc.

[smcgruer, 2017-04-28 19:32:13]

I have changed the code as per Chris' suggestion. I have also removed a DCHECK
that I believe was already invalid before this CL, for the following reasons:

DCHECK(RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled() ||
Lifecycle().GetState() >= DocumentLifecycle::kCompositingClean);

Ignoring RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled(), which is
currently default-true anyway, let's just look at the lifecycle state. Before
this CL:

1. It is one of kLayoutClean, kCompositingInputsClean, kCompositingClean,
kPrePaintClean, or kPaintClean, because those are the only ones allowed in this
function (via DCHECK above).

2. It is not kLayoutClean, because that early-exits above.

3. It is not kCompositingInputsClean, because that didn't exist when this DCHECK
was here (and will not reach this DCHECK in any version of the code anyway).

4. So it must be kCompositingClean or higher.

Therefore, the DCHECK is always true, therefore we can eliminate it.

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

https://codereview.chromium.org/2844993004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3128: void
FrameView::UpdateLayerTree(
On 2017/04/28 18:28:56, chrishtr wrote:
> On 2017/04/28 at 15:40:35, smcgruer wrote:
> > On 2017/04/28 15:35:51, chrishtr wrote:
> > > I don't think UpdateLayerTree means much of anything - prefer
> > > to keep the logic inlined in the calling method.
> > 
> > Name was taken from the TRACE_EVENT1 that this entire logic is considered
> 'part of'.
> 
> That trace event has a bad name, unfortuantely. Layer is a very overloaded
> concept. The
> reason we didn't change it already is that Catapult and DevTools have
> dependencies on that
> name.
> 
> > 
> > I can inline it back into the caller, but that makes handling the early-exit
> uglier. Would have to add another layer of 'if' nesting for target_state >
> DocumentLifecycle::kCompositingInputsClean, unless either yourself or flackr
> have a better idea?
> 
> goto? LOL
> 
> Another idea: wrap this code:
> 
> if (target_state >= kCompositingClean) {
> ScrollContentsIfNeededRecursive();
> DCHECK(RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled() ||
>              Lifecycle().GetState() >= DocumentLifecycle::kCompositingClean);
> frame_->GetPage()->GlobalRootScrollerController().DidUpdateCompositing();
> 
> }
> 
> 
> Then the whole function would be structured like:
> 
> if (state >= state1) {
>   Do state 1 stuff
> }
> 
> if (state >= state2) {
>  Do State 2 stuff
> }
> 
> etc.

Done.

[smcgruer, 2017-04-28 19:33:38]

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

[commit-bot: I haz the power, 2017-04-28 19:35:11]

Dry run: CQ is trying da patch.

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

[flackr, 2017-04-28 19:38:18]

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

https://codereview.chromium.org/2844993004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3117: if (target_state >=
DocumentLifecycle::kPrePaintClean) {
Can we unnest this condition if there's no follow-on work to be done?

https://codereview.chromium.org/2844993004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3141: if (target_state >=
DocumentLifecycle::kPrePaintClean)
Unless my eyes deceive me, this is the identical check to the branch immeidately
above it and target_state doesn't change, we should just move the code up.

[smcgruer, 2017-04-28 19:40:03]

The CQ bit was unchecked by smcgruer@chromium.org

[smcgruer, 2017-04-28 19:40:56]

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

[commit-bot: I haz the power, 2017-04-28 19:42:09]

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-28 19:42:29]

The CQ bit was unchecked by smcgruer@chromium.org

[smcgruer, 2017-04-28 19:43:38]

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

[smcgruer, 2017-04-28 19:43:44]

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

https://codereview.chromium.org/2844993004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3117: if (target_state >=
DocumentLifecycle::kPrePaintClean) {
On 2017/04/28 19:38:18, flackr wrote:
> Can we unnest this condition if there's no follow-on work to be done?

Uh. Yes. I am silly.

https://codereview.chromium.org/2844993004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3141: if (target_state >=
DocumentLifecycle::kPrePaintClean)
On 2017/04/28 19:38:18, flackr wrote:
> Unless my eyes deceive me, this is the identical check to the branch
immeidately
> above it and target_state doesn't change, we should just move the code up.

Done.

[commit-bot: I haz the power, 2017-04-28 19:44:31]

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-28 21:41:44]

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

[commit-bot: I haz the power, 2017-04-28 21:41:45]

Dry run: This issue passed the CQ dry run.

[smcgruer, 2017-05-03 19:28:48]

Ping for BLINK owners since I just realized this is blocking
https://codereview.chromium.org/2839263002/

[chrishtr, 2017-05-03 21:12:48]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-05-03 21:12:49]

lgtm

[chrishtr, 2017-05-03 21:12:49]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org
Link to the patchset: https://codereview.chromium.org/2844993004/#ps100001
(title: "Additional comment fix")

[commit-bot: I haz the power, 2017-05-03 21:13:13]

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-05-03 23:12:36]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493845968625670,
"parent_rev": "9dd38d20453e05cecaa0f883f0865da3837a380c", "commit_rev":
"dc545e9fc108b42ff88ae4d696ea4b5ed3bffcc1"}

[commit-bot: I haz the power, 2017-05-03 23:12:51]

Description was changed from

==========
Don't skip logic in in UpdateLifecyclePhasesInternal for kCompositingInputsClean

This was an oversight when I originally implemented kCompositingInputsUpdate;
the early-exit from UpdateLifecyclePhasesInternal meant that some necessary
logic was being skipped accidentally.

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

to

==========
Don't skip logic in in UpdateLifecyclePhasesInternal for kCompositingInputsClean

This was an oversight when I originally implemented kCompositingInputsUpdate;
the early-exit from UpdateLifecyclePhasesInternal meant that some necessary
logic was being skipped accidentally.

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

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

[commit-bot: I haz the power, 2017-05-03 23:12:53]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/dc545e9fc108b42ff88ae4d696ea...