Closed Bug 1319496 Opened 3 years ago Closed 3 years ago

Convert prompts to BundleEventListener

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(5 files)

Convert prompts to use GeckoBundle and BundleEventListener.
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>.
Attachment #8814953 - Flags: review?(snorp)
Add more tests for edge cases in testEventDispatcher, such as null
arrays, nonexistent values, and object arrays containing only nulls.
Attachment #8814954 - Flags: review?(snorp)
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.
Attachment #8814955 - Flags: review?(s.kaspari)
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.
Attachment #8814956 - Flags: review?(s.kaspari)
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.
Attachment #8814957 - Flags: review?(s.kaspari)
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?
Attachment #8814955 - Flags: review?(s.kaspari) → review+
Attachment #8814956 - Flags: review?(s.kaspari) → review+
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?
Attachment #8814957 - Flags: review?(s.kaspari) → review+
Attachment #8814953 - Flags: review?(snorp) → review+
Attachment #8814954 - Flags: review?(snorp) → review+
(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.
Pushed by nchen@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.