Closed Bug 759824 Opened 8 years ago Closed 8 years ago

Doorhanger button callback functions in LoginManagerPrompter.js contain incorrect arguments

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 17

People

(Reporter: Margaret, Assigned: capella)

Details

(Whiteboard: [good first bug][mentor=margaret][lang=js])

Attachments

(1 file)

This cruft is leftover from porting LoginManagerPrompter.js from XUL fennec. We should get rid of the aNotificationBar and aButton arguments in the doorhanger callbacks, like the one here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/LoginManagerPrompter.js#224

(Note: Doorhanger callback functions may expect to be called with a "checked" argument, but that's only if they are created with a checkbox, which is not the case here.)
Attached patch Patch (v1)Splinter Review
Ok, so this patch removes the two parms from four callbacks in LoginManagerPrompter.js in the mobile/android folder, and leaves the mobile/xul version as-is.

Is this all you're looking for? How would I go about testing this? I wouldn't mind getting involved in more Android patches. I've done a lot with a11y, and also some small patches in core, widgets, scratchpad, extensions etc. (mostly C++ but some JS).

Let me know? -- mark
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #649116 - Flags: review?(margaret.leibovic)
Comment on attachment 649116 [details] [diff] [review]
Patch (v1)

Thanks for the patch! This looks good to me.

Unfortunately, our automated testing story for fennec native is pretty sad (that would be a great place to help out!), so you'll just have to manually test this. To do that, you could just go to a site to trigger the "remember password?" prompt (like gmail.com), then make sure the buttons do what you expect. After you choose remember, submitting a password form with a different password should trigger the "change password?" prompt. And you can just test these with random username/password values, since we prompt to remember even if login isn't successful :)
Attachment #649116 - Flags: review?(margaret.leibovic) → review+
Nice. fyi, I've seen that different groups have different ways of working. I mentioned that I'm new to this area. I just got a Galaxy SIII, loaded VMWare on my WIN machine, learning linux, and Android build / test procedures. 

Does your group usually take over approved patches from contributors and land them? I can push to TRY and land my own stuff if its expected.

Understand, I've not gotten as far as loading / testing my own patches on my device yet, and don't know how much a TRY push will catch.
(In reply to Mark Capella [:capella] from comment #3)
> Nice. fyi, I've seen that different groups have different ways of working. I
> mentioned that I'm new to this area. I just got a Galaxy SIII, loaded VMWare
> on my WIN machine, learning linux, and Android build / test procedures. 

Awesome :)

> Does your group usually take over approved patches from contributors and
> land them? I can push to TRY and land my own stuff if its expected.

We usually land patches for contributors if they don't have commit access, or take advantage of the "checkin-needed" keyword.

> Understand, I've not gotten as far as loading / testing my own patches on my
> device yet, and don't know how much a TRY push will catch.

I don't think a try push will catch much, since we don't have test coverage for these functions. If you let me know when you get a build set up on your phone and verify that things work, I can land this for you. Feel free to reach out in #mobile if you have any issues getting a build set up! That's where we all hang out :)
Ok, fyi ...

   I built this and tested it on my device ... I added a few test literal / button changes to ensure I was getting a true end-to-end test of my modified code ... so cool when it worked!

   Re: testing the change itself ... it's a little hard to show that the change "worked", as we're basically removing references to unused vars, but I did verify that no regressions occurred / all that worked before the patch still works after the patch.

   I have commit access and have landed many other patches in other areas. If I don't hear otherwise, I'll push this to TRY (as I always do), post results here, and push to inbound later tomorrow or the next morning.

  -- mark
Sounds great! Thanks, Mark!
Push to TRY (taking its time...)
https://tbpl.mozilla.org/?tree=Try&rev=a6bebe0ad206
https://hg.mozilla.org/mozilla-central/rev/9f185f2ebded
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
You need to log in before you can comment on or make changes to this bug.