[SPv2] FrameView::synchronizedPaint should apply root property nodes

[SPv2] FrameView::synchronizedPaint should apply root property nodes

The root property nodes weren't applied because we expect the PaintLayer of
the root LayoutView to apply them before painting. However in debug builds
the GraphicsLayer could paint debug background before we ever reach the
PaintLayer. Also the scrollbar layers (not implemented in SPv2. Upcoming.)
are not backed by PaintLayer.

Committed: https://crrev.com/ee0ddb682af672b075afdd36d563c7ade135c7dd
Cr-Commit-Position: refs/heads/master@{#405080}

[trchen, 2016-07-12 22:36:44]

Patchset #1 (id:1) has been deleted

[trchen, 2016-07-12 22:36:49]

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

[trchen, 2016-07-12 22:36:57]

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

[commit-bot: I haz the power, 2016-07-12 22:40:01]

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

[pdr., 2016-07-12 23:09:29]

Looks pretty good! Do you need to call setRootEffect in
PaintPropertyTreeBuilder::buildTreeRootNodes?

Is the debug red fill relevant here? Can it have effects?

It looks like we don't support effects on scrollbars, but webkit does:
http://jsbin.com/koqamik, so this seems useful for fixing that.

[trchen, 2016-07-12 23:31:05]

On 2016/07/12 23:09:29, pdr. wrote:
> Looks pretty good! Do you need to call setRootEffect in
> PaintPropertyTreeBuilder::buildTreeRootNodes?

Whoops! I forgot to git add. (Insert shame cube meme here.) One second.

> Is the debug red fill relevant here? Can it have effects?
>
> It looks like we don't support effects on scrollbars, but webkit does:
> http://jsbin.com/koqamik, so this seems useful for fixing that.

They can't have effect, and in fact the root effect node is a no-op. It is
debated before whether we want to use nullptr to represent the root render
surface, and the conclusion was to create a root effect node and made nullptr an
invalid value. I added a DCHECK in my PAC opacity CL that caught this.

[trchen, 2016-07-13 00:21:09]

On 2016/07/12 23:31:05, trchen wrote:
> On 2016/07/12 23:09:29, pdr. wrote:
> > Looks pretty good! Do you need to call setRootEffect in
> > PaintPropertyTreeBuilder::buildTreeRootNodes?
> 
> Whoops! I forgot to git add. (Insert shame cube meme here.) One second.

Nope, I realized I made a terrible mistake. The red background wasn't painted
but FramePainter, but GraphicsLayer::paint. We should have initialized the
PaintController with the root property nodes in the beginning of
FrameView::synchronizedPaint(). Will take a few minutes to verify things work as
expected.

[commit-bot: I haz the power, 2016-07-13 01:37:34]

The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 01:37:35]

Dry run: This issue passed the CQ dry run.

[trchen, 2016-07-13 01:59:43]

Description was changed from

==========
[SPv2] FramePainter should apply root effect node

The root effect node wasn't applied because we didn't expect FrameView to
create any display items, but it could actually paint debug backgrounds in
debug builds, and also scrollbars (not implemented, upcoming) in non-RLS mode.
==========

to

==========
[SPv2] FrameView::synchronizedPaint should apply root property nodes
    
The root property nodes weren't applied because we expect the PaintLayer of
the root LayoutView to apply them before painting. However in debug builds
the GraphicsLayer could paint debug background before we ever reach the
PaintLayer. Also the scrollbar layers (not implemented in SPv2. Upcoming.)
are not backed by PaintLayer.
==========

[trchen, 2016-07-13 01:59:54]

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

[trchen, 2016-07-13 02:00:08]

Revised, and changed the CL desc accordingly.

[commit-bot: I haz the power, 2016-07-13 02:03:02]

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

[pdr., 2016-07-13 02:26:51]

I like this approach. A few small comments.

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

https://codereview.chromium.org/2140173004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2638: // Usually this is not
needed because the PaintLayer will setup the chunk properties
Can you wrap this in:
#ifndef NDEBUG

...

#endif

https://codereview.chromium.org/2140173004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2641: if
(RuntimeEnabledFeatures::slimmingPaintV2Enabled()) {
Instead of special-casing this, is it possible to trigger a full update by
calling updatePaintProperties();?

https://codereview.chromium.org/2140173004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):

https://codereview.chromium.org/2140173004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:147:
EXPECT_EQ(TransformationMatrix(), frameView->preTranslation()->matrix());
Which test caused the original test to be rolled out? Can you confirm that it
passes with both patches applied?

https://codereview.chromium.org/2140173004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:155:
EXPECT_EQ(nullptr, frameView->rootClip()->parent());
Add a basic test for effect too?

[trchen, 2016-07-13 02:50:39]

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

https://codereview.chromium.org/2140173004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2638: // Usually this is not
needed because the PaintLayer will setup the chunk properties
I'm thinking non-RLS scrollbars will need this too.

https://codereview.chromium.org/2140173004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2641: if
(RuntimeEnabledFeatures::slimmingPaintV2Enabled()) {
On 2016/07/13 02:26:51, pdr. wrote:
> Instead of special-casing this, is it possible to trigger a full update by
> calling updatePaintProperties();?

We always have up-to-date property tree at this point so calling
updatePaintProperties() will be no-op.

This ensures none of the paint chunk property can be nullptr (which really meant
the root node).

https://codereview.chromium.org/2140173004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):

https://codereview.chromium.org/2140173004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:147:
EXPECT_EQ(TransformationMatrix(), frameView->preTranslation()->matrix());
On 2016/07/13 02:26:51, pdr. wrote:
> Which test caused the original test to be rolled out? Can you confirm that it
> passes with both patches applied?

HTMLCanvasPainterTestForSPv2.Canvas2DLayerAppearsInLayerTree
VideoPainterTestForSPv2.VideoLayerAppearsInLayerTree

Yes, verified this time.

https://codereview.chromium.org/2140173004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:155:
EXPECT_EQ(nullptr, frameView->rootClip()->parent());
On 2016/07/13 02:26:51, pdr. wrote:
> Add a basic test for effect too?

Root effect was already unconditionally created. This CL didn't change effect
tree. (Pretty sure there are unit tests that verifies effect hierarchy.)

[pdr., 2016-07-13 03:00:46]

Cool, LGTM

[commit-bot: I haz the power, 2016-07-13 04:22:51]

The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 04:22:52]

Dry run: This issue passed the CQ dry run.

[trchen, 2016-07-13 08:22:55]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2016-07-13 08:23:17]

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-07-13 08:28:55]

Description was changed from

==========
[SPv2] FrameView::synchronizedPaint should apply root property nodes
    
The root property nodes weren't applied because we expect the PaintLayer of
the root LayoutView to apply them before painting. However in debug builds
the GraphicsLayer could paint debug background before we ever reach the
PaintLayer. Also the scrollbar layers (not implemented in SPv2. Upcoming.)
are not backed by PaintLayer.
==========

to

==========
[SPv2] FrameView::synchronizedPaint should apply root property nodes
    
The root property nodes weren't applied because we expect the PaintLayer of
the root LayoutView to apply them before painting. However in debug builds
the GraphicsLayer could paint debug background before we ever reach the
PaintLayer. Also the scrollbar layers (not implemented in SPv2. Upcoming.)
are not backed by PaintLayer.
==========

[commit-bot: I haz the power, 2016-07-13 08:28:56]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-07-13 08:30:27]

Description was changed from

==========
[SPv2] FrameView::synchronizedPaint should apply root property nodes
    
The root property nodes weren't applied because we expect the PaintLayer of
the root LayoutView to apply them before painting. However in debug builds
the GraphicsLayer could paint debug background before we ever reach the
PaintLayer. Also the scrollbar layers (not implemented in SPv2. Upcoming.)
are not backed by PaintLayer.
==========

to

==========
[SPv2] FrameView::synchronizedPaint should apply root property nodes

The root property nodes weren't applied because we expect the PaintLayer of
the root LayoutView to apply them before painting. However in debug builds
the GraphicsLayer could paint debug background before we ever reach the
PaintLayer. Also the scrollbar layers (not implemented in SPv2. Upcoming.)
are not backed by PaintLayer.

Committed: https://crrev.com/ee0ddb682af672b075afdd36d563c7ade135c7dd
Cr-Commit-Position: refs/heads/master@{#405080}
==========

[commit-bot: I haz the power, 2016-07-13 08:30:29]

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