DCHECK that paint properties are never null

DCHECK that paint properties are never null

This patch adds a new DCHECK that paint chunk properties are not null,
along with comments describing how to fix this situation if one is
unlucky enough to hit it. Now that we have root paint property nodes,
the properties of a paint chunk should never be null.

The asserts in this patch uncovered a bug where FrameView scrollbars
were painted with null paint property nodes. This has been fixed by
ensuring all properties, not just the transform property, are
specified in FramePainter. This fixed 18 test failures due to the new
assert.

The asserts in this patch also uncovered a case where we were using
null paint properties in SkPictureBuilder when optimizing the display
item list of the CompositingRecorder. An assert disabler has been
added because this case is safe, and comments have been added
describing why.

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

Committed: https://crrev.com/29bb3301df59c321abee8d9d226bf38e3877a5fb
Cr-Commit-Position: refs/heads/master@{#431967}

[pdr., 2016-11-04 20:58:21]

pdr@chromium.org changed reviewers:
+ szager@chromium.org, trchen@chromium.org

[pdr., 2016-11-04 20:58:52]

This is ready for review, though it can't land until two other patches land that
fix existing asserts.

[szager1, 2016-11-08 21:02:51]

lgtm

[pdr., 2016-11-12 02:10:54]

Description was changed from

==========
DCHECK that paint properties are never null

This patch adds a new DCHECK that paint chunk properties are not null,
along with comments describing how to fix this situation if one is
unlucky enough to hit it. Now that we have root paint property nodes,
the properties of a paint chunk should never be null.

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

to

==========
DCHECK that paint properties are never null

This patch adds a new DCHECK that paint chunk properties are not null,
along with comments describing how to fix this situation if one is
unlucky enough to hit it. Now that we have root paint property nodes,
the properties of a paint chunk should never be null.

This assert in this patch uncovered a bug where FrameView scrollbars
were painted with null paint property nodes. This has been fixed by
ensuring all properties, not just the transform property, are
specified in FramePainter. This fixed 18 test failures due to the new
assert.

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

[pdr., 2016-11-12 02:13:02]

Stefan, this patch caught another case of null properties in FramePainter. I've
fixed that in the latest patch. Can you take another look? If you like it,
please Commit+CQ.

[pdr., 2016-11-12 02:13:20]

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

[commit-bot: I haz the power, 2016-11-12 02:13:46]

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, 2016-11-12 03:55:52]

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

[commit-bot: I haz the power, 2016-11-12 03:55:55]

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

[pdr., 2016-11-13 06:07:27]

Description was changed from

==========
DCHECK that paint properties are never null

This patch adds a new DCHECK that paint chunk properties are not null,
along with comments describing how to fix this situation if one is
unlucky enough to hit it. Now that we have root paint property nodes,
the properties of a paint chunk should never be null.

This assert in this patch uncovered a bug where FrameView scrollbars
were painted with null paint property nodes. This has been fixed by
ensuring all properties, not just the transform property, are
specified in FramePainter. This fixed 18 test failures due to the new
assert.

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

to

==========
DCHECK that paint properties are never null

This patch adds a new DCHECK that paint chunk properties are not null,
along with comments describing how to fix this situation if one is
unlucky enough to hit it. Now that we have root paint property nodes,
the properties of a paint chunk should never be null.

The asserts in this patch uncovered a bug where FrameView scrollbars
were painted with null paint property nodes. This has been fixed by
ensuring all properties, not just the transform property, are
specified in FramePainter. This fixed 18 test failures due to the new
assert.

The asserts in this patch also uncovered a case where we were using
null paint properties in SkPictureBuilder when optimizing the display
item list of the CompositingRecorder. An assert disabler has been
added because this case is safe, and comments have been added
describing why.

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

[pdr., 2016-11-13 06:08:26]

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

[commit-bot: I haz the power, 2016-11-13 06:08:36]

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, 2016-11-13 06:20:07]

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

[commit-bot: I haz the power, 2016-11-13 06:20:08]

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

[pdr., 2016-11-13 07:41:56]

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

[commit-bot: I haz the power, 2016-11-13 07:42:08]

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, 2016-11-13 09:08:34]

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

[commit-bot: I haz the power, 2016-11-13 09:08:35]

Dry run: This issue passed the CQ dry run.

[szager1, 2016-11-14 05:34:04]

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

https://codereview.chromium.org/2480863002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/FramePainter.cpp:107: auto* scrollBarScroll
= contentsState->scroll();
As far as I can tell, contentsState->scroll() should always be the same as
m_frameView->scroll().  Under what circumstance would that not be true
(necessitating this fall back logic)?

[pdr., 2016-11-14 17:57:52]

On 2016/11/14 at 05:34:04, szager wrote:
>
https://codereview.chromium.org/2480863002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/FramePainter.cpp (right):
> 
>
https://codereview.chromium.org/2480863002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/FramePainter.cpp:107: auto*
scrollBarScroll = contentsState->scroll();
> As far as I can tell, contentsState->scroll() should always be the same as
m_frameView->scroll().  Under what circumstance would that not be true
(necessitating this fall back logic)?

This special case is for situations where we don't create a scroll node for the
FrameView, from
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai....
When we don't create a scroll node, the frameview has a null scroll node but the
contents state inherits the scroll node from the existing context. For example,
a document that does not scroll would have a null frameview scroll node but the
contents state would point to the root scroll node.

[szager1, 2016-11-14 21:32:08]

lgtm

[szager1, 2016-11-14 21:32:16]

The CQ bit was checked by szager@chromium.org

[commit-bot: I haz the power, 2016-11-14 21:34:40]

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-11-14 23:28:43]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-14 23:33:50]

Description was changed from

==========
DCHECK that paint properties are never null

This patch adds a new DCHECK that paint chunk properties are not null,
along with comments describing how to fix this situation if one is
unlucky enough to hit it. Now that we have root paint property nodes,
the properties of a paint chunk should never be null.

The asserts in this patch uncovered a bug where FrameView scrollbars
were painted with null paint property nodes. This has been fixed by
ensuring all properties, not just the transform property, are
specified in FramePainter. This fixed 18 test failures due to the new
assert.

The asserts in this patch also uncovered a case where we were using
null paint properties in SkPictureBuilder when optimizing the display
item list of the CompositingRecorder. An assert disabler has been
added because this case is safe, and comments have been added
describing why.

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

to

==========
DCHECK that paint properties are never null

This patch adds a new DCHECK that paint chunk properties are not null,
along with comments describing how to fix this situation if one is
unlucky enough to hit it. Now that we have root paint property nodes,
the properties of a paint chunk should never be null.

The asserts in this patch uncovered a bug where FrameView scrollbars
were painted with null paint property nodes. This has been fixed by
ensuring all properties, not just the transform property, are
specified in FramePainter. This fixed 18 test failures due to the new
assert.

The asserts in this patch also uncovered a case where we were using
null paint properties in SkPictureBuilder when optimizing the display
item list of the CompositingRecorder. An assert disabler has been
added because this case is safe, and comments have been added
describing why.

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

Committed: https://crrev.com/29bb3301df59c321abee8d9d226bf38e3877a5fb
Cr-Commit-Position: refs/heads/master@{#431967}
==========

[commit-bot: I haz the power, 2016-11-14 23:33:51]

Patchset 5 (id:??) landed as
https://crrev.com/29bb3301df59c321abee8d9d226bf38e3877a5fb
Cr-Commit-Position: refs/heads/master@{#431967}