Last Comment Bug 734382 - Don't show the list of activities if there is only one
: Don't show the list of activities if there is only one
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: mozilla14
Assigned To: Mounir Lamouri (:mounir)
:
: Jim Chen [:jchen] [:darchons]
Mentors:
Depends on:
Blocks: 730289
  Show dependency treegraph
 
Reported: 2012-03-09 07:06 PST by Mounir Lamouri (:mounir)
Modified: 2012-03-24 08:59 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Patch v1 (3.02 KB, patch)
2012-03-09 07:06 PST, Mounir Lamouri (:mounir)
blassey.bugs: review-
Details | Diff | Splinter Review
Patch v2 (3.54 KB, patch)
2012-03-14 04:36 PDT, Mounir Lamouri (:mounir)
blassey.bugs: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2012-03-09 07:06:23 PST
Created attachment 604404 [details] [diff] [review]
Patch v1

Follow-up from bug 730289. Quite simple actually.
Comment 1 Doug Turner (:dougt) 2012-03-09 07:07:29 PST
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?) 2012-03-09 10:06:04 PST
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) 2012-03-14 04:36:28 PDT
Created attachment 605714 [details] [diff] [review]
Patch v2
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2012-03-14 08:58:04 PDT
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) 2012-03-14 11:07:50 PDT
(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) 2012-03-17 17:12:17 PDT
https://hg.mozilla.org/mozilla-central/rev/7e8278f5e814
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2012-03-17 19:00:23 PDT
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] 2012-03-20 03:55:11 PDT
https://hg.mozilla.org/mozilla-central/rev/c47e0e62d6d0
Comment 9 Mark Finkle (:mfinkle) (use needinfo?) 2012-03-24 08:59:45 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/ec0e723221ae

Note You need to log in before you can comment on or make changes to this bug.