Resolve all but two VC++ build warnings in pdfium.

Resolve all but two VC++ build warnings in pdfium.

pdfium builds on Win32 have about 85 warnings (250 in the XFA
branch, totaling over 480 lines!), mostly from four lines in
a header file and a warning that should be disabled. This
change resolves all but two of them and turns on
warning-as-errors. Bugs have been filed for the two
remaining warnings:
https://code.google.com/p/pdfium/issues/detail?id=100
the 64-bit warnings:
https://code.google.com/p/pdfium/issues/detail?id=101
and the Linux warnings:
https://code.google.com/p/pdfium/issues/detail?id=102

The fix to the double->float truncation bugs will also
improve code-generation.

R=bo_xu@foxitsoftware.com, scottmg@chromium.org, tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/d12a4f465a0bcc8b233079ccd54bf7882f3532d5

[brucedawson, 2015-01-02 23:20:37]

brucedawson@chromium.org changed reviewers:
+ bo_xu@foxitsoftware.com

[brucedawson, 2015-01-02 23:20:38]

Any comments on this change?

In particular, do the two remaining warnings point out a serious
integer truncation bug, or can they be safely suppressed with
casts.

[brucedawson, 2015-01-02 23:48:56]

The 64-bit build has a number of __int64 to int32_t truncation warnings, from
icu and from javascript\publicmethods.cpp, so I'm only turning on
warnings-as-errors for 32-bit builds.

[Bo Xu, 2015-01-03 00:00:18]

On 2015/01/02 23:48:56, brucedawson wrote:
> The 64-bit build has a number of __int64 to int32_t truncation warnings, from
> icu and from javascript\publicmethods.cpp, so I'm only turning on
> warnings-as-errors for 32-bit builds.

Let me check with our developers and also the openjpeg developers on this.

[brucedawson, 2015-01-05 22:41:13]

Any word?

The two remaining warnings can either be suppressed with a cast, fixed (whatever
that means), or made non-fatal so that they can be investigated later. I'm happy
with any one of these solutions.

[Bo Xu, 2015-01-05 22:50:45]

On 2015/01/05 22:41:13, brucedawson wrote:
> Any word?
> 
> The two remaining warnings can either be suppressed with a cast, fixed
(whatever
> that means), or made non-fatal so that they can be investigated later. I'm
happy
> with any one of these solutions.

I haven't heard back from them, can we make them non-fatal so we can change them
later?
LGTM with changes here.

[Tom Sepez, 2015-01-05 23:02:57]

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

[Tom Sepez, 2015-01-05 23:02:57]

lgtm

https://codereview.chromium.org/792953005/diff/20001/build/standalone.gypi
File build/standalone.gypi (right):

https://codereview.chromium.org/792953005/diff/20001/build/standalone.gypi#ne...
build/standalone.gypi:188: 'WarnAsError': 'false',
nit: indent +4 ?  Strange, I thought that [ and { would each give +2. But it
looks like the rest of the file doesn't follow this

https://codereview.chromium.org/792953005/diff/20001/build/standalone.gypi#ne...
build/standalone.gypi:191: }
nit: prefer }], on same line.

[brucedawson, 2015-01-06 18:03:32]

brucedawson@chromium.org changed reviewers:
+ scottmg@chromium.org

[brucedawson, 2015-01-06 18:03:32]

Take a look Scott?

https://codereview.chromium.org/792953005/diff/20001/build/standalone.gypi
File build/standalone.gypi (right):

https://codereview.chromium.org/792953005/diff/20001/build/standalone.gypi#ne...
build/standalone.gypi:188: 'WarnAsError': 'false',
There's a sort of logic to having two scoping delimiters on the same line not
doubling their effect. Leaving at an indent of two for consistency.

https://codereview.chromium.org/792953005/diff/20001/build/standalone.gypi#ne...
build/standalone.gypi:191: }
Fixed. And added the missing comma.

https://codereview.chromium.org/792953005/diff/40001/pdfium.gyp
File pdfium.gyp (right):

https://codereview.chromium.org/792953005/diff/40001/pdfium.gyp#newcode314
pdfium.gyp:314: 'WarnAsError': 'false',
Scott -- is there any better way to disable WarnAsError for
fx_codec_jpx_opj.cpp? I don't want to suppress the warning until we are more
certain whether it should be fixed.

I'm okay with this compromise solution (disabling WarnAsError for one project)
but if we can easily do it for one file, better yet.

[scottmg, 2015-01-06 19:48:53]

lgtm

https://codereview.chromium.org/792953005/diff/60001/build/standalone.gypi
File build/standalone.gypi (right):

https://codereview.chromium.org/792953005/diff/60001/build/standalone.gypi#ne...
build/standalone.gypi:188: 'WarnAsError': 'false',
This deserves a comment and a link to a crbug if any.

https://codereview.chromium.org/792953005/diff/60001/core/src/fxge/win32/fx_w...
File core/src/fxge/win32/fx_win32_device.cpp (right):

https://codereview.chromium.org/792953005/diff/60001/core/src/fxge/win32/fx_w...
core/src/fxge/win32/fx_win32_device.cpp:406: #pragma warning(suppress : 4996) //
Suppress deprecation warning
This is globally disabled in Chromium, maybe you want to do that instead.
Otherwise, nit: two spaces before //, period at end.

https://codereview.chromium.org/792953005/diff/60001/pdfium.gyp
File pdfium.gyp (right):

https://codereview.chromium.org/792953005/diff/60001/pdfium.gyp#newcode313
pdfium.gyp:313: # Unresolved warnings in fx_codec_jpx_opj.cpp
Would be good to add a crbug here that contains the list of warnings, and why
they haven't/can't be fixed easily.

[brucedawson, 2015-01-06 21:01:18]

Thanks for the feedback. All issues addressed. I will land now.

https://codereview.chromium.org/792953005/diff/60001/build/standalone.gypi
File build/standalone.gypi (right):

https://codereview.chromium.org/792953005/diff/60001/build/standalone.gypi#ne...
build/standalone.gypi:188: 'WarnAsError': 'false',
On 2015/01/06 19:48:53, scottmg wrote:
> This deserves a comment and a link to a crbug if any.

Done.

https://codereview.chromium.org/792953005/diff/60001/pdfium.gyp
File pdfium.gyp (right):

https://codereview.chromium.org/792953005/diff/60001/pdfium.gyp#newcode313
pdfium.gyp:313: # Unresolved warnings in fx_codec_jpx_opj.cpp
On 2015/01/06 19:48:53, scottmg wrote:
> Would be good to add a crbug here that contains the list of warnings, and why
> they haven't/can't be fixed easily.

Done.

[brucedawson, 2015-01-06 21:12:14]

Committed patchset #5 (id:80001) manually as
d12a4f465a0bcc8b233079ccd54bf7882f3532d5 (presubmit successful).

[Nico, 2015-06-09 15:51:38]

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

[Nico, 2015-06-09 15:51:39]

https://codereview.chromium.org/792953005/diff/80001/pdfium.gyp
File pdfium.gyp (right):

https://codereview.chromium.org/792953005/diff/80001/pdfium.gyp#newcode315
pdfium.gyp:315: 'WarnAsError': 'false',
Please don't add WarnAsError: false anywhere, ever. We want a quiet build, and
WarnAsError makes it possible to add warning noise to the build.

Is this still needed? A build of Chromium seems to still be warning-free, and
this is built as part of Chromium, right?

[Tom Sepez, 2015-06-09 16:01:22]

> Is this still needed? A build of Chromium seems to still be warning-free, and
> this is built as part of Chromium, right?
Yes, needed.  The difference is the third_party/pdfium/build/standalone.gypi
used only when building pdfium by itself turns on a lot more warnings than are
present in the chromium build.  We want to keep these on, since we intend to fix
them, but for the pdfium build only.  I guess this setting should move to
standalone.gypi then.

[Tom Sepez, 2015-06-09 16:02:59]

On 2015/06/09 16:01:22, Tom Sepez wrote:
> > Is this still needed? A build of Chromium seems to still be warning-free,
and
> > this is built as part of Chromium, right?
> Yes, needed.  The difference is the third_party/pdfium/build/standalone.gypi
> used only when building pdfium by itself turns on a lot more warnings than are
> present in the chromium build.  We want to keep these on, since we intend to
fix
> them, but for the pdfium build only.  I guess this setting should move to
> standalone.gypi then.

Which it already is, which implies this shouldn't affect a real chromium build.

[Tom Sepez, 2015-06-09 16:05:16]

On 2015/06/09 16:02:59, Tom Sepez wrote:
> On 2015/06/09 16:01:22, Tom Sepez wrote:
> > > Is this still needed? A build of Chromium seems to still be warning-free,
> and
> > > this is built as part of Chromium, right?
> > Yes, needed.  The difference is the third_party/pdfium/build/standalone.gypi
> > used only when building pdfium by itself turns on a lot more warnings than
are
> > present in the chromium build.  We want to keep these on, since we intend to
> fix
> > them, but for the pdfium build only.  I guess this setting should move to
> > standalone.gypi then.
> 
> Which it already is, which implies this shouldn't affect a real chromium
build.

Which it isn't because I can't read.

[Nico, 2015-06-09 16:16:12]

It's still suboptimal since people copy-paste these files around. The flag has
been there for 5 months, which means chances of progress on this are slim,
right? Could you just disable the "more warnings" until someone actively works
on this again?

[brucedawson, 2015-06-09 17:10:57]

That seems reasonable. We should fix the warnings now, or turn them off.

Worst-case, we can suppress the warnings, enable warnings-as-errors, and we'll
be slightly better off than we are now, and we'll have the bugs to remind us to
re-enable the warnings and fix them later.