Remove unneeded abstractions and simplify RecordingImageBufferSurface

Remove unneeded abstractions and simplify RecordingImageBufferSurface

This is a set of misc cleanups I found while rearranging code around
RecordingImageBufferSurface in order to remove
PaintCanvas::writePixels.

- Replaces WrapUnique with MakeUnique.
- Removes the unneeded factory abstraction, since all instances of it
make an UnacceleratedImageBufferSurface.
- Removes unneeded friend relationship from RecordingImageBufferSurface
to its unit tests.
- Change tests to check IsRecording() instead of peeking internals or
checking if the factory was used. IsRecording() is true if we did not
fall back to UnacceleratedImageBufferSurface and false otherwise,
which is what the tests want to know.
- Use IntRect::Contains instead of writing that with comparisons.
- Remove "ForTesting" from a class name defined in tests, its also
redundant with the Fake prefix that is already present.
- Some capitalization/punctuation for comments.

R=pdr@chromium.org, xlai@chromium.org
BUG=671433
Review-Url: https://codereview.chromium.org/2833593002
Cr-Commit-Position: refs/heads/master@{#466397}
Committed: https://chromium.googlesource.com/chromium/src/+/a07bd8034e0ebf854efb8cd80eb7d543d7b8b228

[danakj, 2017-04-19 21:06:36]

Description was changed from

==========
Remove unneeded abstractions and simplify RecordingImageBufferSurface

This is a set of misc cleanups I found while rearranging code around
RecordingImageBufferSurface in order to remove
PaintCanvas::writePixels.

- Replaces WrapUnique with MakeUnique in the related unit tests.
- Removes the unneeded factory abstraction, since all instances of it
make an UnacceleratedImageBufferSurface.
- Removes unneeded friend relationship from RecordingImageBufferSurface
to its unit tests.
- Change tests to check IsRecording() instead of peeking internals or
checking if the factory was used. IsRecording() is true if we did not
fall back to UnacceleratedImageBufferSurface and false otherwise,
which is what the tests want to know.

R=pdr@chromium.org, xlai@chromium.org
BUG=671433
==========

to

==========
Remove unneeded abstractions and simplify RecordingImageBufferSurface

This is a set of misc cleanups I found while rearranging code around
RecordingImageBufferSurface in order to remove
PaintCanvas::writePixels.

- Replaces WrapUnique with MakeUnique in the related unit tests.
- Removes the unneeded factory abstraction, since all instances of it
make an UnacceleratedImageBufferSurface.
- Removes unneeded friend relationship from RecordingImageBufferSurface
to its unit tests.
- Change tests to check IsRecording() instead of peeking internals or
checking if the factory was used. IsRecording() is true if we did not
fall back to UnacceleratedImageBufferSurface and false otherwise,
which is what the tests want to know.
- Use IntRect::Contains instead of writing that with comparisons.

R=pdr@chromium.org, xlai@chromium.org
BUG=671433
==========

[danakj, 2017-04-19 21:08:30]

Description was changed from

==========
Remove unneeded abstractions and simplify RecordingImageBufferSurface

This is a set of misc cleanups I found while rearranging code around
RecordingImageBufferSurface in order to remove
PaintCanvas::writePixels.

- Replaces WrapUnique with MakeUnique in the related unit tests.
- Removes the unneeded factory abstraction, since all instances of it
make an UnacceleratedImageBufferSurface.
- Removes unneeded friend relationship from RecordingImageBufferSurface
to its unit tests.
- Change tests to check IsRecording() instead of peeking internals or
checking if the factory was used. IsRecording() is true if we did not
fall back to UnacceleratedImageBufferSurface and false otherwise,
which is what the tests want to know.
- Use IntRect::Contains instead of writing that with comparisons.

R=pdr@chromium.org, xlai@chromium.org
BUG=671433
==========

to

==========
Remove unneeded abstractions and simplify RecordingImageBufferSurface

This is a set of misc cleanups I found while rearranging code around
RecordingImageBufferSurface in order to remove
PaintCanvas::writePixels.

- Replaces WrapUnique with MakeUnique.
- Removes the unneeded factory abstraction, since all instances of it
make an UnacceleratedImageBufferSurface.
- Removes unneeded friend relationship from RecordingImageBufferSurface
to its unit tests.
- Change tests to check IsRecording() instead of peeking internals or
checking if the factory was used. IsRecording() is true if we did not
fall back to UnacceleratedImageBufferSurface and false otherwise,
which is what the tests want to know.
- Use IntRect::Contains instead of writing that with comparisons.
- Remove "ForTesting" from a class name defined in tests, its also
redundant with the Fake prefix that is already present.

R=pdr@chromium.org, xlai@chromium.org
BUG=671433
==========

[danakj, 2017-04-19 21:15:44]

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

[commit-bot: I haz the power, 2017-04-19 21:16: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-04-19 21:16:32]

Description was changed from

==========
Remove unneeded abstractions and simplify RecordingImageBufferSurface

This is a set of misc cleanups I found while rearranging code around
RecordingImageBufferSurface in order to remove
PaintCanvas::writePixels.

- Replaces WrapUnique with MakeUnique.
- Removes the unneeded factory abstraction, since all instances of it
make an UnacceleratedImageBufferSurface.
- Removes unneeded friend relationship from RecordingImageBufferSurface
to its unit tests.
- Change tests to check IsRecording() instead of peeking internals or
checking if the factory was used. IsRecording() is true if we did not
fall back to UnacceleratedImageBufferSurface and false otherwise,
which is what the tests want to know.
- Use IntRect::Contains instead of writing that with comparisons.
- Remove "ForTesting" from a class name defined in tests, its also
redundant with the Fake prefix that is already present.

R=pdr@chromium.org, xlai@chromium.org
BUG=671433
==========

to

==========
Remove unneeded abstractions and simplify RecordingImageBufferSurface

This is a set of misc cleanups I found while rearranging code around
RecordingImageBufferSurface in order to remove
PaintCanvas::writePixels.

- Replaces WrapUnique with MakeUnique.
- Removes the unneeded factory abstraction, since all instances of it
make an UnacceleratedImageBufferSurface.
- Removes unneeded friend relationship from RecordingImageBufferSurface
to its unit tests.
- Change tests to check IsRecording() instead of peeking internals or
checking if the factory was used. IsRecording() is true if we did not
fall back to UnacceleratedImageBufferSurface and false otherwise,
which is what the tests want to know.
- Use IntRect::Contains instead of writing that with comparisons.
- Remove "ForTesting" from a class name defined in tests, its also
redundant with the Fake prefix that is already present.
- Some capitalization/punctuation for comments.

R=pdr@chromium.org, xlai@chromium.org
BUG=671433
==========

[danakj, 2017-04-19 21:16:36]

xlai: please review
pdr: owners stamps

https://codereview.chromium.org/2833593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp (left):

https://codereview.chromium.org/2833593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp:94: new
RecordingImageBufferSurface(size, nullptr /* fallbackFactory */,
This nullptr would have triggered a DCHECK and crash if fallback happened, and
would prevent fallback due to kExpensiveRecordingStackDepth. Is this needed, cuz
I could pass a bool here to do what the nullptr used to if you think it is?

https://codereview.chromium.org/2833593002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurfaceTest.cpp
(left):

https://codereview.chromium.org/2833593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurfaceTest.cpp:63:
test_surface_->InitializeCurrentFrame();
This is already done in the RecordingImageBufferSurface constructor.

[danakj, 2017-04-19 21:17:17]

danakj@chromium.org changed reviewers:
+ ikilpatrick@chromium.org

[danakj, 2017-04-19 21:17:17]

+ikilpatrick for modules/csspaint

[danakj, 2017-04-19 21:17:47]

danakj@chromium.org changed reviewers:
+ senorblanco@chromium.org

[danakj, 2017-04-19 21:17:48]

+senorblanco for modules/canvas2d

[commit-bot: I haz the power, 2017-04-19 21:26:29]

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

[commit-bot: I haz the power, 2017-04-19 21:26:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[danakj, 2017-04-19 21:33:45]

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

[commit-bot: I haz the power, 2017-04-19 21:34:12]

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

[Stephen White, 2017-04-19 21:39:46]

LGTM

[commit-bot: I haz the power, 2017-04-19 21:56:01]

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

[commit-bot: I haz the power, 2017-04-19 21:56:03]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[pdr., 2017-04-19 23:28:31]

Overall this looks good (+1 to no ForTesting in *Test.cpp) but I think there's a
real android compile failure.

[xlai (Olivia), 2017-04-20 14:20:09]

lgtm provided that Android compile is fixed

[danakj, 2017-04-20 22:56:38]

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

[commit-bot: I haz the power, 2017-04-20 22:57:04]

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

[danakj, 2017-04-20 22:57:13]

ikilpatrick: ping. should i find a higher level reviewer?

I think the android thing was just TOT at the time. Trying the bots again.

[commit-bot: I haz the power, 2017-04-20 22:59:42]

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

[commit-bot: I haz the power, 2017-04-20 22:59:43]

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

[ikilpatrick, 2017-04-21 04:45:45]

On 2017/04/20 22:57:13, danakj wrote:
> ikilpatrick: ping. should i find a higher level reviewer?
> 
> I think the android thing was just TOT at the time. Trying the bots again.

lgtm, however just one question related to csspaint:

 In the case of the kExpensiveRecordingStackDepth fallback - is useful/important
to fallback even if the RecordingImageBufferSurface is just being used for one
frame?
(The csspaint api has a new PRC2D each frame).

[danakj, 2017-04-21 14:52:11]

On 2017/04/21 04:45:45, ikilpatrick wrote:
> On 2017/04/20 22:57:13, danakj wrote:
> > ikilpatrick: ping. should i find a higher level reviewer?
> > 
> > I think the android thing was just TOT at the time. Trying the bots again.
> 
> lgtm, however just one question related to csspaint:
> 
>  In the case of the kExpensiveRecordingStackDepth fallback - is
useful/important
> to fallback even if the RecordingImageBufferSurface is just being used for one
> frame?
> (The csspaint api has a new PRC2D each frame).

My understanding is no. The point of the fallback is to flatten the recording so
its not very expensive to play back over and over. Thanks!

[danakj, 2017-04-21 15:38:13]

Oh I see the error, I was looking at something on one bot that wasn't me. I need
to upcast the return type to match with gcc.

[danakj, 2017-04-21 15:38:22]

The CQ bit was checked by danakj@chromium.org

[danakj, 2017-04-21 15:38:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
ikilpatrick@chromium.org, xlai@chromium.org, senorblanco@chromium.org
Link to the patchset: https://codereview.chromium.org/2833593002/#ps80001
(title: "imagebuffersurface-cleanups: fix-compile")

[commit-bot: I haz the power, 2017-04-21 15:38:39]

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-04-21 16:08:09]

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

[commit-bot: I haz the power, 2017-04-21 16:08:10]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[danakj, 2017-04-21 17:02:52]

The CQ bit was checked by danakj@chromium.org

[danakj, 2017-04-21 17:02:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
ikilpatrick@chromium.org, xlai@chromium.org, senorblanco@chromium.org
Link to the patchset: https://codereview.chromium.org/2833593002/#ps100001
(title: "imagebuffersurface-cleanups: morefixcompile")

[commit-bot: I haz the power, 2017-04-21 17:03:20]

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-04-21 18:36:07]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492794172784030,
"parent_rev": "2a99e9e1b32d0218d1e13a88bb6a7db80dc4a970", "commit_rev":
"a07bd8034e0ebf854efb8cd80eb7d543d7b8b228"}

[commit-bot: I haz the power, 2017-04-21 18:36:19]

Description was changed from

==========
Remove unneeded abstractions and simplify RecordingImageBufferSurface

This is a set of misc cleanups I found while rearranging code around
RecordingImageBufferSurface in order to remove
PaintCanvas::writePixels.

- Replaces WrapUnique with MakeUnique.
- Removes the unneeded factory abstraction, since all instances of it
make an UnacceleratedImageBufferSurface.
- Removes unneeded friend relationship from RecordingImageBufferSurface
to its unit tests.
- Change tests to check IsRecording() instead of peeking internals or
checking if the factory was used. IsRecording() is true if we did not
fall back to UnacceleratedImageBufferSurface and false otherwise,
which is what the tests want to know.
- Use IntRect::Contains instead of writing that with comparisons.
- Remove "ForTesting" from a class name defined in tests, its also
redundant with the Fake prefix that is already present.
- Some capitalization/punctuation for comments.

R=pdr@chromium.org, xlai@chromium.org
BUG=671433
==========

to

==========
Remove unneeded abstractions and simplify RecordingImageBufferSurface

This is a set of misc cleanups I found while rearranging code around
RecordingImageBufferSurface in order to remove
PaintCanvas::writePixels.

- Replaces WrapUnique with MakeUnique.
- Removes the unneeded factory abstraction, since all instances of it
make an UnacceleratedImageBufferSurface.
- Removes unneeded friend relationship from RecordingImageBufferSurface
to its unit tests.
- Change tests to check IsRecording() instead of peeking internals or
checking if the factory was used. IsRecording() is true if we did not
fall back to UnacceleratedImageBufferSurface and false otherwise,
which is what the tests want to know.
- Use IntRect::Contains instead of writing that with comparisons.
- Remove "ForTesting" from a class name defined in tests, its also
redundant with the Fake prefix that is already present.
- Some capitalization/punctuation for comments.

R=pdr@chromium.org, xlai@chromium.org
BUG=671433

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

[commit-bot: I haz the power, 2017-04-21 18:36:21]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/a07bd8034e0ebf854efb8cd80eb7...

[ikilpatrick, 2017-04-21 23:22:52]

On 2017/04/21 14:52:11, danakj wrote:
> On 2017/04/21 04:45:45, ikilpatrick wrote:
> > On 2017/04/20 22:57:13, danakj wrote:
> > > ikilpatrick: ping. should i find a higher level reviewer?
> > > 
> > > I think the android thing was just TOT at the time. Trying the bots again.
> > 
> > lgtm, however just one question related to csspaint:
> > 
> >  In the case of the kExpensiveRecordingStackDepth fallback - is
> useful/important
> > to fallback even if the RecordingImageBufferSurface is just being used for
one
> > frame?
> > (The csspaint api has a new PRC2D each frame).
> 
> My understanding is no. The point of the fallback is to flatten the recording
so
> its not very expensive to play back over and over. Thanks!

Should RecordingImageBufferSurface take an additional bool then to prevent
fallback for the PRC2D case?

[danakj, 2017-04-25 21:46:40]

On 2017/04/21 23:22:52, ikilpatrick wrote:
> On 2017/04/21 14:52:11, danakj wrote:
> > On 2017/04/21 04:45:45, ikilpatrick wrote:
> > > On 2017/04/20 22:57:13, danakj wrote:
> > > > ikilpatrick: ping. should i find a higher level reviewer?
> > > > 
> > > > I think the android thing was just TOT at the time. Trying the bots
again.
> > > 
> > > lgtm, however just one question related to csspaint:
> > > 
> > >  In the case of the kExpensiveRecordingStackDepth fallback - is
> > useful/important
> > > to fallback even if the RecordingImageBufferSurface is just being used for
> one
> > > frame?
> > > (The csspaint api has a new PRC2D each frame).
> > 
> > My understanding is no. The point of the fallback is to flatten the
recording
> so
> > its not very expensive to play back over and over. Thanks!
> 
> Should RecordingImageBufferSurface take an additional bool then to prevent
> fallback for the PRC2D case?

Ok ya sure. I'll add an AllowFallback enum to the constructor in another CL
since this landed.

[danakj, 2017-04-25 22:00:07]

Patchset #7 (id:120001) has been deleted

[ikilpatrick, 2017-04-25 22:54:49]

On 2017/04/25 21:46:40, danakj wrote:
> On 2017/04/21 23:22:52, ikilpatrick wrote:
> > On 2017/04/21 14:52:11, danakj wrote:
> > > On 2017/04/21 04:45:45, ikilpatrick wrote:
> > > > On 2017/04/20 22:57:13, danakj wrote:
> > > > > ikilpatrick: ping. should i find a higher level reviewer?
> > > > > 
> > > > > I think the android thing was just TOT at the time. Trying the bots
> again.
> > > > 
> > > > lgtm, however just one question related to csspaint:
> > > > 
> > > >  In the case of the kExpensiveRecordingStackDepth fallback - is
> > > useful/important
> > > > to fallback even if the RecordingImageBufferSurface is just being used
for
> > one
> > > > frame?
> > > > (The csspaint api has a new PRC2D each frame).
> > > 
> > > My understanding is no. The point of the fallback is to flatten the
> recording
> > so
> > > its not very expensive to play back over and over. Thanks!
> > 
> > Should RecordingImageBufferSurface take an additional bool then to prevent
> > fallback for the PRC2D case?
> 
> Ok ya sure. I'll add an AllowFallback enum to the constructor in another CL
> since this landed.

Sounds great, thanks :).