Remove begin/end methods on CompositingRecorder.

Remove begin/end methods on CompositingRecorder.

These methods complicate the API of the recorder and are really an
implementation detail of how the recorder is being used. So move that
detail out to using Optional<CompositingRecorder>s to end the
recorder independently from the stack frame.

R=pdr@chromium.org
BUG=671439
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[danakj, 2017-03-06 19:30:48]

Description was changed from

==========
Remove begin/end methods on CompositingRecorder.

These methods complicate the API of the recorder and are really an
implementation detail of how the recorder is being used. So move that
detail out to using Optional<CompositingRecorder>s to end the
recorder independently from the stack frame.

R=pdr@chromium.org
BUG=671439
==========

to

==========
Remove begin/end methods on CompositingRecorder.

These methods complicate the API of the recorder and are really an
implementation detail of how the recorder is being used. So move that
detail out to using Optional<CompositingRecorder>s to end the
recorder independently from the stack frame.

R=pdr@chromium.org
BUG=671439
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[danakj, 2017-03-06 19:33:14]

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

[danakj, 2017-03-06 19:33:17]

added a TODO(pdr) on the defns

[commit-bot: I haz the power, 2017-03-06 19:34:24]

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

[pdr., 2017-03-06 19:52:16]

LGTM

[danakj, 2017-03-06 19:53:54]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2017-03-06 19:53:57]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2017-03-06 19:54: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-03-06 21:14:17]

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

[commit-bot: I haz the power, 2017-03-06 21:14: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_...)

[danakj, 2017-03-06 22:51:31]

Description was changed from

==========
Remove begin/end methods on CompositingRecorder.

These methods complicate the API of the recorder and are really an
implementation detail of how the recorder is being used. So move that
detail out to using Optional<CompositingRecorder>s to end the
recorder independently from the stack frame.

R=pdr@chromium.org
BUG=671439
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Remove begin/end methods on CompositingRecorder.

These methods complicate the API of the recorder and are really an
implementation detail of how the recorder is being used. So move that
detail out to using Optional<CompositingRecorder>s to end the
recorder independently from the stack frame.

R=pdr@chromium.org
BUG=671439
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[danakj, 2017-03-06 22:52:51]

Huh cool, bunch of SVG tests broke somehow.

[danakj, 2017-03-06 23:09:54]

It looks like SVGClipPainter does not live long enough to keep the
CompositingRecorder alive. A different SVGClipPainter is used for
prepareEffect() than for finishEffect(), which is.. surprising.

[danakj, 2017-03-06 23:13:39]

On 2017/03/06 23:09:54, danakj wrote:
> It looks like SVGClipPainter does not live long enough to keep the
> CompositingRecorder alive. A different SVGClipPainter is used for
> prepareEffect() than for finishEffect(), which is.. surprising.

Specifically:

- ClipPathClipper::ClipPathClipper() makes a SVGClipPainter(*clipper) temporary
to call prepareEffect() on. It seems more like that should be a static method
there.

- SVGClipPainter constructor takes a thing, which it stores as a member. But
since the object is not long lived, it acts like a parameter to the
prepareEffect() method really.

- ClipPathClipper::~ClipPathClipper() makes another temporary
SVGClipPainter(*m_resourceClipper) to call finishEffect() on.

- m_resourceClipper was set to clipper in the constructor, so the member is
longer-lived in the ClipPathClipper tho gives the appearance of being
longer-lived in both classes.

[danakj, 2017-03-07 00:00:50]

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

[commit-bot: I haz the power, 2017-03-07 00:01:26]

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

[danakj, 2017-03-07 00:05:05]

PTAL.

I've collapsed the SVGClipPainter into ClipPathClipper. There is one behaviour
change here, and one I'm not sure if I should make. Questions inline.

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

https://codereview.chromium.org/2738493002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ClipPathClipper.cpp:120:
SVGClipExpansionCycleHelper inClipExpansionChange(*m_resourceClipper);
The SVGClipPainter used to hold one of these for it's lifetime, so it would be
alive for the code above here in prepareEffect, and we'd have to active below
here. Is it right to leave this one here? Should I add another at the top? Or
move this to the top?

https://codereview.chromium.org/2738493002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ClipPathClipper.cpp:220:
SVGClipExpansionCycleHelper inClipExpansionChange(*m_resourceClipper);
This preserves the behaviour of the SVGClipPainter holding one of these while it
is alive. But I'm not sure that this method needs it, since it didn't have one
internally but the beginEffect did, which raises flags for me. Should this be
here or should we remove it?

[commit-bot: I haz the power, 2017-03-07 02:28:30]

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

[commit-bot: I haz the power, 2017-03-07 02:28:31]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[pdr., 2017-03-07 02:29:03]

LGTM

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

https://codereview.chromium.org/2738493002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ClipPathClipper.cpp:120:
SVGClipExpansionCycleHelper inClipExpansionChange(*m_resourceClipper);
On 2017/03/07 at 00:05:05, danakj wrote:
> The SVGClipPainter used to hold one of these for it's lifetime, so it would be
alive for the code above here in prepareEffect, and we'd have to active below
here. Is it right to leave this one here? Should I add another at the top? Or
move this to the top?

I traced this back to when it was originally added
(https://codereview.chromium.org/23643003/) and the code is really just trying
to detect cycles at the start of the clip. I think we should leave this user of
SVGClipExpansionCycleHelper but remove the one in finishEffect.

[danakj, 2017-03-07 15:42:26]

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

https://codereview.chromium.org/2738493002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ClipPathClipper.cpp:120:
SVGClipExpansionCycleHelper inClipExpansionChange(*m_resourceClipper);
On 2017/03/07 02:29:03, pdr. wrote:
> On 2017/03/07 at 00:05:05, danakj wrote:
> > The SVGClipPainter used to hold one of these for it's lifetime, so it would
be
> alive for the code above here in prepareEffect, and we'd have to active below
> here. Is it right to leave this one here? Should I add another at the top? Or
> move this to the top?
> 
> I traced this back to when it was originally added
> (https://codereview.chromium.org/23643003/) and the code is really just trying
> to detect cycles at the start of the clip. I think we should leave this user
of
> SVGClipExpansionCycleHelper but remove the one in finishEffect.

Thanks, done.

[danakj, 2017-03-07 15:42:31]

The CQ bit was checked by danakj@chromium.org

[danakj, 2017-03-07 15:42:31]

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/2738493002/#ps60001
(title: "rm-compositingrecorder-methods: remove-finish")

[commit-bot: I haz the power, 2017-03-07 15:43:34]

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

[danakj, 2017-03-07 15:45:46]

The CQ bit was checked by danakj@chromium.org

[danakj, 2017-03-07 15:45:47]

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/2738493002/#ps80001
(title: "rm-compositingrecorder-methods: replace-default")

[commit-bot: I haz the power, 2017-03-07 15:46:18]

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

[danakj, 2017-03-07 15:46:57]

https://codereview.chromium.org/2738493002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/ClipPathClipper.cpp (right):

https://codereview.chromium.org/2738493002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ClipPathClipper.cpp:234: case
ClipperState::NotApplied:
I made one little change here since I noticed it. I replaced default: with a
NotApplied case. This way if another state were added sometime, the compiler
would point out that this code needs to consider how to handle it.

[pdr., 2017-03-07 17:34:37]

On 2017/03/07 at 15:46:57, danakj wrote:
>
https://codereview.chromium.org/2738493002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/ClipPathClipper.cpp (right):
> 
>
https://codereview.chromium.org/2738493002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/ClipPathClipper.cpp:234: case
ClipperState::NotApplied:
> I made one little change here since I noticed it. I replaced default: with a
NotApplied case. This way if another state were added sometime, the compiler
would point out that this code needs to consider how to handle it.

SGTM thanks!

[commit-bot: I haz the power, 2017-03-07 18:27:18]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488901546815610,
"parent_rev": "efcd30ca164e0e1bbcfe402840d8c23aa9d40fd9", "commit_rev":
"c3d234f50f3d487796c1c859ca7d5636e2932d91"}

[commit-bot: I haz the power, 2017-03-07 18:28:01]

Description was changed from

==========
Remove begin/end methods on CompositingRecorder.

These methods complicate the API of the recorder and are really an
implementation detail of how the recorder is being used. So move that
detail out to using Optional<CompositingRecorder>s to end the
recorder independently from the stack frame.

R=pdr@chromium.org
BUG=671439
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Remove begin/end methods on CompositingRecorder.

These methods complicate the API of the recorder and are really an
implementation detail of how the recorder is being used. So move that
detail out to using Optional<CompositingRecorder>s to end the
recorder independently from the stack frame.

R=pdr@chromium.org
BUG=671439
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[commit-bot: I haz the power, 2017-03-07 18:28:02]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c3d234f50f3d487796c1c859ca7d...