734382 - Don't show the list of activities if there is only one

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Follow-up from bug 730289. Quite simple actually.

Comment 1

[Doug Turner (:dougt)]

Comment on attachment 604404 [details] [diff] [review]
Patch v1

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

brad requested this follow.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 604404 [details] [diff] [review]
Patch v1

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

code looks good, but I'd like to see a new patch with the refactor I describe below.

::: mobile/android/base/GeckoApp.java
@@ +2485,5 @@
> +            }
> +
> +            intent = intents.get(itemId);
> +        } else {
> +            intent = intents.get(0);

my preference would be to factor this code out into a helper function, getFilePickerIntent(String mimeType)

In that helper function, 0 and 1 possible intents can be early returns.

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 605714 [details] [diff] [review]
Patch v2

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

::: js/src/config/check-sync-dirs.py
@@ +1,1 @@
> +#!/usr/bin/env python2

I have no idea if there is some js style guide against this. Probably easiest just to drop this change.

::: mobile/android/base/GeckoApp.java
@@ +2459,5 @@
>          ArrayList<Intent> intents = new ArrayList<Intent>();
>          PromptService.PromptListItem[] items = getItemsAndIntentsForFilePicker(aMimeType, intents);
>  
> +        if (intents.size() == 0) {
> +            Log.e(LOGTAG, "no activities for the file picker!");

this isn't really an error. Log.i would be better

@@ +2465,5 @@
> +        }
> +
> +        if (intents.size() == 1) {
> +            return intents.get(0);
> +        }

nit, no curly braces for a one line if statement

Comment 5

[Mounir Lamouri (:mounir)]

(In reply to Brad Lassey [:blassey] from comment #4)
> ::: js/src/config/check-sync-dirs.py
> @@ +1,1 @@
> > +#!/usr/bin/env python2
> 
> I have no idea if there is some js style guide against this. Probably
> easiest just to drop this change.

That thing had nothing to do in that patch actually. Sorry and thank you for noticing ;)

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7e8278f5e814

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

I backed this out along with all the other patches in the initial push. Something in the push regressed Ts on Android by 20% and I don't know which patch is to blame:

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

Regression Ts increase 20.6% on Android 2.2 Mozilla-Inbound
--------------------------------------------------------------
    Previous: avg 2653.656 stddev 87.856 of 30 runs up to revision 1239bd0779a6
    New     : avg 3201.178 stddev 110.202 of 5 runs since revision 0d61a0d8dba4
    Change  : +547.522 (20.6% / z=6.232)
    Graph   : http://mzl.la/zD3EWy

Comment 8

[Marco Bonardo [:mak]]

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

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ec0e723221ae