893774 - doorhanger API should allow null for buttons

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mike Kaply [:mkaply]]

Doorhangers work great without buttons for showing messages that you want to persist.

In order to do no buttons, though, you have to pass an empty array ([]).

Just passing null for the buttons should work as well.

Comment 1

[:Margaret Leibovic]

This will involve modifying both the JS and Java side of the doorhanger code, but it should be pretty straightforward. See:
http://hg.mozilla.org/mozilla-central/file/60e5fd5a379c/mobile/android/chrome/content/browser.js#l1642
http://hg.mozilla.org/mozilla-central/file/60e5fd5a379c/mobile/android/base/DoorHangerPopup.java#l52
http://hg.mozilla.org/mozilla-central/file/60e5fd5a379c/mobile/android/base/DoorHangerPopup.java#l147

Comment 2

[Anand Soni]

Hi Margaret!

I would like to fix this one. Please tell me about the changes we need to make here and how.

Comment 3

[:Margaret Leibovic]

Hi Anand!

First of all, do you have a build environment set up? If not, you should follow the directions here:
https://wiki.mozilla.org/Mobile/Fennec/Android

After that, you should look at the code I linked to in my last comment. Fixing this bug involves changing the NativeWinodw.doorhanger.show implementation to accept null as an aButtons parameter.

Thinking about this a bit more, you could probably just create an empty array if aButtons is null, and pass that to Java, so you probably don't need to mess with the Java files like I mentioned in that comment.

To test this, you should write a little test add-on that exercises this API. We have a skeleton add-on on github that you could fork to do this really easily:
https://github.com/mfinkle/skeleton-addon-fxandroid

You would just need to modify this line:
https://github.com/mfinkle/skeleton-addon-fxandroid/blob/master/bootstrap.js#L29

Feel free to join #mobile on irc.mozilla.org to ask more questions. We also have more information on our Get Involved page:
https://wiki.mozilla.org/Mobile/Get_Involved

Comment 4

[Anand Soni]

Hi Margaret!
Sorry for the delay, I was a bit busy and couldn't do this. I was going through the code you pointed to, above. You say that I will just need to create an empty array if aButtons is null and pass it to Java. I didn't understand from the code where should I check this (whether aButtons is null or not). How is the array to be passed to java? I understand that there is nothing to be done with the Java files, though.

And for the test-addon, you said that I just need to change a particular line. This lines just displays the doorhanger attached to the tab. What do we do with this? Since the button is null, what has to be displayed. Is it not an error in that case? 

I need a bit more clarification on the above points before I write this down. Thanks a lot!

Comment 5

[:Margaret Leibovic]

(In reply to Anand Soni from comment #4)

> Sorry for the delay, I was a bit busy and couldn't do this. I was going
> through the code you pointed to, above. You say that I will just need to
> create an empty array if aButtons is null and pass it to Java. I didn't
> understand from the code where should I check this (whether aButtons is null
> or not). How is the array to be passed to java? I understand that there is
> nothing to be done with the Java files, though.

You should check to see if aButtons is null right at the top of the doorhanger.show function:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1707

You can see in that function that we're already creating a "Doorhanger:Add" message that we pass to Java, which already includes a buttons property that's just set to aButtons. So you don't need to change anything here.  

> And for the test-addon, you said that I just need to change a particular
> line. This lines just displays the doorhanger attached to the tab. What do
> we do with this? Since the button is null, what has to be displayed. Is it
> not an error in that case? 

That's what this bug is about :) You should pass null as the buttons parameter in the test add-on and make sure that no error occurs. Passing null should mean that no buttons appear.

Comment 6

[Stefan :Sobering]

Attachment: bug-893774-fix.patch

Hey Margaret,

I very briefly spoke to you in #mobile last week in regards to taking over this bug.

I've submitted a patch as well as a link to my fork of the skeleton Firefox addon containing a test for this fix: https://github.com/sobering/skeleton-addon-fxandroid/tree/893774_test_null_aButtons_param

I've tried to follow all the instructions for building, hacking, and patch submission, but this is my first bug. If I did anything wrong please let me know!

--ss

Comment 7

[:Margaret Leibovic]

Comment on attachment 827858 [details] [diff] [review]
bug-893774-fix.patch

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

Great work! Thanks for the detailed comment and the link to the skeleton add-on you used to test. This looks good.

In the future, when you upload a patch, you should set the review? flag to an appropriate reviewer (in this case it would be me :). Since I'm already looking at this patch and think it looks good, we can just skip that step and I'll set review+ to express my approval :)

Comment 8

[:Margaret Leibovic]

I'm setting the checkin-needed keyword, which means someone with commit access will come along and land this for you.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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