Bugs in CJS_PublicMethods::ParseNumber()

Bugs in CJS_PublicMethods::ParseNumber().

Fix the bugs by removing ParseNumber() entirely.

For PDFium's JavaScript bindings, we want to get out of the
numeric conversion business and inflict that on V8 as
possible, avoiding platform-specific issue in strtod().

For other uses, there is a FX_atof() which is similarly
buggy, but we can consolidate the use.  Add an overloaded
FX_atof() to handle wide strings more simply.

BUG=pdfium:361
R=jochen@chromium.org

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

[Tom Sepez, 2016-01-15 21:31:12]

Description was changed from

==========
Reland (re-worked) Cleanup CJS_PublicMethods::ParseNumber
==========

to

==========
Reland (re-worked) Cleanup CJS_PublicMethods::ParseNumber

BUG=pdfium:361
==========

[Tom Sepez, 2016-01-15 21:31:12]

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

[Tom Sepez, 2016-01-15 21:34:23]

Description was changed from

==========
Reland (re-worked) Cleanup CJS_PublicMethods::ParseNumber

BUG=pdfium:361
==========

to

==========
Bugs in CJS_PublicMethods::ParseNumber().

For PDFium's JavaScript bindings, we want to get out of the
numeric conversion business and inflict that on V8 as
possible, avoiding platform-specific issue in strtod().

This is a partial fix for the issue at hand.

BUG=pdfium:361
==========

[Tom Sepez, 2016-01-15 21:34:46]

Jochen, pls review the usage of V8.

[Tom Bergan, 2016-01-15 21:49:52]

This is much cleaner than parsing strings.

https://codereview.chromium.org/1586203006/diff/1/testing/resources/javascrip...
File testing/resources/javascript/bug_361_expected.txt (right):

https://codereview.chromium.org/1586203006/diff/1/testing/resources/javascrip...
testing/resources/javascript/bug_361_expected.txt:28: Alert: Answer for "NaN"
is: string NaN
This seems wrong ... we pass a literal NaN (not "NaN"), and the output is "NaN",
not NaN. Looks like this is due to the std::isnan check in MaybeCoerceNumber.
Are you mimicking Acrobat behavior here, or is there some other reason for the
std::isnan check?

[Tom Sepez, 2016-01-15 22:53:07]

On 2016/01/15 21:49:52, Tom Bergan wrote:
> This is much cleaner than parsing strings.
> 
>
https://codereview.chromium.org/1586203006/diff/1/testing/resources/javascrip...
> File testing/resources/javascript/bug_361_expected.txt (right):
> 
>
https://codereview.chromium.org/1586203006/diff/1/testing/resources/javascrip...
> testing/resources/javascript/bug_361_expected.txt:28: Alert: Answer for "NaN"
> is: string NaN
> This seems wrong ... we pass a literal NaN (not "NaN"), and the output is
"NaN",
> not NaN. Looks like this is due to the std::isnan check in MaybeCoerceNumber.
> Are you mimicking Acrobat behavior here, or is there some other reason for the
> std::isnan check?

V8 will convert V8 arrays, etc to NaN, which is unfortunate.  Its kind of an
alternate error indication in addition to the maybe<> stuff.  I think we should
however give the string "NaN" a pass and promote it to NaN.

[Tom Sepez, 2016-01-15 23:03:13]

Description was changed from

==========
Bugs in CJS_PublicMethods::ParseNumber().

For PDFium's JavaScript bindings, we want to get out of the
numeric conversion business and inflict that on V8 as
possible, avoiding platform-specific issue in strtod().

This is a partial fix for the issue at hand.

BUG=pdfium:361
==========

to

==========
Bugs in CJS_PublicMethods::ParseNumber().

Fix the bugs by removing ParseNumber() entirely.

For PDFium's JavaScript bindings, we want to get out of the
numeric conversion business and inflict that on V8 as
possible, avoiding platform-specific issue in strtod().

For other uses, there is a FX_atof() which is similarly
buggy, but we can consolidate the use.

BUG=pdfium:361
==========

[Tom Sepez, 2016-01-15 23:03:58]

Description was changed from

==========
Bugs in CJS_PublicMethods::ParseNumber().

Fix the bugs by removing ParseNumber() entirely.

For PDFium's JavaScript bindings, we want to get out of the
numeric conversion business and inflict that on V8 as
possible, avoiding platform-specific issue in strtod().

For other uses, there is a FX_atof() which is similarly
buggy, but we can consolidate the use.

BUG=pdfium:361
==========

to

==========
Bugs in CJS_PublicMethods::ParseNumber().

Fix the bugs by removing ParseNumber() entirely.

For PDFium's JavaScript bindings, we want to get out of the
numeric conversion business and inflict that on V8 as
possible, avoiding platform-specific issue in strtod().

For other uses, there is a FX_atof() which is similarly
buggy, but we can consolidate the use.  Add an overloaded
FX_atof() to handle wide strings more simply.

BUG=pdfium:361
==========

[Tom Sepez, 2016-01-15 23:07:45]

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

[Tom Sepez, 2016-01-15 23:08:03]

+Lei for the fx_string stuff, etc.

[jochen (gone - plz use gerrit), 2016-01-19 17:16:25]

https://codereview.chromium.org/1586203006/diff/40001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/JS_Value.h (right):

https://codereview.chromium.org/1586203006/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/JS_Value.h:68: void MaybeCoerceToNumber();
is it ok if the number has values like +infinity or -0?

https://codereview.chromium.org/1586203006/diff/40001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/PublicMethods.cpp (right):

https://codereview.chromium.org/1586203006/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/PublicMethods.cpp:1718: ws.Replace(L",", L".");
that's kinda odd, why is this needed? wouldn't that turn e.g. 1,000.42 (one
thousand point fortytwo) into something not a number?

[Charles Lew, 2016-01-19 18:01:45]

https://codereview.chromium.org/1586203006/diff/40001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/PublicMethods.cpp (right):

https://codereview.chromium.org/1586203006/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/PublicMethods.cpp:1722: vRet = 0;
Actually the Adobe implementation of these functions (and all the other AF*
functions), which are written in Javascript, are directly accessible from the
console of the Acrobat application. Maybe could use that for the maximum
interoperability.
But not sure whether it is allowed in the license to do so though.
----* the following are the output of evaluating "AFMakeNumber" *----

function AFMakeNumber(string)
{	/* attempts to make a number out of a string that may not use '.' as the
	 * seperator; it expects that the number is fairly well-behaved other than
	 * possibly having a non-JavaScript friendly separator */
	var type = typeof string;

	if (type == "number")
		return string;
	if (type != "string")
		return null;

	var array = AFExtractNums(string);

	if(array)
	{
		var joined = array.join(".");

		if (string.indexOf("-.") >= 0)
			joined = "0." + joined;
		return joined * (string.indexOf("-") >= 0 ? -1.0 : 1.0);
	}
	else
		return null;
}

[Tom Sepez, 2016-01-19 22:16:31]

On 2016/01/19 17:16:25, jochen wrote:
>
https://codereview.chromium.org/1586203006/diff/40001/fpdfsdk/src/javascript/...
> File fpdfsdk/src/javascript/JS_Value.h (right):
> 
>
https://codereview.chromium.org/1586203006/diff/40001/fpdfsdk/src/javascript/...
> fpdfsdk/src/javascript/JS_Value.h:68: void MaybeCoerceToNumber();
> is it ok if the number has values like +infinity or -0?
> 
>
https://codereview.chromium.org/1586203006/diff/40001/fpdfsdk/src/javascript/...
> File fpdfsdk/src/javascript/PublicMethods.cpp (right):
> 
>
https://codereview.chromium.org/1586203006/diff/40001/fpdfsdk/src/javascript/...
> fpdfsdk/src/javascript/PublicMethods.cpp:1718: ws.Replace(L",", L".");
> that's kinda odd, why is this needed? wouldn't that turn e.g. 1,000.42 (one
> thousand point fortytwo) into something not a number?

Either way, 1,000.42 or 1.000.42 won't parse, and we return it in string form.
"1,000" is delightfully ambiguous, I'm going to go with the european
interpretation.

[Tom Sepez, 2016-01-19 22:19:16]

https://codereview.chromium.org/1586203006/diff/40001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/JS_Value.h (right):

https://codereview.chromium.org/1586203006/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/JS_Value.h:68: void MaybeCoerceToNumber();
On 2016/01/19 17:16:25, jochen wrote:
> is it ok if the number has values like +infinity or -0?

Probably.  Remember, there isn't really a spec for this.

https://codereview.chromium.org/1586203006/diff/40001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/PublicMethods.cpp (right):

https://codereview.chromium.org/1586203006/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/PublicMethods.cpp:1718: ws.Replace(L",", L".");
On 2016/01/19 17:16:25, jochen wrote:
> that's kinda odd, why is this needed? wouldn't that turn e.g. 1,000.42 (one
> thousand point fortytwo) into something not a number?

We don't know if we're in europe so we make a guess that numbers don't have
commas in them every 3 digits (which isn't handled in other products either).

[jochen (gone - plz use gerrit), 2016-01-20 12:29:50]

v8 usage lgtm

[Tom Sepez, 2016-01-20 19:34:08]

Description was changed from

==========
Bugs in CJS_PublicMethods::ParseNumber().

Fix the bugs by removing ParseNumber() entirely.

For PDFium's JavaScript bindings, we want to get out of the
numeric conversion business and inflict that on V8 as
possible, avoiding platform-specific issue in strtod().

For other uses, there is a FX_atof() which is similarly
buggy, but we can consolidate the use.  Add an overloaded
FX_atof() to handle wide strings more simply.

BUG=pdfium:361
==========

to

==========
Bugs in CJS_PublicMethods::ParseNumber().

Fix the bugs by removing ParseNumber() entirely.

For PDFium's JavaScript bindings, we want to get out of the
numeric conversion business and inflict that on V8 as
possible, avoiding platform-specific issue in strtod().

For other uses, there is a FX_atof() which is similarly
buggy, but we can consolidate the use.  Add an overloaded
FX_atof() to handle wide strings more simply.

BUG=pdfium:361
R=jochen@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/f13d510cf267c27f4c123494de67670ec201...
==========

[Tom Sepez, 2016-01-20 19:34:09]

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