Follow up on previous cl

Follow up on previous cl

Fixed nits and issue from https://codereview.chromium.org/418653002/

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180722

[Stephen White, 2014-08-19 22:39:32]

rubber-stamp LGTM (ie., wait for Noel's LG).

[Noel Gordon, 2014-08-20 16:24:16]

lgtm with nits

https://codereview.chromium.org/480093003/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right):

https://codereview.chromium.org/480093003/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:470: if
(overrideColorSpace != JCS_UNKNOWN) {
Is this if statement needed?

[sugoi1, 2014-08-20 16:35:46]

On 2014/08/20 16:24:16, Noel Gordon wrote:
> lgtm with nits
> 
>
https://codereview.chromium.org/480093003/diff/1/Source/platform/image-decode...
> File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right):
> 
>
https://codereview.chromium.org/480093003/diff/1/Source/platform/image-decode...
> Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:470: if
> (overrideColorSpace != JCS_UNKNOWN) {
> Is this if statement needed?

It's not, I'll remove it. Thanks.

[sugoi1, 2014-08-20 17:05:33]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2014-08-20 17:05:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sugoi@chromium.org/480093003/20001

[commit-bot: I haz the power, 2014-08-20 17:49:03]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/20575)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/23688)

[sugoi1, 2014-08-20 18:00:29]

The CQ bit was unchecked by sugoi@chromium.org

[sugoi1, 2014-08-20 18:26:19]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2014-08-20 18:27:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sugoi@chromium.org/480093003/40001

[sugoi1, 2014-08-20 18:40:51]

https://codereview.chromium.org/480093003/diff/40001/Source/platform/image-de...
File Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp (right):

https://codereview.chromium.org/480093003/diff/40001/Source/platform/image-de...
Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp:224:
EXPECT_EQ(256u, outputUVWidth);
There's a color profile on that test jpg file, so I'll revert this change once
the yuv decoding supports color profiles.

[commit-bot: I haz the power, 2014-08-20 19:13:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/23708)

[commit-bot: I haz the power, 2014-08-20 19:16:07]

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

[commit-bot: I haz the power, 2014-08-20 19:16:09]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[sugoi1, 2014-08-20 22:34:33]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2014-08-20 22:35:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sugoi@chromium.org/480093003/40001

[commit-bot: I haz the power, 2014-08-20 22:38:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[commit-bot: I haz the power, 2014-08-20 22:41:44]

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

[commit-bot: I haz the power, 2014-08-20 22:41:45]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[sugoi1, 2014-08-21 01:27:10]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2014-08-21 01:27:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sugoi@chromium.org/480093003/40001

[commit-bot: I haz the power, 2014-08-21 01:31:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[commit-bot: I haz the power, 2014-08-21 01:33:43]

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

[commit-bot: I haz the power, 2014-08-21 01:33:44]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[Noel Gordon, 2014-08-21 01:33:46]

https://codereview.chromium.org/480093003/diff/40001/Source/platform/image-de...
File Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp (right):

https://codereview.chromium.org/480093003/diff/40001/Source/platform/image-de...
Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp:224:
EXPECT_EQ(256u, outputUVWidth);
There's no color profile in fast/images/resources/lenna.jpg (you can upload it
to http://regex.info/exif.cgi to check).

[Noel Gordon, 2014-08-21 02:59:26]

https://codereview.chromium.org/480093003/diff/40001/Source/platform/image-de...
File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right):

https://codereview.chromium.org/480093003/diff/40001/Source/platform/image-de...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:441: m_state =
JPEG_START_DECOMPRESS;
I think the problem is here: there may has been a reader state change that
effectively disables overrideColorSpace even though it's allowed.  Maybe
overrideColorSpace needs to be a member variable of the reader to avoid that?

[sugoi1, 2014-08-21 10:59:14]

On 2014/08/21 02:59:26, Noel Gordon wrote:
>
https://codereview.chromium.org/480093003/diff/40001/Source/platform/image-de...
> File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right):
> 
>
https://codereview.chromium.org/480093003/diff/40001/Source/platform/image-de...
> Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:441: m_state =
> JPEG_START_DECOMPRESS;
> I think the problem is here: there may has been a reader state change that
> effectively disables overrideColorSpace even though it's allowed.  Maybe
> overrideColorSpace needs to be a member variable of the reader to avoid that?

Oh, I think I know what it is. At line JPEGImageDecoder.cpp:461, I set
overrideColorSpace = JCS_UNKNOWN; unconditionally, but this only depends on
!ignoresGammaAndColorProfile(). I need to do this conditionally only when
there's an m_transform :)

[Noel Gordon, 2014-08-21 11:12:44]

OK good, let's try it and and put the test back the way it was.

[sugoi1, 2014-08-21 12:24:19]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2014-08-21 12:25:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sugoi@chromium.org/480093003/60001

[Noel Gordon, 2014-08-21 12:52:24]

lgtm pending linux test results.

[commit-bot: I haz the power, 2014-08-21 16:52:00]

Committed patchset #4 (60001) as 180722