Fix behaviour of app.alert() with a single object argument.

Fix behaviour of app.alert() with a single object argument.

Centralize the "arguments in an object" logic.  See the section
"Method Arguments" in the js_api_reference.pdf.  Add some tests
to hit the ambiguous cases this section implies, and test that
all parameters are passed end-to-end to our callbacks.

R=thestig@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/66519af52b61ca158044651d0507d47efb364f87

[Tom Sepez, 2016-01-27 23:59:53]

Lei, for review.

This came up when I tried to app.alert(this.getField()) in another test, and got
"Alert: undefined" even though the getField() call was returning an object.

The new code will at least throw an exception in this case,
since alerting an object that doesn't have well-known keyword fields is
delightfully ambiguous (other readers throw).

[Lei Zhang, 2016-01-28 02:11:11]

https://codereview.chromium.org/1641693003/diff/1/fpdfsdk/src/javascript/JS_V...
File fpdfsdk/src/javascript/JS_Value.cpp (right):

https://codereview.chromium.org/1641693003/diff/1/fpdfsdk/src/javascript/JS_V...
fpdfsdk/src/javascript/JS_Value.cpp:896: if (originals.size() != 1 ||
originals[0].GetType() != CJS_Value::VT_object ||
Wouldn't we go out of bounds if |originals| is empty?

https://codereview.chromium.org/1641693003/diff/1/fpdfsdk/src/javascript/JS_V...
fpdfsdk/src/javascript/JS_Value.cpp:897: originals[0].IsArrayObject())
braces?

https://codereview.chromium.org/1641693003/diff/1/fpdfsdk/src/javascript/JS_V...
File fpdfsdk/src/javascript/JS_Value.h (right):

https://codereview.chromium.org/1641693003/diff/1/fpdfsdk/src/javascript/JS_V...
fpdfsdk/src/javascript/JS_Value.h:216: // Some JS methods have the bizzare
convention that they may also be called
bizarre

https://codereview.chromium.org/1641693003/diff/1/fpdfsdk/src/javascript/JS_V...
fpdfsdk/src/javascript/JS_Value.h:224: const std::vector<CJS_Value>&
original_params,
|original_params| -> |originals| to match the impl.

[Tom Sepez, 2016-01-28 17:07:37]

https://codereview.chromium.org/1641693003/diff/1/fpdfsdk/src/javascript/JS_V...
File fpdfsdk/src/javascript/JS_Value.cpp (right):

https://codereview.chromium.org/1641693003/diff/1/fpdfsdk/src/javascript/JS_V...
fpdfsdk/src/javascript/JS_Value.cpp:896: if (originals.size() != 1 ||
originals[0].GetType() != CJS_Value::VT_object ||
On 2016/01/28 02:11:11, Lei Zhang wrote:
> Wouldn't we go out of bounds if |originals| is empty?

I'm counting on the originals.size() != 1 check to short-circuit before the [0]
is applied.

https://codereview.chromium.org/1641693003/diff/1/fpdfsdk/src/javascript/JS_V...
fpdfsdk/src/javascript/JS_Value.cpp:897: originals[0].IsArrayObject())
On 2016/01/28 02:11:11, Lei Zhang wrote:
> braces?
Yup.

https://codereview.chromium.org/1641693003/diff/1/fpdfsdk/src/javascript/JS_V...
File fpdfsdk/src/javascript/JS_Value.h (right):

https://codereview.chromium.org/1641693003/diff/1/fpdfsdk/src/javascript/JS_V...
fpdfsdk/src/javascript/JS_Value.h:216: // Some JS methods have the bizzare
convention that they may also be called
On 2016/01/28 02:11:11, Lei Zhang wrote:
> bizarre

Done.

https://codereview.chromium.org/1641693003/diff/1/fpdfsdk/src/javascript/JS_V...
fpdfsdk/src/javascript/JS_Value.h:224: const std::vector<CJS_Value>&
original_params,
On 2016/01/28 02:11:11, Lei Zhang wrote:
> |original_params| -> |originals| to match the impl.

Done.

[Lei Zhang, 2016-01-28 22:58:55]

lgtm

https://codereview.chromium.org/1641693003/diff/1/fpdfsdk/src/javascript/JS_V...
File fpdfsdk/src/javascript/JS_Value.cpp (right):

https://codereview.chromium.org/1641693003/diff/1/fpdfsdk/src/javascript/JS_V...
fpdfsdk/src/javascript/JS_Value.cpp:896: if (originals.size() != 1 ||
originals[0].GetType() != CJS_Value::VT_object ||
On 2016/01/28 17:07:36, Tom Sepez wrote:
> On 2016/01/28 02:11:11, Lei Zhang wrote:
> > Wouldn't we go out of bounds if |originals| is empty?
> 
> I'm counting on the originals.size() != 1 check to short-circuit before the
[0]
> is applied.

Ya, I was being stupid. Ignore.

[Tom Sepez, 2016-01-29 16:55:34]

Description was changed from

==========
Fix behaviour of app.alert() with a single object argument.

Centralize the "arguments in an object" logic.  See the section
"Method Arguments" in the js_api_reference.pdf.  Add some tests
to hit the ambiguous cases this section implies, and test that
all parameters are passed end-to-end to our callbacks.

R=thestig@chromium.org
==========

to

==========
Fix behaviour of app.alert() with a single object argument.

Centralize the "arguments in an object" logic.  See the section
"Method Arguments" in the js_api_reference.pdf.  Add some tests
to hit the ambiguous cases this section implies, and test that
all parameters are passed end-to-end to our callbacks.

R=thestig@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/66519af52b61ca158044651d0507d47efb36...
==========

[Tom Sepez, 2016-01-29 16:55:35]

Committed patchset #2 (id:20001) manually as
66519af52b61ca158044651d0507d47efb364f87 (presubmit successful).