Refactor the way PaintArtifactCompositor is attached and used.

Refactor the way PaintArtifactCompositor is attached and used.

The new way is simpler and uses the same pattern as PaintLayerCompositor.

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

Committed: https://crrev.com/7e410b319bf81341e65a280be187996b8cab97c7
Cr-Commit-Position: refs/heads/master@{#419389}

[chrishtr, 2016-09-15 19:19:21]

Description was changed from

==========
none


Merge branch 'removecompositing' into pacrefactor


none


none


none


none


none


none

BUG=
==========

to

==========
none


Merge branch 'removecompositing' into pacrefactor


none


none


none


none


none


none

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

[chrishtr, 2016-09-15 20:18:31]

Description was changed from

==========
none


Merge branch 'removecompositing' into pacrefactor


none


none


none


none


none


none

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

to

==========
Refactor the way PaintArtifactCompositor is attached and used.

The new way is simpler and uses the same pattern as PaintLayerCompositor.

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

[chrishtr, 2016-09-15 20:45:28]

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

[chrishtr, 2016-09-15 20:45:29]

Depends on https://codereview.chromium.org/2322073006.

[pdr., 2016-09-15 20:53:08]

Looks great! Just one question and a couple nits.

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

https://codereview.chromium.org/2342103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2769: if (!page)
Can this ever happen? If it does, wouldn't we forget to attach the root layer?

https://codereview.chromium.org/2342103002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h
(right):

https://codereview.chromium.org/2342103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h:46:

Nit: extra space

https://codereview.chromium.org/2342103002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2342103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:817: 
Nit: extra space

[chrishtr, 2016-09-15 21:05:39]

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

https://codereview.chromium.org/2342103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2769: if (!page)
On 2016/09/15 at 20:53:07, pdr. wrote:
> Can this ever happen? If it does, wouldn't we forget to attach the root layer?

I don't know if it can happen. What I can say is that there are several places
that defensively return on null page() already in this class, 
including jbroman's here (which was probably also cargo-culted).

Yes it means we won't attach, but that's ok because there is no page to attach
to anyway. :)

https://codereview.chromium.org/2342103002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h
(right):

https://codereview.chromium.org/2342103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h:46:

On 2016/09/15 at 20:53:07, pdr. wrote:
> Nit: extra space

Done.

https://codereview.chromium.org/2342103002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2342103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:817: 
On 2016/09/15 at 20:53:08, pdr. wrote:
> Nit: extra space

Done.

[pdr., 2016-09-15 21:08:17]

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

https://codereview.chromium.org/2342103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2769: if (!page)
On 2016/09/15 at 21:05:39, chrishtr wrote:
> On 2016/09/15 at 20:53:07, pdr. wrote:
> > Can this ever happen? If it does, wouldn't we forget to attach the root
layer?
> 
> I don't know if it can happen. What I can say is that there are several places
that defensively return on null page() already in this class, 
> including jbroman's here (which was probably also cargo-culted).
> 
> Yes it means we won't attach, but that's ok because there is no page to attach
to anyway. :)

Why don't we just bail then, instead of creating and updating the paint artifact
compositor?

[chrishtr, 2016-09-15 21:11:04]

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

https://codereview.chromium.org/2342103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2769: if (!page)
On 2016/09/15 at 21:08:17, pdr. wrote:
> On 2016/09/15 at 21:05:39, chrishtr wrote:
> > On 2016/09/15 at 20:53:07, pdr. wrote:
> > > Can this ever happen? If it does, wouldn't we forget to attach the root
layer?
> > 
> > I don't know if it can happen. What I can say is that there are several
places that defensively return on null page() already in this class, 
> > including jbroman's here (which was probably also cargo-culted).
> > 
> > Yes it means we won't attach, but that's ok because there is no page to
attach to anyway. :)
> 
> Why don't we just bail then, instead of creating and updating the paint
artifact compositor?

Done.

[pdr., 2016-09-15 21:11:50]

On 2016/09/15 at 21:11:04, chrishtr wrote:
>
https://codereview.chromium.org/2342103002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/2342103002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/FrameView.cpp:2769: if (!page)
> On 2016/09/15 at 21:08:17, pdr. wrote:
> > On 2016/09/15 at 21:05:39, chrishtr wrote:
> > > On 2016/09/15 at 20:53:07, pdr. wrote:
> > > > Can this ever happen? If it does, wouldn't we forget to attach the root
layer?
> > > 
> > > I don't know if it can happen. What I can say is that there are several
places that defensively return on null page() already in this class, 
> > > including jbroman's here (which was probably also cargo-culted).
> > > 
> > > Yes it means we won't attach, but that's ok because there is no page to
attach to anyway. :)
> > 
> > Why don't we just bail then, instead of creating and updating the paint
artifact compositor?
> 
> Done.

LGTM

[jbroman, 2016-09-15 21:31:03]

jbroman@chromium.org changed reviewers:
+ jbroman@chromium.org

[jbroman, 2016-09-15 21:31:04]

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

https://codereview.chromium.org/2342103002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2772:
page->chromeClient().attachRootLayer(m_paintArtifactCompositor->getWebLayer(),
&frame());
For what it's worth, the concern here was that WebLayer is on the path to
eventual deprecation and core/ is not allowed to talk to cc::Layer. That was the
reason this was done in Source/web/ (though we can cross that bridge when we get
to it).

[chrishtr, 2016-09-15 21:35:27]

On 2016/09/15 at 21:31:04, jbroman wrote:
>
https://codereview.chromium.org/2342103002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/2342103002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/FrameView.cpp:2772:
page->chromeClient().attachRootLayer(m_paintArtifactCompositor->getWebLayer(),
&frame());
> For what it's worth, the concern here was that WebLayer is on the path to
eventual deprecation and core/ is not allowed to talk to cc::Layer. That was the
reason this was done in Source/web/ (though we can cross that bridge when we get
to it).

I think I'd rather keep around a pass-through WebLayer class just for this
purpose than the other complication. Once SPv2 launches we'll be able to delete
all the other stuff on WebLayer.

[chrishtr, 2016-09-15 21:36:51]

On 2016/09/15 at 21:35:27, chrishtr wrote:
> On 2016/09/15 at 21:31:04, jbroman wrote:
> >
https://codereview.chromium.org/2342103002/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/frame/FrameView.cpp (right):
> > 
> >
https://codereview.chromium.org/2342103002/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/frame/FrameView.cpp:2772:
page->chromeClient().attachRootLayer(m_paintArtifactCompositor->getWebLayer(),
&frame());
> > For what it's worth, the concern here was that WebLayer is on the path to
eventual deprecation and core/ is not allowed to talk to cc::Layer. That was the
reason this was done in Source/web/ (though we can cross that bridge when we get
to it).
> 
> I think I'd rather keep around a pass-through WebLayer class just for this
purpose than the other complication. Once SPv2 launches we'll be able to delete
all the other stuff on WebLayer.

Not to say that I figured out the reason when I wrote this patch. :)  Thanks for
pointing it out.

[chrishtr, 2016-09-17 01:11:35]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-09-17 01:11:35]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2342103002/#ps100001
(title: "none")

[commit-bot: I haz the power, 2016-09-17 01:11:55]

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-09-17 02:13:53]

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

[commit-bot: I haz the power, 2016-09-17 02:13:54]

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

[chrishtr, 2016-09-17 19:02:53]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-09-17 19:02:53]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2342103002/#ps140001
(title: "none")

[commit-bot: I haz the power, 2016-09-17 19:03:11]

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-09-17 23:21:09]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-17 23:23:27]

Description was changed from

==========
Refactor the way PaintArtifactCompositor is attached and used.

The new way is simpler and uses the same pattern as PaintLayerCompositor.

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

to

==========
Refactor the way PaintArtifactCompositor is attached and used.

The new way is simpler and uses the same pattern as PaintLayerCompositor.

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

Committed: https://crrev.com/7e410b319bf81341e65a280be187996b8cab97c7
Cr-Commit-Position: refs/heads/master@{#419389}
==========

[commit-bot: I haz the power, 2016-09-17 23:23:28]

Patchset 8 (id:??) landed as
https://crrev.com/7e410b319bf81341e65a280be187996b8cab97c7
Cr-Commit-Position: refs/heads/master@{#419389}