1014965 - NativeJSObject.optString does the wrong thing on "key":null

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him] (PTO August 1-10)]

Per http://stackoverflow.com/a/23377941, JSONObject.optString on "key":null returns "null".  NativeJSObject.optString throws an IllegalArgumentException (which is caught and *ignored* in the event handling, which is awful but not this bug).  Like Android, we need to be bug-compatible, even though it sucks.

Comment 1

[Nick Alexander :nalexander [he/him] (PTO August 1-10)]

While we're at it, NativeJSObject doesn't implement isNull, making it impossible to work-around.

Comment 2

[Mark Capella [:capella]]

Attachment: bug1014965.diff Implement isNull()

Implement isNull() for NativeJSObject, add tests ...

Comment 3

[Nick Alexander :nalexander [he/him] (PTO August 1-10)]

Comment on attachment 8428724 [details] [diff] [review]
bug1014965.diff Implement isNull()

Review of attachment 8428724 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, but I'm going to redirect to jchen.

::: mobile/android/base/tests/testEventDispatcher.java
@@ +294,5 @@
>              null, object.optStringArray("stringArray", null));
>          fAssertSame("optStringArray returns fallback value if nonexistent",
>              null, object.optStringArray("nonexistent_stringArray", null));
> +
> +        fAssertEquals("Native (undefined) is null", true, object.isNull("isNullTest0"));

Can we include a" has" test for these first two (or all four) to make it really clear how the first two act?

Comment 4

[Mark Capella [:capella]]

Attachment: bug1014965.diff Implement isNull()

Sure :)

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8428761 [details] [diff] [review]
bug1014965.diff Implement isNull()

Review of attachment 8428761 [details] [diff] [review]:
-----------------------------------------------------------------

Instead of adding isNull, can we treat null as undefined in GetProperty? See https://mail.mozilla.org/pipermail/mobile-firefox-dev/2014-May/000683.html

Comment 6

[Mark Capella [:capella]]

Attachment: bug1014965.diff

mmmm .... that seems to make sense :-)

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8429371 [details] [diff] [review]
bug1014965.diff

Review of attachment 8429371 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Thanks!

::: mobile/android/base/tests/testEventDispatcher.java
@@ +311,5 @@
> +
> +        fAssertEquals("Native optDouble returns fallback value if undefined",
> +            -3.1415926535, object.optDouble("undefined", -3.1415926535));
> +        fAssertEquals("Native optDouble returns fallback value if null",
> +            -3.1415926535, object.optDouble("null", -3.1415926535));

I tend to use numbers that have exact floating point representation (e.g. 0.5, 1.75, etc.) to avoid possible subtle bugs, but it's not a big deal.

@@ +318,5 @@
> +            "chromodynamics", object.optString("undefined", "chromodynamics"));
> +        fAssertEquals("Native optString returns fallback value if null",
> +            "chromodynamics", object.optString("null", "chromodynamics"));
> +        fAssertNotSame("Native optString does not return fallback value if emptyString",
> +            "chromodynamics", object.optString("emptyString", "chromodynamics"));

fAssertNotEquals. And let's stick with "foo" and "bar" :)

Comment 8

[Mark Capella [:capella]]

TRY: https://tbpl.mozilla.org/?tree=Try&rev=4fe70a19725

Comment 9

[Mark Capella [:capella]]

Attachment: bug1014965.diff

Re-requesting review ... I had to remove the test for the "undefined" var as the push-to-try failed, pointing out that it won't pass the |message.toBundle();| portion of the test.

Comment 10

[Mark Capella [:capella]]

re-push to try passed ok
https://tbpl.mozilla.org/?tree=Try&rev=fcbab0b69168

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8430198 [details] [diff] [review]
bug1014965.diff

Review of attachment 8430198 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

Comment 12

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/baf6ec3031f5

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/baf6ec3031f5