Closed Bug 1346722 Opened 3 years ago Closed 2 years ago

Display permissions in alphabetical order

Categories

(Toolkit :: Add-ons Manager, defect, P5)

defect

Tracking

()

VERIFIED FIXED
mozilla58
Tracking Status
firefox52 --- unaffected
firefox53 --- affected
firefox54 --- affected
firefox55 --- affected
firefox58 --- verified

People

(Reporter: vasilica.mihasca, Assigned: aastha.gupta4104, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [triaged])

Attachments

(4 files, 7 obsolete files)

Attached image 2017-03-13_1205.png
[Note]
This is a follow-up bug for GitHub issue: https://github.com/mozilla/addons-server/issues/4822

[Affected versions]:
Firefox 55.0a1 (2017-03-12)
Firefox 54.0a2 (2017-03-12)
Firefox 53.0b1 (20170307064827)


[Affected platforms]:
Windows 10 64-bit
Ubuntu 16.04 32-bit


[Steps to reproduce]:
1.Launch Firefox with clean profile.
2.Create extensions.webextPermissionPrompts and set it to true.
3.Create xpinstall.signatures.dev-root and set it to true.
4.Restart the browser.
5.Click on “+ Add to Firefox” button to install the following webextension: https://addons-dev.allizom.org/en-US/firefox/addon/promptable6/. 


[Expected Results]:
The permissions are listed in alphabetical order of permission name also taking into account the priority rule.  

[Actual Results]:
 - The permissions, except host and nativeMessaging, are listed in the order they are declared in the manifest.json.
 - AMO respects the alphabetical order and this is the reason why there is no consistency between AMO and Firefox permissions pop-ups.
Sorry, we made a change in the install in Firefox, but didn't make a change in AMO. Vasilica could you please file a bug on AMO in github to bump hosts and nativeMessaging to the top please.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INVALID
(In reply to Andy McKay [:andym] from comment #1)
> Sorry, we made a change in the install in Firefox, but didn't make a change
> in AMO. Vasilica could you please file a bug on AMO in github to bump hosts
> and nativeMessaging to the top please.

I think you misunderstood the bug’s purpose.
The issues you suggested is already logged and fixed in AMO: https://github.com/mozilla/addons-server/issues/4822 - this can be confirmed by the attached screenshot.

But, in this bug I’m referring to “the rest” category of permissions (which you classified in https://bugzilla.mozilla.org/show_bug.cgi?id=1339952#c1). In AMO, these permissions, except host and nativeMessaging, are listed in alphabetical order and not in the order they are declared in manifest.json as Firefox does.  

Please see the order discrepancies of descriptions between desktop and AMO pop-up for the same permissions in the attached screenshot.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Attached image 2017-03-14_1130.png
I’m attaching a new colored screenshot in order to evidence better the differences.
Thinking about it, I don't know why I said alphabetical order. Probably that's the way I like them, but I realise that it doesn't really matter that much. Also alphabetical order is really hard to maintain between languages.

What's important is that: host and nativeMessaging at the top. 

Having the rest is a consistent order between AMO and Add-ons manager would be nice to have. Strict alphabetical order isn't going to matter.  Whomever wants to change AMO or Add-ons Manager can go for it.
Priority: -- → P5
Whiteboard: [triaged]
AMO (currently) does alphabetical order* based on the permission name (rather than the description) so each add-on has the permissions in the same order, regardless of which order of permissions the developer choose in the manifest.

* Apart from match patterns and nativeMessaging at the top.
Flags: needinfo?(amckay)
Nominating for a good first bug, it seems like its a matter of ordering the permissions here:

https://dxr.mozilla.org/mozilla-central/source/browser/modules/ExtensionsUI.jsm#312
Flags: needinfo?(amckay)
Keywords: good-first-bug
Mentor: aswan
Hello Vasilica,
 In case if the bug is still open, can I work on this as my first bug. Please let me know how to get started.
Hi Swapnil,
Thanks for your interest!  I've assigned the bug to you.

Have you build Firefox before?  If not, follow the instructions here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
note that for this bug you should be able to use artifact builds:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds

As for the actual bug, if you're not already familiar with the extension installation process and permissions prompts, I would create a few test extensions and get acquanited with how those work.

When you're ready to make changes, it should be as straightforward as sorting the list of permissions here:
https://dxr.mozilla.org/mozilla-central/rev/da66c4a05fda49d457d9411a7092fed87cf9e53a/browser/modules/ExtensionsUI.jsm#324
You can begin by testing manually.  Once you've got that working, we can talk about writing an automated test.

Please feel free to ask any questions that come up along the way, if you click the "Need more information" box in bugzilla and enter my address, it will ensure I notice your question promptly.
Assignee: nobody → swapnilaga3
Hello Vasilica,

Thanks for giving me an opportunity to work on the bug but as per the hardware specifications are concerned I am not having those type of specifications on my system. Is there any other way to move ahead?
Hi, first of all, if you're asking a question and need an answer from somebody to make progress, please use the "Need more information from ..." button at the bottom of the page, that will help ensure the person you're asking knows that you're waiting for them.

As for your specific question, I'm not sure which hardware specifications you're referring to?  As long as you can build and run Firefox, you should be able to handle this bug.
Hi Swapnil! I just wanted to check in and see how you are doing with this bug.
Flags: needinfo?(swapnilaga3)
Hello Caitin,

Actually I am struggling with Visual Studio installation due to memory issues. Can you please regarding this? Is there any alternative for it.
Flags: needinfo?(cneiman)
Hi Swapnil! I'm needinfo'ing Andrew Swan to see if he might be able to help. :)
Flags: needinfo?(cneiman) → needinfo?(aswan)
I don't know much about building on Windows.  I would suggest joining the #introduction channel on IRC where there are people who can help with this sort of question.
Flags: needinfo?(aswan)
Hey Swapnil -- just wanted to check in and see if you were able to install Video Studio and if you are still interested in working on this bug.
Hello Caitlin, I am sorry I am not able to work on the issue due to other priorities. Hence please assign this bug to someone else.

Thanks
Flags: needinfo?(swapnilaga3)
Assignee: swapnilaga3 → nobody
No problem, Swapnil! Hope to see you around sometime soon.
Hi!
Kindly review it and let me if know if any changes are required.
Thanks
Flags: needinfo?(cneiman)
Hey Aastha! I'm needinfo'ing Andrew Swan, the bug's mentor, for review. :)
Flags: needinfo?(cneiman) → needinfo?(aswan)
Blocks: 1401643
No longer blocks: webext-permissions
Comment on attachment 8906350 [details] [diff] [review]
permissions now in same order in AMO and desktop pop-up

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

Sorry for the slow reply, please use the r? flag for future review requests.
I think this needs two additional things to be landable:
1. It needs a test
2. The javascript array sort() method modified the array it is called on, but formatPermissionStrings() should not modify its arguments.  I guess that leaves us with making a copy of the array and then sorting the copy...
Attachment #8906350 - Flags: review-
Flags: needinfo?(aswan)
I have made another copy of the permissions and then sorted it.
Can you explain me a bit more about what sort of tests to write and where?
Attachment #8906350 - Attachment is obsolete: true
Flags: needinfo?(aswan)
(In reply to Aastha from comment #21)
> Created attachment 8913259 [details] [diff] [review]
Flags: review?(aswan@mozilla.com)
(In reply to Aastha from comment #21)
> Created attachment 8913259 [details] [diff] [review]
> permissions now in same order in AMO and desktop pop-up
> 
> I have made another copy of the permissions and then sorted it.

Your new patch just copies a reference to the array, to actually copy the array you'll need to use something like Array.slice.

> Can you explain me a bit more about what sort of tests to write and where?

The current permissions tests are a bit of a mess but in fact they already cover this case and they now fail with this patch.  I guess for now just fixing the existing tests should be sufficient.  Try running something like the following:
mach mochitest browser/base/content/test/webextensions

Just fixing the existing failures there with this patch should be sufficient.
Thanks!
Flags: needinfo?(aswan)
Attached patch changes in v3 of the patch (obsolete) — Splinter Review
Attached patch v3 of the patch (obsolete) — Splinter Review
Changes in v3:
- shallow copy of the permissions using array.slice(0)
- fixed the existing failures in mach mochitest browser/base/content/test/webextensions
Attachment #8913259 - Attachment is obsolete: true
Attachment #8914599 - Attachment is obsolete: true
Attachment #8914603 - Flags: review?(aswan)
Comment on attachment 8914603 [details] [diff] [review]
v3 of the patch

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

This is looking good, just some nits about comments and then we'll be good to go.
Please add a comment in head.js showing the order of the permissions in the manifest and explaining that the order in the test is deliberately different.
Also lets update the comment in Extension.jsm to explicitly say that permissions are sorted alphabetically by the permission string to match AMO.  The comment about making a copy of the array is unnecessary.
Attachment #8914603 - Flags: review?(aswan)
Attached patch v4 of the patch (obsolete) — Splinter Review
changes in v4:
- more descriptive comments as suggested
Attachment #8914603 - Attachment is obsolete: true
Attachment #8915016 - Flags: review?(aswan)
Assignee: nobody → aastha.gupta4104
This looks good, thanks!  I've started a try run for you to verify that tests still work in automation:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6edbe79f06a677a683729cced0fafbaa661e49e

Sorry for not noticing this in an earlier revision but to land this the patch needs to be formatted in a way that it can be imported by hg.  It should also have the bug number and a short summary of the change in the subject.  There are some more details here:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

I can help you with that if needed, are you using hg for your local source tree or something else?
Flags: needinfo?(aastha.gupta4104)
Yes I am using hg.
Flags: needinfo?(aastha.gupta4104)
Great, so referring to the MDN document linked in comment 28, if you make sure your commit message is formatted as described in the section "Commit Message Conventions", either of the methods in "How can I generate a patch for somebody else to check-in for me?" should work.
Attached patch hg export patch (obsolete) — Splinter Review
Attachment #8915016 - Attachment is obsolete: true
Attachment #8915016 - Flags: review?(aswan)
Flags: needinfo?(aswan)
Great, please add the string "r=aswan" to the commit message subject and we'll be good to go.
Flags: needinfo?(aswan)
Attachment #8916047 - Attachment is obsolete: true
Flags: needinfo?(aswan)
Your commit message looks good but now you added a bunch of other unrelated stuff to the patch...
Flags: needinfo?(aswan)
Sorry about that. I have corrected the patch now.
Attachment #8916200 - Attachment is obsolete: true
Flags: needinfo?(aswan)
Comment on attachment 8916609 [details] [diff] [review]
removed unnecessary files from the patch

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

Thanks for the contribution and for your perseverance.  You can add the checkin-needed keyword and this should land in the next day or so!
Attachment #8916609 - Flags: review+
Flags: needinfo?(aswan)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/784028f4bb88
Display permissions in alphabetical order. r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/784028f4bb88
Status: REOPENED → RESOLVED
Closed: 3 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Attached image PermissionsOrder.png
This issue is verified as fixed on Firefox 58.0a1 (20171011220113) under Wind 7 64-bit and Ubuntu 16.04 32-bit.

The permissions are displayed in the pop-up in alphabetical order based on the permission name and not by description name.

Please see the attached screenshot.
Status: RESOLVED → VERIFIED
Thanks so much for the patch, Aastha! Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition#October_2017

Also, if you'd like to set up a profile on mozillians.org, I'd be happy to vouch for you. :) 

Welcome onboard! I look forward to seeing you around.
This is the link to my profile: https://mozillians.org/en-GB/u/AasthaGupta/

Thank you for vouching for me. :)
Flags: needinfo?(cneiman)
Vouched! \o/
Flags: needinfo?(cneiman)
You need to log in before you can comment on or make changes to this bug.