1319496 - Convert prompts to BundleEventListener

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Convert prompts to use GeckoBundle and BundleEventListener.

Comment 1

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

Attachment: 1. Fix GeckoBundle array handling (v1)

Fix several bugs when handling arrays in GeckoBundle.

1. Correctly return null when getting an array that is not in the
   bundle, instead of crashing.

2. Convert object arrays to GeckoBundle arrays in EventDispatcher
   instead of leaving it as a single GeckoBundle with integer keys, due
   to lack of object array support in NativeJSObject.toBundle.

3. Return error when trying to convert a JS array of arrays to
   GeckoBundle, instead of crashing.

4. Add convenience methods for setting arrays; for example, setting
   boolean arrays from Boolean[] and Collection<Boolean>.

Comment 2

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

Attachment: 2. Add more GeckoBundle tests in testEventDispatcher (v1)

Add more tests for edge cases in testEventDispatcher, such as null
arrays, nonexistent values, and object arrays containing only nulls.

Comment 3

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

Attachment: 3. Convert prompts to BundleEventListener (v1)

Convert prompts to use BundleEventListener and GeckoBundle.

DefaultDoorHanger.setOptions accepts a JSONObject argument, but if we
converted it to GeckoBundle, it would involve a lot of extra changes in
the other doorhanger code. So this patch adds GeckoBundle.fromJSONObject
and converts JSONObject to GeckoBundle within
DefaultDoorHanger.setOptions. In the future, another patch would convert
all doorhanger code to use GeckoBundle instead of JSONObject.

Comment 4

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

Attachment: 4. Fix context menu item IDs (v1)

Context menu items used UUIDs as their prompt list item IDs. However,
prompt list items only support integers as IDs. This error didn't show
up before because JSONObject was silently ignoring the error. This patch
changes to using an incremental integer as the ID and fixes the error.

Comment 5

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

Attachment: 5. Change prompt response to use GeckoBundle (v1)

Change prompt response from using JSONObject/String to using
GeckoBundle. The GeckoBundle is automatically translated to a JS object,
like before, when dispatched to JS code.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8814955 [details] [diff] [review]
3. Convert prompts to BundleEventListener (v1)

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/GeckoBundle.java
@@ +631,5 @@
> +        if (value instanceof JSONArray) {
> +            final JSONArray array = (JSONArray) value;
> +            final int len = array.length();
> +            if (len == 0) {
> +                return EMPTY_BOOLEAN_ARRAY;

An empty JSON array will always be turned into an empty Java boolean array?

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8814957 [details] [diff] [review]
5. Change prompt response to use GeckoBundle (v1)

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

::: mobile/android/base/java/org/mozilla/gecko/delegates/BookmarkStateChangeDelegate.java
@@ +165,5 @@
>  
> +        if (tab == null) {
> +            return;
> +        }
> +

Oh yeah, makes sense to check this before showing the prompt.

@@ +177,5 @@
>                      Telemetry.sendUIEvent(TelemetryContract.Event.ACTION,
>                              TelemetryContract.Method.DIALOG, extrasId);
>  
>                      new EditBookmarkDialog(browserApp).show(tab.getURL());
> +

nit: Unrelated new line?

Comment 8

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

(In reply to Sebastian Kaspari (:sebastian) from comment #6)
> Comment on attachment 8814955 [details] [diff] [review]
> 3. Convert prompts to BundleEventListener (v1)
> 
> Review of attachment 8814955 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > +            if (len == 0) {
> > +                return EMPTY_BOOLEAN_ARRAY;
> 
> An empty JSON array will always be turned into an empty Java boolean array?

Yep, GeckoBundle represents empty JS arrays as empty boolean arrays. It automatically performs conversions to other types of empty arrays if needed.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cf56f4e7c0d
1. Fix GeckoBundle array handling; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/15407fdf9c66
2. Add more GeckoBundle tests in testEventDispatcher; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/10a6565daede
3. Convert prompts to BundleEventListener; r=sebastian
https://hg.mozilla.org/integration/mozilla-inbound/rev/16dbb57af3e2
4. Fix context menu item IDs; r=sebastian
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e8406f38c09
5. Change prompt response to use GeckoBundle; r=sebastian

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7cf56f4e7c0d
https://hg.mozilla.org/mozilla-central/rev/15407fdf9c66
https://hg.mozilla.org/mozilla-central/rev/10a6565daede
https://hg.mozilla.org/mozilla-central/rev/16dbb57af3e2
https://hg.mozilla.org/mozilla-central/rev/5e8406f38c09