Revert the removal of CompositingReasonOutOfFlowClipping

Revert the removal of CompositingReasonOutOfFlowClipping

Change https://codereview.chromium.org/2382563002/ modified
the logic for compositing layers with a clip parent when
we prefer compositing to LCD text. Revert that change to fix
painting of fixed position clipped elements over accelerated
scrolling elements.

R=flackr@chromium.org
BUG=650446
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/2d9b508d37b0fac7f0b3c4fcce5f8a278d4fb9d7
Cr-Commit-Position: refs/heads/master@{#423609}

[Stephen Chennney, 2016-10-04 19:27:51]

Description was changed from

==========
Revert the removal of CompositingReasonOutOfFlowClipping

Change https://codereview.chromium.org/2382563002/ modified
the logic for compositing layers with a clip parent when
we prefer compositing to LCD text. Revert that change to fix
painting of fixed position clipped elements over accelerated
scrolling elements.

R=flackr@chromium.org
BUG=650446
==========

to

==========
Revert the removal of CompositingReasonOutOfFlowClipping

Change https://codereview.chromium.org/2382563002/ modified
the logic for compositing layers with a clip parent when
we prefer compositing to LCD text. Revert that change to fix
painting of fixed position clipped elements over accelerated
scrolling elements.

R=flackr@chromium.org
BUG=650446
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Stephen Chennney, 2016-10-04 19:28:06]

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

[commit-bot: I haz the power, 2016-10-04 19:28:25]

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

[flackr, 2016-10-04 19:31:44]

lgtm

[Stephen Chennney, 2016-10-04 19:34:15]

The CQ bit was unchecked by schenney@chromium.org

[Stephen Chennney, 2016-10-04 19:34:16]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2016-10-04 19:34: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, 2016-10-04 21:03:29]

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

[commit-bot: I haz the power, 2016-10-04 21:03:30]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Stephen Chennney, 2016-10-05 13:10:11]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2016-10-05 13:10:11]

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/2389293002/#ps20001
(title: "New baseline for
compositing/overflow/scroll-parent-absolute-with-backdrop-filter.html")

[commit-bot: I haz the power, 2016-10-05 13:10:23]

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-05 14:34:17]

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

[commit-bot: I haz the power, 2016-10-05 14:34:18]

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

[Stephen Chennney, 2016-10-06 16:23:51]

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

[commit-bot: I haz the power, 2016-10-06 16:24:06]

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

[Stephen Chennney, 2016-10-06 16:24:39]

I think this needs another review after I changed all the ASSERTS and some
formatting. There is really no contentful change, but a sanity check would be
good.

[commit-bot: I haz the power, 2016-10-06 16:37:16]

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

[commit-bot: I haz the power, 2016-10-06 16:37:17]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Stephen Chennney, 2016-10-06 16:46:49]

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

[commit-bot: I haz the power, 2016-10-06 16:47:12]

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

[flackr, 2016-10-06 16:52:22]

On 2016/10/06 at 16:24:39, schenney wrote:
> I think this needs another review after I changed all the ASSERTS and some
formatting. There is really no contentful change, but a sanity check would be
good.

I don't understand, why did all of these ASSERTS need to be changed? I didn't
see any failures other than the one test in the tryjob run for patch 3. Also, if
they were causing failures, won't the DCHECKs fire in debug mode?

[Stephen Chennney, 2016-10-06 16:55:55]

On 2016/10/06 16:52:22, flackr wrote:
> On 2016/10/06 at 16:24:39, schenney wrote:
> > I think this needs another review after I changed all the ASSERTS and some
> formatting. There is really no contentful change, but a sanity check would be
> good.
> 
> I don't understand, why did all of these ASSERTS need to be changed? I didn't
> see any failures other than the one test in the tryjob run for patch 3. Also,
if
> they were causing failures, won't the DCHECKs fire in debug mode?

The pre-submit check barfed on all the ASSERTS and I did not see a "continue
anyway" option. I'm guessing that something recently changed after the great
line width reformat.

The asserts need to be changed at some point. We might as well deal with it now.

[commit-bot: I haz the power, 2016-10-06 16:59:29]

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

[commit-bot: I haz the power, 2016-10-06 16:59:31]

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

[flackr, 2016-10-06 17:01:05]

On 2016/10/06 at 16:55:55, schenney wrote:
> On 2016/10/06 16:52:22, flackr wrote:
> > On 2016/10/06 at 16:24:39, schenney wrote:
> > > I think this needs another review after I changed all the ASSERTS and some
> > formatting. There is really no contentful change, but a sanity check would
be
> > good.
> > 
> > I don't understand, why did all of these ASSERTS need to be changed? I
didn't
> > see any failures other than the one test in the tryjob run for patch 3.
Also, if
> > they were causing failures, won't the DCHECKs fire in debug mode?
> 
> The pre-submit check barfed on all the ASSERTS and I did not see a "continue
anyway" option. I'm guessing that something recently changed after the great
line width reformat.
> 
> The asserts need to be changed at some point. We might as well deal with it
now.

Ah okay, I suppose it is just requiring chromium style checks.

[flackr, 2016-10-06 17:01:14]

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

https://codereview.chromium.org/2389293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:138:
(fastAnswer == slowAnswer);
DCHECK_EQ?

https://codereview.chromium.org/2389293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:157:
(layer->stackingNode()->isStackingContext());
DCHECK?

[Stephen Chennney, 2016-10-06 17:06:51]

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

[commit-bot: I haz the power, 2016-10-06 17:07:16]

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

[Stephen Chennney, 2016-10-06 17:07:52]

On 2016/10/06 17:01:14, flackr wrote:
>
https://codereview.chromium.org/2389293002/diff/60001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
> (right):
> 
>
https://codereview.chromium.org/2389293002/diff/60001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:138:
> (fastAnswer == slowAnswer);
> DCHECK_EQ?
> 
>
https://codereview.chromium.org/2389293002/diff/60001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:157:
> (layer->stackingNode()->isStackingContext());
> DCHECK?

Fixed those. This is embarrassing, but I'm on a Macbook with a poor internet
connection so building to verify changes is a real pain.

[flackr, 2016-10-06 17:23:44]

LGTM with nits

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

https://codereview.chromium.org/2389293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:44:
DCHECK(potentialCompositingReasonsFromStyle(layer->layoutObject()) ==
nit: DCHECK_EQ(potentialCompositingReasonsFromStyle(layer->layoutObject()),
layer->potentialCompositingReasonsFromStyle())

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

https://codereview.chromium.org/2389293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:138:
DCHECK_EQ(fastAnswer, slowAnswer);
nit: expected value should come first for equality checks (I'm not sure if it
technically matters for DCHECK but for many of our macros the error message
shows "expected" and "actual" in the resulting message), in this case slowAnswer
would be the expected value since we don't normally compute it.

[Stephen Chennney, 2016-10-06 17:31:24]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2016-10-06 17:31:24]

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/2389293002/#ps100001
(title: "Improve DCHECKs.")

[commit-bot: I haz the power, 2016-10-06 17:32:04]

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-06 18:52:43]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-06 18:55:07]

Description was changed from

==========
Revert the removal of CompositingReasonOutOfFlowClipping

Change https://codereview.chromium.org/2382563002/ modified
the logic for compositing layers with a clip parent when
we prefer compositing to LCD text. Revert that change to fix
painting of fixed position clipped elements over accelerated
scrolling elements.

R=flackr@chromium.org
BUG=650446
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Revert the removal of CompositingReasonOutOfFlowClipping

Change https://codereview.chromium.org/2382563002/ modified
the logic for compositing layers with a clip parent when
we prefer compositing to LCD text. Revert that change to fix
painting of fixed position clipped elements over accelerated
scrolling elements.

R=flackr@chromium.org
BUG=650446
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/2d9b508d37b0fac7f0b3c4fcce5f8a278d4fb9d7
Cr-Commit-Position: refs/heads/master@{#423609}
==========

[commit-bot: I haz the power, 2016-10-06 18:55:08]

Patchset 6 (id:??) landed as
https://crrev.com/2d9b508d37b0fac7f0b3c4fcce5f8a278d4fb9d7
Cr-Commit-Position: refs/heads/master@{#423609}