Merge color options into ColorBehavior

Merge color options into ColorBehavior

There are dozens of places where we were passing an
blink::ImageDecoder::ColorSpaceOption and sk_sp<SkColorSpace>. Merge
these into a single ColorBehavior structure.

TBR=peter
BUG=667420
Committed: https://crrev.com/bf8e2f1a7b41c6f96136f9fc9617e685e0cb92bf
Cr-Commit-Position: refs/heads/master@{#437100}

[ccameron, 2016-12-06 22:50:35]

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

[ccameron, 2016-12-06 22:51:09]

Description was changed from

==========
Merge color options into ColorBehavior

There are dozens of places where we were passing an
blink::ImageDecoder::ColorSpaceOption and sk_sp<SkColorSpace>. Merge
these into a single ColorBehavior structure.

BUG=667420
==========

to

==========
Merge color options into ColorBehavior

There are dozens of places where we were passing an
blink::ImageDecoder::ColorSpaceOption and sk_sp<SkColorSpace>. Merge
these into a single ColorBehavior structure.

BUG=667420
==========

[ccameron, 2016-12-06 22:51:10]

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

[ccameron, 2016-12-06 22:51:31]

Lots of manual search-replace ...

[commit-bot: I haz the power, 2016-12-06 22:51:32]

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

[ccameron, 2016-12-06 22:53:38]

Description was changed from

==========
Merge color options into ColorBehavior

There are dozens of places where we were passing an
blink::ImageDecoder::ColorSpaceOption and sk_sp<SkColorSpace>. Merge
these into a single ColorBehavior structure.

BUG=667420
==========

to

==========
Merge color options into ColorBehavior

There are dozens of places where we were passing an
blink::ImageDecoder::ColorSpaceOption and sk_sp<SkColorSpace>. Merge
these into a single ColorBehavior structure.

TBR=peter
BUG=667420
==========

[commit-bot: I haz the power, 2016-12-07 00:56:26]

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

[commit-bot: I haz the power, 2016-12-07 00:56:27]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-12-07 19:29:37]

s/peter/philip but otherwise a great change :)

Some minor comments below.

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

https://codereview.chromium.org/2556723003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:392: m_image =
cropImage(input.get(), parsedOptions, PremultiplyAlpha, true);
I think passing a bool here is a small regression since it's no longer clear
what it means at the callsites. WDYT about using an enum for this?

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

https://codereview.chromium.org/2556723003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ColorBehavior.h:52: Ignore,
These are referenced outside this class but it's hard to follow what they mean
since the comments are up in ColorBehavior::ignore(). Can you think of a better
way to document these or encapsulate their details inside this class?

[ccameron, 2016-12-07 20:30:35]

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

[commit-bot: I haz the power, 2016-12-07 20:31:07]

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

[ccameron, 2016-12-07 20:31:38]

Thanks -- updated

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

https://codereview.chromium.org/2556723003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:392: m_image =
cropImage(input.get(), parsedOptions, PremultiplyAlpha, true);
On 2016/12/07 19:29:36, pdr. wrote:
> I think passing a bool here is a small regression since it's no longer clear
> what it means at the callsites. WDYT about using an enum for this?

It'll need to be a ColorBehavior eventually (a future patch allows us to specify
that to imageForCurrentFrame), so I just did that now.

Also got rid of the default arguments...

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

https://codereview.chromium.org/2556723003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ColorBehavior.h:52: Ignore,
On 2016/12/07 19:29:37, pdr. wrote:
> These are referenced outside this class but it's hard to follow what they mean
> since the comments are up in ColorBehavior::ignore(). Can you think of a
better
> way to document these or encapsulate their details inside this class?

Added isIgnore/isTransformToTargetColorSpace methods next to the create
functions, and made the enum private.

[pdr., 2016-12-07 22:19:02]

On 2016/12/07 at 20:31:38, ccameron wrote:
> Thanks -- updated
> 
>
https://codereview.chromium.org/2556723003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):
> 
>
https://codereview.chromium.org/2556723003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/ImageBitmap.cpp:392: m_image =
cropImage(input.get(), parsedOptions, PremultiplyAlpha, true);
> On 2016/12/07 19:29:36, pdr. wrote:
> > I think passing a bool here is a small regression since it's no longer clear
> > what it means at the callsites. WDYT about using an enum for this?
> 
> It'll need to be a ColorBehavior eventually (a future patch allows us to
specify that to imageForCurrentFrame), so I just did that now.
> 
> Also got rid of the default arguments...
> 
>
https://codereview.chromium.org/2556723003/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/graphics/ColorBehavior.h (right):
> 
>
https://codereview.chromium.org/2556723003/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/graphics/ColorBehavior.h:52: Ignore,
> On 2016/12/07 19:29:37, pdr. wrote:
> > These are referenced outside this class but it's hard to follow what they
mean
> > since the comments are up in ColorBehavior::ignore(). Can you think of a
better
> > way to document these or encapsulate their details inside this class?
> 
> Added isIgnore/isTransformToTargetColorSpace methods next to the create
functions, and made the enum private.

LGTM

[commit-bot: I haz the power, 2016-12-07 22:19:10]

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

[commit-bot: I haz the power, 2016-12-07 22:19:11]

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

[ccameron, 2016-12-07 22:26:57]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2016-12-07 22:27: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, 2016-12-07 23:06:28]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481149617709370,
"parent_rev": "1e00385c285b690238169e6bd9036bb686fd72ae", "commit_rev":
"6d786d3d076e85525d84f19ba65e0279ccabf02b"}

[commit-bot: I haz the power, 2016-12-07 23:06:56]

Description was changed from

==========
Merge color options into ColorBehavior

There are dozens of places where we were passing an
blink::ImageDecoder::ColorSpaceOption and sk_sp<SkColorSpace>. Merge
these into a single ColorBehavior structure.

TBR=peter
BUG=667420
==========

to

==========
Merge color options into ColorBehavior

There are dozens of places where we were passing an
blink::ImageDecoder::ColorSpaceOption and sk_sp<SkColorSpace>. Merge
these into a single ColorBehavior structure.

TBR=peter
BUG=667420
==========

[commit-bot: I haz the power, 2016-12-07 23:06:57]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-07 23:09:06]

Description was changed from

==========
Merge color options into ColorBehavior

There are dozens of places where we were passing an
blink::ImageDecoder::ColorSpaceOption and sk_sp<SkColorSpace>. Merge
these into a single ColorBehavior structure.

TBR=peter
BUG=667420
==========

to

==========
Merge color options into ColorBehavior

There are dozens of places where we were passing an
blink::ImageDecoder::ColorSpaceOption and sk_sp<SkColorSpace>. Merge
these into a single ColorBehavior structure.

TBR=peter
BUG=667420

Committed: https://crrev.com/bf8e2f1a7b41c6f96136f9fc9617e685e0cb92bf
Cr-Commit-Position: refs/heads/master@{#437100}
==========

[commit-bot: I haz the power, 2016-12-07 23:09:08]

Patchset 2 (id:??) landed as
https://crrev.com/bf8e2f1a7b41c6f96136f9fc9617e685e0cb92bf
Cr-Commit-Position: refs/heads/master@{#437100}