1192788 - Can't install add-on panel at position 0

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

When creating an add-on that creates a home panel and specifying position 0 then the panel is added as last item to the list.

Comment 1

[:Margaret Leibovic]

I probably did something dumb in bug 1160346...

Comment 2

[:Margaret Leibovic]

Aha, options.position is falsy if it is 0:
http://hg.mozilla.org/mozilla-central/rev/43512d4631b7#l2.31

To fix this bug we should be more explicit and check options.position !== undefined:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Home.jsm#384

Comment 3

[Heiko Reese]

Attachment: Possible fix

Possible fix. I can't confirm it because I don't have an add-on that triggers the bug.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8648386 [details] [diff] [review]
Possible fix

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

Thank you for the patch Heiko!

I tested it with the speed-dial sample add-on[1] (Set position to 0 in bootstrap.js) and it works.

[1] https://github.com/pocmo/speed-dial

::: mobile/android/modules/Home.jsm
@@ +380,5 @@
>          this.authConfig.imageUrl = options.auth.imageUrl;
>        }
>      }
>  
> +    if (typeof options.position !== "undefined" && typeof options.position === "number") {

Can the type ever be 'number' AND 'undefined'? My JavaScript is rusty and I wouldn't be surprised if JavaScript does weird things. :) Otherwise I'd just check for 'number' and do some validation like >= 0? But let's wait for Margaret's feedback.

Comment 5

[:Margaret Leibovic]

(In reply to Sebastian Kaspari (:sebastian) from comment #4)
> Comment on attachment 8648386 [details] [diff] [review]
> Possible fix
> 
> Review of attachment 8648386 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for the patch Heiko!
> 
> I tested it with the speed-dial sample add-on[1] (Set position to 0 in
> bootstrap.js) and it works.
> 
> [1] https://github.com/pocmo/speed-dial
> 
> ::: mobile/android/modules/Home.jsm
> @@ +380,5 @@
> >          this.authConfig.imageUrl = options.auth.imageUrl;
> >        }
> >      }
> >  
> > +    if (typeof options.position !== "undefined" && typeof options.position === "number") {
> 
> Can the type ever be 'number' AND 'undefined'? My JavaScript is rusty and I
> wouldn't be surprised if JavaScript does weird things. :) Otherwise I'd just
> check for 'number' and do some validation like >= 0? But let's wait for
> Margaret's feedback.

Good call. I don't think the type can ever be both 'number' and 'undefined', so a check for type 'number' should suffice, and I agree the check for >= 0 makes a lot of sense, since we don't support negative position values.

Comment 6

[Heiko Reese]

Attachment: “Minified” Patch

Looks like my first patch was a little halfhearted as I only concentrated on the first part of the test condition :-)

I attached a new version of the patch for completeness.

If I read the source correctly, options.position basically maps to the position member of Android's android.widget.GridView which should handle all remaining cases. I tested both negative numbers and large integers using the speed-dial sample add-on and it was added last in both cases.

Comment 7

[:Margaret Leibovic]

Comment on attachment 8650762 [details] [diff] [review]
“Minified” Patch

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

::: mobile/android/modules/Home.jsm
@@ +380,5 @@
>          this.authConfig.imageUrl = options.auth.imageUrl;
>        }
>      }
>  
> +    if (typeof options.position === "number") {

I think we should still follow Sebastian's suggestion to validate that options.position is >= 0, since a position < 0 won't do anything useful.

Also, please make sure to set the review? flag when you upload new patches. Otherwise patches tend to fall through the cracks! :)

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Heiko, do you have time to push this over the finish line or do you want someone else to pick this up? :)

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Maybe somebody else wants to pick it up from here. :)

Comment 10

[Tushar Saini (:shatur)]

Attachment: BUG-1192788.patch

Hi. 
   This is my first patch for js,  Please Review.

Comment 11

[:Margaret Leibovic]

Comment on attachment 8731652 [details] [diff] [review]
BUG-1192788.patch

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

::: mobile/android/modules/Home.jsm
@@ +380,5 @@
>          this.authConfig.imageUrl = options.auth.imageUrl;
>        }
>      }
>  
> +    if (typeof options.position >= 0) {

This statement evaluates the type of options.position, then compares that to 0. That's not what you want.

You should remove the `typeof` operator here.

Comment 12

[Tushar Saini (:shatur)]

Attachment: Bug-1192788-v2.patch

Hi,
    Sorry for that. Hope this will solved the issue.
Regards.

Comment 13

[:Margaret Leibovic]

Comment on attachment 8732005 [details] [diff] [review]
Bug-1192788-v2.patch

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

Great, thanks for the update. I'll mark this bug as ready to check in for you.

Let us know if you need help finding more bugs to work on! There is a long list of mentor bugs maintained here:
http://www.joshmatthews.net/bugsahoy/?mobileandroid=1

Comment 14

[Pulsebot]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2edb5ec1f8c2