Refactor ImageDecoder factories

Refactor ImageDecoder factories

The vast majority of ImageDecoder::create clients do something along
these lines:

  sniffResult = ImageDecoder::determineImageType(data);
  decoder = ImageDecoder::create(sniffResult, ...);
  if (decoder)
      decoder->setData(data, ...);

I.e.

1) we start off with some data in a SharedBuffer
2) we call determineImageType() on that data, to figure the codec type
3) we instantiate the decoder
4) and, finally, we pass the data to the new decoder using setData()

The downsides are

a) determineImageType() allocates a temporary SegmentReader in order
to use FastSegmentReader for signature consolidation; this reader is
then thrown away and recreated in setData() (because internally
ImageDecoder stores a SegmentReader).

b) unnecessarily verbose/stateful decoder construction sequence

This CL expands the create() factory to fuse all these steps.  We get
to reuse a single SegmentReader, and the construction is now atomic.

We also get to remove determineImageType from the public ImageDecoder
API.

Refactoring only, no functional changes expected.

Committed: https://crrev.com/9e2e0f5824975f43ad9374ea5e97100b77925fc1
Cr-Commit-Position: refs/heads/master@{#413736}

[f(malita), 2016-08-17 16:30:08]

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

[commit-bot: I haz the power, 2016-08-17 16:30:26]

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

[f(malita), 2016-08-17 16:32:22]

Description was changed from

==========
Refactor ImageDecoder factories

The vast majority of ImageDecoder::create clients do something along
these lines:

  sniffResult = ImageDecoder::determineImageType(data);
  decoder = ImageDecoder::create(sniffResult, ...);
  if (decoder)
      decoder->setData(data, ...);

On top of being unnecessarily explicit, this also has the downside of a
potential temporary SegmentReader allocation in determineImageType.

This CL explores the alternative approach of passing the data directly
to the ImageDecoder factory:

  decoder = ImageDecoder::create(data, ...);

Ultimately this should alow us to make determineImageType private.
==========

to

==========
Refactor ImageDecoder factories

The vast majority of ImageDecoder::create clients do something along
these lines:

  sniffResult = ImageDecoder::determineImageType(data);
  decoder = ImageDecoder::create(sniffResult, ...);
  if (decoder)
      decoder->setData(data, ...);

On top of being unnecessarily explicit, this also has the downside of a
potential temporary SegmentReader allocation in determineImageType.

This CL explores the alternative approach of passing the data directly
to the ImageDecoder factory:

  decoder = ImageDecoder::create(data, ...);

Since ImageDecoder stores a SegmentReader, by fusing the factory with
the determineImageType call we can avoid the extra SegmentReader
allocation.

Ultimately this should also allow us to make determineImageType
private.
==========

[commit-bot: I haz the power, 2016-08-17 19:37:27]

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

[commit-bot: I haz the power, 2016-08-17 19:37:28]

Dry run: This issue passed the CQ dry run.

[f(malita), 2016-08-18 17:12:52]

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

[commit-bot: I haz the power, 2016-08-18 17:13:19]

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

[f(malita), 2016-08-18 17:32:56]

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

[commit-bot: I haz the power, 2016-08-18 17:33:28]

Dry run: 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-08-18 21:20:08]

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

[commit-bot: I haz the power, 2016-08-18 21:20:09]

Dry run: 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_...)

[f(malita), 2016-08-19 17:14:40]

Description was changed from

==========
Refactor ImageDecoder factories

The vast majority of ImageDecoder::create clients do something along
these lines:

  sniffResult = ImageDecoder::determineImageType(data);
  decoder = ImageDecoder::create(sniffResult, ...);
  if (decoder)
      decoder->setData(data, ...);

On top of being unnecessarily explicit, this also has the downside of a
potential temporary SegmentReader allocation in determineImageType.

This CL explores the alternative approach of passing the data directly
to the ImageDecoder factory:

  decoder = ImageDecoder::create(data, ...);

Since ImageDecoder stores a SegmentReader, by fusing the factory with
the determineImageType call we can avoid the extra SegmentReader
allocation.

Ultimately this should also allow us to make determineImageType
private.
==========

to

==========
Refactor ImageDecoder factories

The vast majority of ImageDecoder::create clients do something along
these lines:

  sniffResult = ImageDecoder::determineImageType(data);
  decoder = ImageDecoder::create(sniffResult, ...);
  if (decoder)
      decoder->setData(data, ...);

I.e.

1) we start off with some data in a SharedBuffer
2) we call determineImageType() on that data, to figure the codec type
3) we instantiate the decoder
4) and, finally, we pass the data to the new decoder using setData()

The downsides are

a) determineImageType() allocates a temporary SegmentReader in order
to use FastSegmentReader for signature consolidation; this reader is
then thrown away and recreated in setData() (because internally
ImageDecoder stores a SegmentReader).

b) unnecessarily verbose/stateful decoder construction sequence


This CL expands the create() factory to fuse all these steps.  We get
to reuse a single SegmentReader, and the construction is now atomic.

We also get to remove determineImageType from the public ImageDecoder
API.

Refactoring only, nu functional changes expected.
==========

[f(malita), 2016-08-19 17:39:41]

fmalita@chromium.org changed reviewers:
+ pkasting@chromium.org, schenney@chromium.org, scroggo@chromium.org

[f(malita), 2016-08-19 17:39:42]

Follow-up to http://crrev.com/2252723003.

Hiding some ImageDecoder details, and hopefully making the client code cleaner.

[f(malita), 2016-08-19 17:40:32]

Description was changed from

==========
Refactor ImageDecoder factories

The vast majority of ImageDecoder::create clients do something along
these lines:

  sniffResult = ImageDecoder::determineImageType(data);
  decoder = ImageDecoder::create(sniffResult, ...);
  if (decoder)
      decoder->setData(data, ...);

I.e.

1) we start off with some data in a SharedBuffer
2) we call determineImageType() on that data, to figure the codec type
3) we instantiate the decoder
4) and, finally, we pass the data to the new decoder using setData()

The downsides are

a) determineImageType() allocates a temporary SegmentReader in order
to use FastSegmentReader for signature consolidation; this reader is
then thrown away and recreated in setData() (because internally
ImageDecoder stores a SegmentReader).

b) unnecessarily verbose/stateful decoder construction sequence


This CL expands the create() factory to fuse all these steps.  We get
to reuse a single SegmentReader, and the construction is now atomic.

We also get to remove determineImageType from the public ImageDecoder
API.

Refactoring only, nu functional changes expected.
==========

to

==========
Refactor the ImageDecoder factories

The vast majority of ImageDecoder::create clients do something along
these lines:

  sniffResult = ImageDecoder::determineImageType(data);
  decoder = ImageDecoder::create(sniffResult, ...);
  if (decoder)
      decoder->setData(data, ...);

I.e.

1) we start off with some data in a SharedBuffer
2) we call determineImageType() on that data, to figure the codec type
3) we instantiate the decoder
4) and, finally, we pass the data to the new decoder using setData()

The downsides are

a) determineImageType() allocates a temporary SegmentReader in order
to use FastSegmentReader for signature consolidation; this reader is
then thrown away and recreated in setData() (because internally
ImageDecoder stores a SegmentReader).

b) unnecessarily verbose/stateful decoder construction sequence


This CL expands the create() factory to fuse all these steps.  We get
to reuse a single SegmentReader, and the construction is now atomic.

We also get to remove determineImageType from the public ImageDecoder
API.

Refactoring only, nu functional changes expected.
==========

[Peter Kasting, 2016-08-19 23:04:59]

LGTM

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
(right):

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:93:
std::unique_ptr<ImageDecoder> decoder = ImageDecoder::create(m_data, true /*
allDataReceived */,
Nit: I'd just omit the comment

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:70:
std::unique_ptr<DeferredImageDecoder> decoder = wrapUnique(new
DeferredImageDecoder(std::move(actualDecoder)));
Nit: Seems like the wrapUnique() call here doesn't buy us anything (just
construct |decoder| directly)

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTestWoPlatform.cpp
(left):

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTestWoPlatform.cpp:87:
ImageDecoder::SniffResult m_expectedResult;
It seems like we've lost the functionality of checking that the correct type is
sniffed.  Is that tested elsewhere?  If so, remove the struct wrapper around the
single remaining member.  If not, check the image type of the resulting image
decoder.

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:263: bool
newNonFactoryDecoder = false;
Nit: This is only checked in a negation below, so find a way to reverse its
meaning to remove that negation.  I would maybe call it "shouldCallSetData".

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:112: decoder
= wrapUnique(new JPEGImageDecoder(alphaOption, colorOptions, maxDecodedBytes));
Nit: Do these wrapUnique() calls buy us anything anymore?

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right):

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:117: // callers
to determine whether a decoder instantiation failure is due to insufficient or
bad data.
Nit: Seems like most of this file prefers to wrap comments at 80 cols.

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:118: static
bool isSufficientData(const SharedBuffer&);
Nit: I'm not a big fan of this function name.  Something like
hasSufficientDataToSniffImageType() or canSniffImageType() might be better. 
(The latter reads more clearly but I'm worried it implies we should return false
if the sniffed type is invalid.)

[f(malita), 2016-08-21 15:00:17]

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

[commit-bot: I haz the power, 2016-08-21 15:00:25]

Dry run: 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-08-21 16:10:13]

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

[commit-bot: I haz the power, 2016-08-21 16:10:14]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[f(malita), 2016-08-22 01:51:16]

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
(right):

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:93:
std::unique_ptr<ImageDecoder> decoder = ImageDecoder::create(m_data, true /*
allDataReceived */,
On 2016/08/19 23:04:58, Peter Kasting wrote:
> Nit: I'd just omit the comment

Done.

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:70:
std::unique_ptr<DeferredImageDecoder> decoder = wrapUnique(new
DeferredImageDecoder(std::move(actualDecoder)));
On 2016/08/19 23:04:58, Peter Kasting wrote:
> Nit: Seems like the wrapUnique() call here doesn't buy us anything (just
> construct |decoder| directly)

Done.

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTestWoPlatform.cpp
(left):

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTestWoPlatform.cpp:87:
ImageDecoder::SniffResult m_expectedResult;
On 2016/08/19 23:04:58, Peter Kasting wrote:
> It seems like we've lost the functionality of checking that the correct type
is
> sniffed.  Is that tested elsewhere?  If so, remove the struct wrapper around
the
> single remaining member.  If not, check the image type of the resulting image
> decoder.

Indeed.  I would hope we have other tests to verify correct decoder selection,
but it's also trivial to update this one and check as you suggested.  Done.

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:263: bool
newNonFactoryDecoder = false;
On 2016/08/19 23:04:58, Peter Kasting wrote:
> Nit: This is only checked in a negation below, so find a way to reverse its
> meaning to remove that negation.  I would maybe call it "shouldCallSetData".

Done.

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:112: decoder
= wrapUnique(new JPEGImageDecoder(alphaOption, colorOptions, maxDecodedBytes));
On 2016/08/19 23:04:58, Peter Kasting wrote:
> Nit: Do these wrapUnique() calls buy us anything anymore?

There's an assert in there that we're not trying to wrap a garbage-collected
object, but in this case it's pretty clear decoders are not GCed.

Updated to reset().

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right):

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:117: // callers
to determine whether a decoder instantiation failure is due to insufficient or
bad data.
On 2016/08/19 23:04:58, Peter Kasting wrote:
> Nit: Seems like most of this file prefers to wrap comments at 80 cols.

Done.

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:118: static
bool isSufficientData(const SharedBuffer&);
On 2016/08/19 23:04:58, Peter Kasting wrote:
> Nit: I'm not a big fan of this function name.  Something like
> hasSufficientDataToSniffImageType() or canSniffImageType() might be better. 
> (The latter reads more clearly but I'm worried it implies we should return
false
> if the sniffed type is invalid.)

Changed to hasSufficientDataToSniffImageType().

[f(malita), 2016-08-23 12:59:20]

Description was changed from

==========
Refactor the ImageDecoder factories

The vast majority of ImageDecoder::create clients do something along
these lines:

  sniffResult = ImageDecoder::determineImageType(data);
  decoder = ImageDecoder::create(sniffResult, ...);
  if (decoder)
      decoder->setData(data, ...);

I.e.

1) we start off with some data in a SharedBuffer
2) we call determineImageType() on that data, to figure the codec type
3) we instantiate the decoder
4) and, finally, we pass the data to the new decoder using setData()

The downsides are

a) determineImageType() allocates a temporary SegmentReader in order
to use FastSegmentReader for signature consolidation; this reader is
then thrown away and recreated in setData() (because internally
ImageDecoder stores a SegmentReader).

b) unnecessarily verbose/stateful decoder construction sequence


This CL expands the create() factory to fuse all these steps.  We get
to reuse a single SegmentReader, and the construction is now atomic.

We also get to remove determineImageType from the public ImageDecoder
API.

Refactoring only, nu functional changes expected.
==========

to

==========
Refactor the ImageDecoder factories

The vast majority of ImageDecoder::create clients do something along
these lines:

  sniffResult = ImageDecoder::determineImageType(data);
  decoder = ImageDecoder::create(sniffResult, ...);
  if (decoder)
      decoder->setData(data, ...);

I.e.

1) we start off with some data in a SharedBuffer
2) we call determineImageType() on that data, to figure the codec type
3) we instantiate the decoder
4) and, finally, we pass the data to the new decoder using setData()

The downsides are

a) determineImageType() allocates a temporary SegmentReader in order
to use FastSegmentReader for signature consolidation; this reader is
then thrown away and recreated in setData() (because internally
ImageDecoder stores a SegmentReader).

b) unnecessarily verbose/stateful decoder construction sequence


This CL expands the create() factory to fuse all these steps.  We get
to reuse a single SegmentReader, and the construction is now atomic.

We also get to remove determineImageType from the public ImageDecoder
API.

Refactoring only, no functional changes expected.
==========

[f(malita), 2016-08-23 12:59:49]

The CQ bit was checked by fmalita@chromium.org

[f(malita), 2016-08-23 12:59:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2257513002/#ps60001
(title: "review")

[commit-bot: I haz the power, 2016-08-23 13:00:01]

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

[f(malita), 2016-08-23 13:01:40]

Description was changed from

==========
Refactor the ImageDecoder factories

The vast majority of ImageDecoder::create clients do something along
these lines:

  sniffResult = ImageDecoder::determineImageType(data);
  decoder = ImageDecoder::create(sniffResult, ...);
  if (decoder)
      decoder->setData(data, ...);

I.e.

1) we start off with some data in a SharedBuffer
2) we call determineImageType() on that data, to figure the codec type
3) we instantiate the decoder
4) and, finally, we pass the data to the new decoder using setData()

The downsides are

a) determineImageType() allocates a temporary SegmentReader in order
to use FastSegmentReader for signature consolidation; this reader is
then thrown away and recreated in setData() (because internally
ImageDecoder stores a SegmentReader).

b) unnecessarily verbose/stateful decoder construction sequence


This CL expands the create() factory to fuse all these steps.  We get
to reuse a single SegmentReader, and the construction is now atomic.

We also get to remove determineImageType from the public ImageDecoder
API.

Refactoring only, no functional changes expected.
==========

to

==========
Refactor ImageDecoder factories

The vast majority of ImageDecoder::create clients do something along
these lines:

  sniffResult = ImageDecoder::determineImageType(data);
  decoder = ImageDecoder::create(sniffResult, ...);
  if (decoder)
      decoder->setData(data, ...);

I.e.

1) we start off with some data in a SharedBuffer
2) we call determineImageType() on that data, to figure the codec type
3) we instantiate the decoder
4) and, finally, we pass the data to the new decoder using setData()

The downsides are

a) determineImageType() allocates a temporary SegmentReader in order
to use FastSegmentReader for signature consolidation; this reader is
then thrown away and recreated in setData() (because internally
ImageDecoder stores a SegmentReader).

b) unnecessarily verbose/stateful decoder construction sequence


This CL expands the create() factory to fuse all these steps.  We get
to reuse a single SegmentReader, and the construction is now atomic.

We also get to remove determineImageType from the public ImageDecoder
API.

Refactoring only, no functional changes expected.
==========

[commit-bot: I haz the power, 2016-08-23 13:06:14]

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

[commit-bot: I haz the power, 2016-08-23 13:06:15]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Peter Beverloo, 2016-08-23 13:06:19]

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

[Peter Beverloo, 2016-08-23 13:06:20]

notifications lgtm

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
(right):

https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:93:
std::unique_ptr<ImageDecoder> decoder = ImageDecoder::create(m_data, true /*
allDataReceived */,
On 2016/08/22 01:51:15, f(malita) wrote:
> On 2016/08/19 23:04:58, Peter Kasting wrote:
> > Nit: I'd just omit the comment
> 
> Done.

Heh, I was about to comment with the opposite based on the latest PS— the
meaning of "true" is not clear when reading of this code, which is why,
historically, Blink preferred enumerations over booleans.

I don't feel super strongly, but do feel it's clearer with the comment.

[f(malita), 2016-08-23 13:25:47]

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

[commit-bot: I haz the power, 2016-08-23 13:26:03]

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

[f(malita), 2016-08-23 13:37:18]

On 2016/08/23 13:06:20, Peter Beverloo wrote:
> notifications lgtm
> 
>
https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
> (right):
> 
>
https://codereview.chromium.org/2257513002/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:93:
> std::unique_ptr<ImageDecoder> decoder = ImageDecoder::create(m_data, true /*
> allDataReceived */,
> On 2016/08/22 01:51:15, f(malita) wrote:
> > On 2016/08/19 23:04:58, Peter Kasting wrote:
> > > Nit: I'd just omit the comment
> > 
> > Done.
> 
> Heh, I was about to comment with the opposite based on the latest PS— the
> meaning of "true" is not clear when reading of this code, which is why,
> historically, Blink preferred enumerations over booleans.
> 
> I don't feel super strongly, but do feel it's clearer with the comment.

I'll go with your preference, as a notifications owner :)

(ACK using an enum; I did try it out a bit and it felt poorly contained within
this layer, had to convert to/from bool all over the place;  but if we expand
the scope a bit it should work nicely -- I'll look into that at some point)

[f(malita), 2016-08-23 13:46:01]

The CQ bit was unchecked by fmalita@chromium.org

[f(malita), 2016-08-23 13:46:07]

The CQ bit was checked by fmalita@chromium.org

[f(malita), 2016-08-23 13:46:08]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2257513002/#ps80001
(title: "review")

[commit-bot: I haz the power, 2016-08-23 13:46:30]

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-08-23 14:47:34]

Description was changed from

==========
Refactor ImageDecoder factories

The vast majority of ImageDecoder::create clients do something along
these lines:

  sniffResult = ImageDecoder::determineImageType(data);
  decoder = ImageDecoder::create(sniffResult, ...);
  if (decoder)
      decoder->setData(data, ...);

I.e.

1) we start off with some data in a SharedBuffer
2) we call determineImageType() on that data, to figure the codec type
3) we instantiate the decoder
4) and, finally, we pass the data to the new decoder using setData()

The downsides are

a) determineImageType() allocates a temporary SegmentReader in order
to use FastSegmentReader for signature consolidation; this reader is
then thrown away and recreated in setData() (because internally
ImageDecoder stores a SegmentReader).

b) unnecessarily verbose/stateful decoder construction sequence


This CL expands the create() factory to fuse all these steps.  We get
to reuse a single SegmentReader, and the construction is now atomic.

We also get to remove determineImageType from the public ImageDecoder
API.

Refactoring only, no functional changes expected.
==========

to

==========
Refactor ImageDecoder factories

The vast majority of ImageDecoder::create clients do something along
these lines:

  sniffResult = ImageDecoder::determineImageType(data);
  decoder = ImageDecoder::create(sniffResult, ...);
  if (decoder)
      decoder->setData(data, ...);

I.e.

1) we start off with some data in a SharedBuffer
2) we call determineImageType() on that data, to figure the codec type
3) we instantiate the decoder
4) and, finally, we pass the data to the new decoder using setData()

The downsides are

a) determineImageType() allocates a temporary SegmentReader in order
to use FastSegmentReader for signature consolidation; this reader is
then thrown away and recreated in setData() (because internally
ImageDecoder stores a SegmentReader).

b) unnecessarily verbose/stateful decoder construction sequence


This CL expands the create() factory to fuse all these steps.  We get
to reuse a single SegmentReader, and the construction is now atomic.

We also get to remove determineImageType from the public ImageDecoder
API.

Refactoring only, no functional changes expected.
==========

[commit-bot: I haz the power, 2016-08-23 14:47:35]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-23 14:49:18]

Description was changed from

==========
Refactor ImageDecoder factories

The vast majority of ImageDecoder::create clients do something along
these lines:

  sniffResult = ImageDecoder::determineImageType(data);
  decoder = ImageDecoder::create(sniffResult, ...);
  if (decoder)
      decoder->setData(data, ...);

I.e.

1) we start off with some data in a SharedBuffer
2) we call determineImageType() on that data, to figure the codec type
3) we instantiate the decoder
4) and, finally, we pass the data to the new decoder using setData()

The downsides are

a) determineImageType() allocates a temporary SegmentReader in order
to use FastSegmentReader for signature consolidation; this reader is
then thrown away and recreated in setData() (because internally
ImageDecoder stores a SegmentReader).

b) unnecessarily verbose/stateful decoder construction sequence


This CL expands the create() factory to fuse all these steps.  We get
to reuse a single SegmentReader, and the construction is now atomic.

We also get to remove determineImageType from the public ImageDecoder
API.

Refactoring only, no functional changes expected.
==========

to

==========
Refactor ImageDecoder factories

The vast majority of ImageDecoder::create clients do something along
these lines:

  sniffResult = ImageDecoder::determineImageType(data);
  decoder = ImageDecoder::create(sniffResult, ...);
  if (decoder)
      decoder->setData(data, ...);

I.e.

1) we start off with some data in a SharedBuffer
2) we call determineImageType() on that data, to figure the codec type
3) we instantiate the decoder
4) and, finally, we pass the data to the new decoder using setData()

The downsides are

a) determineImageType() allocates a temporary SegmentReader in order
to use FastSegmentReader for signature consolidation; this reader is
then thrown away and recreated in setData() (because internally
ImageDecoder stores a SegmentReader).

b) unnecessarily verbose/stateful decoder construction sequence

This CL expands the create() factory to fuse all these steps.  We get
to reuse a single SegmentReader, and the construction is now atomic.

We also get to remove determineImageType from the public ImageDecoder
API.

Refactoring only, no functional changes expected.

Committed: https://crrev.com/9e2e0f5824975f43ad9374ea5e97100b77925fc1
Cr-Commit-Position: refs/heads/master@{#413736}
==========

[commit-bot: I haz the power, 2016-08-23 14:49:19]

Patchset 5 (id:??) landed as
https://crrev.com/9e2e0f5824975f43ad9374ea5e97100b77925fc1
Cr-Commit-Position: refs/heads/master@{#413736}