Closed Bug 893774 Opened 8 years ago Closed 8 years ago

doorhanger API should allow null for buttons

Categories

(Firefox for Android Graveyard :: General, defect)

22 Branch
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: mkaply, Assigned: Sobering)

Details

(Whiteboard: [mentor=margaret][lang=js][lang=java])

Attachments

(1 file)

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.
Hi Margaret!

I would like to fix this one. Please tell me about the changes we need to make here and how.
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
Assignee: nobody → anand.92.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!
(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.
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 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 :)
Attachment #827858 - Flags: review+
I'm setting the checkin-needed keyword, which means someone with commit access will come along and land this for you.
Assignee: anand.92.soni → ss
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/a64353f7f97c
Keywords: checkin-needed
Whiteboard: [mentor=margaret][lang=js][lang=java] → [mentor=margaret][lang=js][lang=java][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a64353f7f97c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=margaret][lang=js][lang=java][fixed-in-fx-team] → [mentor=margaret][lang=js][lang=java]
Target Milestone: --- → Firefox 28
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.