Deal gracefully with null {GraphicsContext,SkPictureBuilder}.endRecording() results.

Deal gracefully with null {GraphicsContext,SkPictureBuilder}.endRecording() results.

GraphicsContext supports the notion of being "disabled". Currently this is only
used by the rasterize_and_record_micro benchmark. In this mode, both
GraphicsContext.endRecording() and SkPictureBuilder.endRecording() can
return null and our code is not currently robust against this result.

Update callsites to deal with a null picture, and add documentation to
GraphicsContext and SkPictureBuilder to make this clearer.

BUG=598358
Committed: https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673
Cr-Commit-Position: refs/heads/master@{#383971}

[wkorman, 2016-03-29 22:32:51]

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

[chrishtr, 2016-03-29 22:37:43]

Description was changed from

==========
Deal gracefully with null {GraphicsContext,SkPictureBuilder}.endRecording()
results.

GraphicsContext supports the notion of being "disabled". Currently this is only
used by the rasterize_and_record_micro benchmark. However, both
GraphicsContext.endRecording() and SkPictureBuilder.endRecording() can
potentially return null and our code is not currently robust against this
potential result.

Update callsites to deal with a null picture, and add documentation to
GraphicsContext and SkPictureBuilder to make this clearer.

BUG=598358
==========

to

==========
Deal gracefully with null {GraphicsContext,SkPictureBuilder}.endRecording()
results.

GraphicsContext supports the notion of being "disabled". Currently this is only
used by the rasterize_and_record_micro benchmark. In this mode, both
GraphicsContext.endRecording() and SkPictureBuilder.endRecording() can return
null and our code is not currently robust against this
potential result.

Update callsites to deal with a null picture, and add documentation to
GraphicsContext and SkPictureBuilder to make this clearer.

BUG=598358
==========

[chrishtr, 2016-03-29 22:42:23]

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:130: if (!recording)
Did you check call sites to be robust aginst nullptr?

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceClipper.h
(right):

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceClipper.h:50: //
Returns a picture to paint the contents of the resource, or nullptr if the
picture couldn't be created.
Add a sentence that the second scenario should only happen in testing.

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMasker.h
(right):

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMasker.h:53: //
Returns a picture to paint the contents of the mask, or nullptr if the picture
couldn't be created.
ditto

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePattern.cpp
(right):

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePattern.cpp:114:
return nullptr;
Callsites are robust against this?

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePattern.h
(right):

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePattern.h:57: //
Returns a picture to paint the pattern, or nullptr if the picture couldn't be
created.
ditto

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/graphics/filters/SVGFEImage.cpp (right):

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/filters/SVGFEImage.cpp:182: return
nullptr;
ditto

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/exported/WebScrollbarThemePainter.cpp
(right):

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/exported/WebScrollbarThemePainter.cpp:50:
static void playbackIfNeeded(SkPictureBuilder* builder, WebCanvas* canvas)
the "IfNeeded" suffix is now deprecated :)

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/GraphicsContext.h:215: // or nullptr
if the canvas is disabled.
add the second sentence

[wkorman, 2016-03-29 22:52:02]

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:130: if (!recording)
On 2016/03/29 at 22:42:23, chrishtr wrote:
> Did you check call sites to be robust aginst nullptr?

Yes.

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceClipper.h
(right):

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceClipper.h:50: //
Returns a picture to paint the contents of the resource, or nullptr if the
picture couldn't be created.
On 2016/03/29 at 22:42:23, chrishtr wrote:
> Add a sentence that the second scenario should only happen in testing.

Done.

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePattern.cpp
(right):

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePattern.cpp:114:
return nullptr;
On 2016/03/29 at 22:42:23, chrishtr wrote:
> Callsites are robust against this?

Yes.

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/graphics/filters/SVGFEImage.cpp (right):

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/filters/SVGFEImage.cpp:182: return
nullptr;
On 2016/03/29 at 22:42:23, chrishtr wrote:
> ditto

Yes.

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/exported/WebScrollbarThemePainter.cpp
(right):

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/exported/WebScrollbarThemePainter.cpp:50:
static void playbackIfNeeded(SkPictureBuilder* builder, WebCanvas* canvas)
On 2016/03/29 at 22:42:23, chrishtr wrote:
> the "IfNeeded" suffix is now deprecated :)

Removed suffix.

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/1841833002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/GraphicsContext.h:215: // or nullptr
if the canvas is disabled.
On 2016/03/29 at 22:42:23, chrishtr wrote:
> add the second sentence

Done, more specific phrasing here since this is the source of it all, as it
were.

[wkorman, 2016-03-29 22:52:24]

Description was changed from

==========
Deal gracefully with null {GraphicsContext,SkPictureBuilder}.endRecording()
results.

GraphicsContext supports the notion of being "disabled". Currently this is only
used by the rasterize_and_record_micro benchmark. In this mode, both
GraphicsContext.endRecording() and SkPictureBuilder.endRecording() can return
null and our code is not currently robust against this
potential result.

Update callsites to deal with a null picture, and add documentation to
GraphicsContext and SkPictureBuilder to make this clearer.

BUG=598358
==========

to

==========
Deal gracefully with null {GraphicsContext,SkPictureBuilder}.endRecording()
results.

GraphicsContext supports the notion of being "disabled". Currently this is only
used by the rasterize_and_record_micro benchmark. In this mode, both
GraphicsContext.endRecording() and SkPictureBuilder.endRecording() can
return null and our code is not currently robust against this result.

Update callsites to deal with a null picture, and add documentation to
GraphicsContext and SkPictureBuilder to make this clearer.

BUG=598358
==========

[chrishtr, 2016-03-29 22:53:53]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-03-29 22:53:53]

lgtm

[commit-bot: I haz the power, 2016-03-29 22:54:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841833002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841833002/20001

[commit-bot: I haz the power, 2016-03-29 23:26:33]

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

[commit-bot: I haz the power, 2016-03-29 23:26:35]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wkorman, 2016-03-30 00:02:12]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2016-03-30 00:02:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1841833002/#ps40001
(title: "Don't end recording twice in DrawingRecorder.")

[commit-bot: I haz the power, 2016-03-30 00:02:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841833002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841833002/40001

[f(malita), 2016-03-30 01:18:51]

Description was changed from

==========
Deal gracefully with null {GraphicsContext,SkPictureBuilder}.endRecording()
results.

GraphicsContext supports the notion of being "disabled". Currently this is only
used by the rasterize_and_record_micro benchmark. In this mode, both
GraphicsContext.endRecording() and SkPictureBuilder.endRecording() can
return null and our code is not currently robust against this result.

Update callsites to deal with a null picture, and add documentation to
GraphicsContext and SkPictureBuilder to make this clearer.

BUG=598358
==========

to

==========
Deal gracefully with null {GraphicsContext,SkPictureBuilder}.endRecording()
results.

GraphicsContext supports the notion of being "disabled". Currently this is only
used by the rasterize_and_record_micro benchmark. In this mode, both
GraphicsContext.endRecording() and SkPictureBuilder.endRecording() can
return null and our code is not currently robust against this result.

Update callsites to deal with a null picture, and add documentation to
GraphicsContext and SkPictureBuilder to make this clearer.

BUG=598358
==========

[f(malita), 2016-03-30 01:27:34]

It feels a bit odd to relax the non-null recording invariant in order to support
test-only configs.  Would it make more sense to return a non-null (empty)
picture when the context is disabled?

Or, better yet, can we dump the disabled GC config (I'm not sure how useful it
is at this stage - Chris/Stephen/Philip can comment on that)?

[wkorman, 2016-03-30 04:00:25]

On 2016/03/30 at 01:27:34, fmalita wrote:
> It feels a bit odd to relax the non-null recording invariant in order to
support test-only configs.  Would it make more sense to return a non-null
(empty) picture when the context is disabled?
> 
> Or, better yet, can we dump the disabled GC config (I'm not sure how useful it
is at this stage - Chris/Stephen/Philip can comment on that)?

Ah, that would be cool. This is about to land but I'm happy to pursue follow-up
on whatever folks think. I just assumed the existing logic was intentional for
better metrics purposes.

To be clear this change itself does not relax any invariant -- endRecording()
already potentially returned null. This change just documents existing behavior,
and fixes latent bugs waiting to bite us.

[commit-bot: I haz the power, 2016-03-30 06:23:03]

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

[commit-bot: I haz the power, 2016-03-30 06:23:05]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[f(malita), 2016-03-30 12:44:24]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2016-03-30 12:44:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841833002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841833002/40001

[commit-bot: I haz the power, 2016-03-30 13:58:24]

Description was changed from

==========
Deal gracefully with null {GraphicsContext,SkPictureBuilder}.endRecording()
results.

GraphicsContext supports the notion of being "disabled". Currently this is only
used by the rasterize_and_record_micro benchmark. In this mode, both
GraphicsContext.endRecording() and SkPictureBuilder.endRecording() can
return null and our code is not currently robust against this result.

Update callsites to deal with a null picture, and add documentation to
GraphicsContext and SkPictureBuilder to make this clearer.

BUG=598358
==========

to

==========
Deal gracefully with null {GraphicsContext,SkPictureBuilder}.endRecording()
results.

GraphicsContext supports the notion of being "disabled". Currently this is only
used by the rasterize_and_record_micro benchmark. In this mode, both
GraphicsContext.endRecording() and SkPictureBuilder.endRecording() can
return null and our code is not currently robust against this result.

Update callsites to deal with a null picture, and add documentation to
GraphicsContext and SkPictureBuilder to make this clearer.

BUG=598358
==========

[commit-bot: I haz the power, 2016-03-30 13:58:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-30 13:59:24]

Description was changed from

==========
Deal gracefully with null {GraphicsContext,SkPictureBuilder}.endRecording()
results.

GraphicsContext supports the notion of being "disabled". Currently this is only
used by the rasterize_and_record_micro benchmark. In this mode, both
GraphicsContext.endRecording() and SkPictureBuilder.endRecording() can
return null and our code is not currently robust against this result.

Update callsites to deal with a null picture, and add documentation to
GraphicsContext and SkPictureBuilder to make this clearer.

BUG=598358
==========

to

==========
Deal gracefully with null {GraphicsContext,SkPictureBuilder}.endRecording()
results.

GraphicsContext supports the notion of being "disabled". Currently this is only
used by the rasterize_and_record_micro benchmark. In this mode, both
GraphicsContext.endRecording() and SkPictureBuilder.endRecording() can
return null and our code is not currently robust against this result.

Update callsites to deal with a null picture, and add documentation to
GraphicsContext and SkPictureBuilder to make this clearer.

BUG=598358

Committed: https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673
Cr-Commit-Position: refs/heads/master@{#383971}
==========

[commit-bot: I haz the power, 2016-03-30 13:59:26]

Patchset 3 (id:??) landed as
https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673
Cr-Commit-Position: refs/heads/master@{#383971}

[caseq, 2016-04-04 21:38:32]

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

[caseq, 2016-04-04 21:38:33]

https://codereview.chromium.org/1841833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorLayerTreeAgent.cpp
(right):

https://codereview.chromium.org/1841833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorLayerTreeAgent.cpp:340: if
(!recording)
This actually needs to set errorString to something to indicate the error (I
realize bail-out at the start of the function may be confusing, but it actually
sets errorString within layerById())

[wkorman, 2016-04-04 22:20:05]

https://codereview.chromium.org/1841833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorLayerTreeAgent.cpp
(right):

https://codereview.chromium.org/1841833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorLayerTreeAgent.cpp:340: if
(!recording)
On 2016/04/04 at 21:38:33, caseq wrote:
> This actually needs to set errorString to something to indicate the error (I
realize bail-out at the start of the function may be confusing, but it actually
sets errorString within layerById())

Ah, thanks for catching. I think we are ok because shortly after this change
fmalita@ landed http://crrev.com/1849433002 which moved this back to previous
code.

[caseq, 2016-04-04 23:16:39]

On 2016/04/04 22:20:05, wkorman wrote:
>
https://codereview.chromium.org/1841833002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/inspector/InspectorLayerTreeAgent.cpp
> (right):
> 
>
https://codereview.chromium.org/1841833002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/inspector/InspectorLayerTreeAgent.cpp:340: if
> (!recording)
> On 2016/04/04 at 21:38:33, caseq wrote:
> > This actually needs to set errorString to something to indicate the error (I
> realize bail-out at the start of the function may be confusing, but it
actually
> sets errorString within layerById())
> 
> Ah, thanks for catching. I think we are ok because shortly after this change
> fmalita@ landed http://crrev.com/1849433002 which moved this back to previous
> code.

Yup. I only noticed Florin's change later as I was digging through my mail after
vacation.