Reenable warnings in src/codec

Reenable warnings in src/codec

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/f724b99435e0c7a6a95112229cc44a2ba813ecc9

[msarett, 2015-10-14 19:49:36]

msarett@google.com changed reviewers:
+ mtklein@google.com, scroggo@google.com

[msarett, 2015-10-14 19:49:37]

This'll need to land after Mike's CL that fixes our accumulated warnings lands.

https://codereview.chromium.org/1400343005/diff/1/gyp/libwebp.gyp
File gyp/libwebp.gyp (left):

https://codereview.chromium.org/1400343005/diff/1/gyp/libwebp.gyp#oldcode246
gyp/libwebp.gyp:246: 'cflags': [ '-w' ],
This is not doing what we thought it was doing.

Since these flags are listed under direct_dependent_settings, any targets that
depend on libwebp will be compiled with these flags.

[scroggo, 2015-10-14 20:00:06]

lgtm

[msarett, 2015-10-14 20:21:54]

The CQ bit was checked by msarett@google.com

[msarett, 2015-10-14 20:21:54]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1400343005/#ps20001
(title: "Adding Mike's warning fixes plus a few others")

[commit-bot: I haz the power, 2015-10-14 20:22:10]

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

[commit-bot: I haz the power, 2015-10-14 20:22:54]

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

[commit-bot: I haz the power, 2015-10-14 20:22:55]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)

[msarett, 2015-10-14 20:27:55]

The CQ bit was checked by msarett@google.com

[msarett, 2015-10-14 20:27:55]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1400343005/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2015-10-14 20:28:07]

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

[commit-bot: I haz the power, 2015-10-14 20:29:09]

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

[commit-bot: I haz the power, 2015-10-14 20:29:10]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)

[scroggo, 2015-10-14 20:41:33]

not lgtm

https://codereview.chromium.org/1400343005/diff/40001/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/1400343005/diff/40001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:154: #if defined (GOOGLE3)
Instead, should this be based on whether JCS_RGB565 means anything? It seems
like if it's not defined, we just want to set colorBytes to
dinfo->out_color_components?

https://codereview.chromium.org/1400343005/diff/40001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:252: // Check the byte ordering of the RGBA color
space for the
nit: I think this comment goes inside the #else statement.

https://codereview.chromium.org/1400343005/diff/40001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:255: return false;
So in GOOGLE3 we can only do CMYK to N32? That seems wrong

https://codereview.chromium.org/1400343005/diff/40001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:413: SK_CRASH();
Why this difference?

[msarett, 2015-10-14 21:56:10]

Patchset #4 (id:60001) has been deleted

[msarett, 2015-10-14 21:59:00]

Sorry for not being so thorough on the previous patch set

https://codereview.chromium.org/1400343005/diff/80001/gyp/images.gyp
File gyp/images.gyp (right):

https://codereview.chromium.org/1400343005/diff/80001/gyp/images.gyp#newcode134
gyp/images.gyp:134: 'cflags' : [
SkImageDecoder_libpng includes png.h, which has this error in the old Android
version:
error: invalid suffix on literal; C++11 requires a space between literal and
identifier

[msarett, 2015-10-14 21:59:06]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-14 21:59:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1400343005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1400343005/80001

[commit-bot: I haz the power, 2015-10-14 22:06:38]

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

[commit-bot: I haz the power, 2015-10-14 22:06:38]

Dry run: This issue passed the CQ dry run.

[mtklein, 2015-10-14 22:15:10]

lgtm, but might want to wait for leon too.

https://codereview.chromium.org/1400343005/diff/80001/gyp/images.gyp
File gyp/images.gyp (right):

https://codereview.chromium.org/1400343005/diff/80001/gyp/images.gyp#newcode134
gyp/images.gyp:134: 'cflags' : [
On 2015/10/14 21:58:59, msarett wrote:
> SkImageDecoder_libpng includes png.h, which has this error in the old Android
> version:
> error: invalid suffix on literal; C++11 requires a space between literal and
> identifier

Add this note here in the file?

[msarett, 2015-10-14 22:28:53]

https://codereview.chromium.org/1400343005/diff/80001/gyp/images.gyp
File gyp/images.gyp (right):

https://codereview.chromium.org/1400343005/diff/80001/gyp/images.gyp#newcode134
gyp/images.gyp:134: 'cflags' : [
On 2015/10/14 22:15:10, mtklein wrote:
> On 2015/10/14 21:58:59, msarett wrote:
> > SkImageDecoder_libpng includes png.h, which has this error in the old
Android
> > version:
> > error: invalid suffix on literal; C++11 requires a space between literal and
> > identifier
> 
> Add this note here in the file?

Done.

[scroggo, 2015-10-15 12:49:54]

> Patch Set 4 : Removed Google3 code from SkJpegCodec, fixed
> skip_scanlines fallback code, fixed additional warning… 

I'm fine with having Google3 code; it just looked to me like it flat out failed
in Google3. Maybe we cannot do better without actually testing in Google3?
Should we file a bug about this?

https://codereview.chromium.org/1400343005/diff/100001/src/codec/SkCodec_libg...
File src/codec/SkCodec_libgif.cpp (right):

https://codereview.chromium.org/1400343005/diff/100001/src/codec/SkCodec_libg...
src/codec/SkCodec_libgif.cpp:184: gif_error("Invalid gif size.\n");
I think this should return false. Otherwise, we go on to read size and
frameRect, even though they have not been initialized. If we do not return
false, we'll go down to create a new codec and return true, even though the
dimensions were not valid (and set to zero, in the fix down below).

https://codereview.chromium.org/1400343005/diff/100001/src/codec/SkCodec_libg...
src/codec/SkCodec_libgif.cpp:363: size->set(0, 0);
I take it this fixes a warning that we read these variables even though they
were uninitialized? I think this is the wrong fix (see comments at the call
site). The header states that these variables will be set if GetDimensions
returns true. If it returns false, this function has no obligation to initialize
them, and more importantly, the caller should not read them.

[msarett, 2015-10-15 13:06:41]

"I'm fine with having Google3 code; it just looked to me like it flat out failed
in Google3. Maybe we cannot do better without actually testing in Google3?
Should we file a bug about this?"

Yes we fail in Google3.  And it's actually on an old, old version of turbo that
doesn't have RGBA, BGRA yet.  libjpeg doesn't have this either.

We need a fallback path that uses SkSwizzler.  I will file a bug.

https://codereview.chromium.org/1400343005/diff/100001/src/codec/SkCodec_libg...
File src/codec/SkCodec_libgif.cpp (right):

https://codereview.chromium.org/1400343005/diff/100001/src/codec/SkCodec_libg...
src/codec/SkCodec_libgif.cpp:184: gif_error("Invalid gif size.\n");
On 2015/10/15 12:49:54, scroggo wrote:
> I think this should return false. Otherwise, we go on to read size and
> frameRect, even though they have not been initialized. If we do not return
> false, we'll go down to create a new codec and return true, even though the
> dimensions were not valid (and set to zero, in the fix down below).

Of course, this is missing a return statement.  I was confused about why the
compiler was complaining about uninitialized values below.

https://codereview.chromium.org/1400343005/diff/100001/src/codec/SkCodec_libg...
src/codec/SkCodec_libgif.cpp:363: size->set(0, 0);
On 2015/10/15 12:49:53, scroggo wrote:
> I take it this fixes a warning that we read these variables even though they
> were uninitialized? I think this is the wrong fix (see comments at the call
> site). The header states that these variables will be set if GetDimensions
> returns true. If it returns false, this function has no obligation to
initialize
> them, and more importantly, the caller should not read them.

Acknowledged.

[scroggo, 2015-10-15 13:33:22]

LGTM

[msarett, 2015-10-15 13:34:12]

The CQ bit was checked by msarett@google.com

[msarett, 2015-10-15 13:34:12]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com
Link to the patchset: https://codereview.chromium.org/1400343005/#ps80002
(title: "Response to comments")

[commit-bot: I haz the power, 2015-10-15 13:34:28]

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

[commit-bot: I haz the power, 2015-10-15 13:41:10]

Committed patchset #6 (id:80002) as
https://skia.googlesource.com/skia/+/f724b99435e0c7a6a95112229cc44a2ba813ecc9