XFA: merge patch from CL 792953005, fix most VC++ warnings

XFA: merge patch from CL 792953005, fix most warnings

Includes fixes to XFA specific warnings -- benign truncations.
Bug https://code.google.com/p/pdfium/issues/detail?id=104
was filed to track changing types to avoid some truncations.

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, tsepez@chromium.org

Review URL: https://codereview.chromium.org/792953005

BUG= https://code.google.com/p/pdfium/issues/detail?id=100

Committed: https://pdfium.googlesource.com/pdfium/+/3f49aa3ba3e8ba2fcedc8e2e2a88f3b06efd52b9

[brucedawson, 2015-01-06 21:33:41]

I had to make XFA specific fixes to fsdk_mgr.h, fpdfformfill.cpp, fsdk_mgr.cpp,
and xfa_localevalue.cpp. Nothing big, but I figured an actual review of those
changes was a good idea.

I'll be very pleased to have the fsdk_mgr.h warnings fixed because they are
*extremely* noisy.

[Bo Xu, 2015-01-06 22:24:29]

On 2015/01/06 21:33:41, brucedawson wrote:
> I had to make XFA specific fixes to fsdk_mgr.h, fpdfformfill.cpp,
fsdk_mgr.cpp,
> and xfa_localevalue.cpp. Nothing big, but I figured an actual review of those
> changes was a good idea.
> 
> I'll be very pleased to have the fsdk_mgr.h warnings fixed because they are
> *extremely* noisy.

I don't know why we used double in fsdk_mgr.cpp and other files for the pdf
coordinates and object location.
The basic structure like core/include/fxcrt/fx_coordinates.h and FS_RECTF all
use float.
At least for the argument of the call back functions in fsdk_mgr.cpp, changing
their type to FX_FLOAT would be OK.

[Bo Xu, 2015-01-06 22:30:39]

https://codereview.chromium.org/834413002/diff/1/xfa/src/fxfa/src/parser/xfa_...
File xfa/src/fxfa/src/parser/xfa_localevalue.cpp (right):

https://codereview.chromium.org/834413002/diff/1/xfa/src/fxfa/src/parser/xfa_...
xfa/src/fxfa/src/parser/xfa_localevalue.cpp:583: FX_WORD wYear = 0, wMonth = 0,
wDay = 0;
declare wMonth and wDay as "FX_BYTE"?

[brucedawson, 2015-01-07 21:14:45]

I changed them to float (FX_FLOAT is not available), which does make for a
cleaner fix, but it means that when we roll pdfium to Chrome we'll have to
update pdfium_engine.*. Is it worthwhile?

[brucedawson, 2015-01-07 21:17:28]

We could do that. However I noticed that the atoi code for wMonth and wDay
doesn't have any range checking. Changing the type of wMonth and wDay would make
wraparound even easier to hit and would make adding overflow detection harder,
so it might end up being a step backwards.

To be clear: with wMonth and wDay at their current size it is easy to add
overflow detection by just checking for out-of-range in the atoi loop. With the
type changed to FX_BYTE then for wMonth in particular it gets trickier since
30*10+n will overflow without easy detection.

[brucedawson, 2015-01-08 18:16:54]

Bo, what do you think? Would Tom have better insights into the relative costs of
changing the callback interface between Chrome and pdfium?

[Bo Xu, 2015-01-08 18:23:54]

On 2015/01/08 18:16:54, brucedawson wrote:
> Bo, what do you think? Would Tom have better insights into the relative costs
of
> changing the callback interface between Chrome and pdfium?

Sure. I think thestig@ and tom should have more insight on whether we should
unify the pdf coordinate and page size to float or double. Can you add them as
reviewers too? Thanks :)

[brucedawson, 2015-01-08 18:30:22]

brucedawson@chromium.org changed reviewers:
+ thestig@chromium.org, tsepez@chromium.org

[brucedawson, 2015-01-08 18:30:22]

Tom/Lei, in merging my warning fixes to XFA I hit some double-to-float
truncation warnings. These can easily be suppressed (cast, warning suppression)
or actually fixed, by changing the types from double to float. However the types
are used in the interface between pdfium and Chrome. And, if we change some
doubles to float then arguably we should change all of them, and then this
starts to feel like a sweater thread.

The casts in question are in fpdfsdk/include/fsdk_mgr.h. There are only four of
them so I'm leaning towards leaving the casts and then filing a bug to remind us
to clean up all of the doubles later.

[Bo Xu, 2015-01-08 18:36:24]

https://codereview.chromium.org/834413002/diff/1/xfa/src/fxfa/src/parser/xfa_...
File xfa/src/fxfa/src/parser/xfa_localevalue.cpp (right):

https://codereview.chromium.org/834413002/diff/1/xfa/src/fxfa/src/parser/xfa_...
xfa/src/fxfa/src/parser/xfa_localevalue.cpp:584: FX_LPCWSTR pDate =
(FX_LPCWSTR)wsDate;
This (FX_LPCWSTR) should be changed to c_str()?

[Tom Sepez, 2015-01-08 18:43:53]

On 2015/01/08 18:30:22, brucedawson wrote:
> Tom/Lei, in merging my warning fixes to XFA I hit some double-to-float
> truncation warnings. These can easily be suppressed (cast, warning
suppression)
> or actually fixed, by changing the types from double to float. However the
types
> are used in the interface between pdfium and Chrome. And, if we change some
> doubles to float then arguably we should change all of them, and then this
> starts to feel like a sweater thread.
> 
> The casts in question are in fpdfsdk/include/fsdk_mgr.h. There are only four
of
> them so I'm leaning towards leaving the casts and then filing a bug to remind
us
> to clean up all of the doubles later.

LGTM, I'm ok with the casts.

[Tom Sepez, 2015-01-08 18:47:50]

https://codereview.chromium.org/834413002/diff/1/fpdfsdk/include/fsdk_mgr.h
File fpdfsdk/include/fsdk_mgr.h (right):

https://codereview.chromium.org/834413002/diff/1/fpdfsdk/include/fsdk_mgr.h#n...
fpdfsdk/include/fsdk_mgr.h:480: dstRect.top = static_cast<float>(top < bottom?
bottom:top);
nit: might as well fix the spacing before |?| and before and after |:|.

[brucedawson, 2015-01-08 18:54:47]

BTW, I should have clarified that the casts *were* in fsdk_mgr.h. The current
version changes the interface with chrome as an alternate way of fixing them.

It sounds like the safest fix is to return to the casts. And fix the spacing.

https://codereview.chromium.org/834413002/diff/1/xfa/src/fxfa/src/parser/xfa_...
File xfa/src/fxfa/src/parser/xfa_localevalue.cpp (right):

https://codereview.chromium.org/834413002/diff/1/xfa/src/fxfa/src/parser/xfa_...
xfa/src/fxfa/src/parser/xfa_localevalue.cpp:584: FX_LPCWSTR pDate =
(FX_LPCWSTR)wsDate;
Yes, it should be changed. My only question would be whether it is okay to make
this change as part of this CL, given that it's not really related.

[Tom Sepez, 2015-01-08 18:57:57]

> Yes, it should be changed. My only question would be whether it is okay to
make
> this change as part of this CL, given that it's not really related.
I'd vote for follow-on CLs rather than this one.

[Bo Xu, 2015-01-08 19:16:56]

LGTM

https://codereview.chromium.org/834413002/diff/1/xfa/src/fxfa/src/parser/xfa_...
File xfa/src/fxfa/src/parser/xfa_localevalue.cpp (right):

https://codereview.chromium.org/834413002/diff/1/xfa/src/fxfa/src/parser/xfa_...
xfa/src/fxfa/src/parser/xfa_localevalue.cpp:584: FX_LPCWSTR pDate =
(FX_LPCWSTR)wsDate;
On 2015/01/08 18:54:47, brucedawson wrote:
> Yes, it should be changed. My only question would be whether it is okay to
make
> this change as part of this CL, given that it's not really related.

Agree.

[brucedawson, 2015-01-08 19:47:54]

Committed patchset #3 (id:40001) manually as
3f49aa3ba3e8ba2fcedc8e2e2a88f3b06efd52b9 (presubmit successful).