1346722 - Display permissions in alphabetical order

Status: VERIFIED FIXED

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

Description

[Vasilica Tamas, QA [:vtamas]]

Attachment: 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.

Comment 1

[Andy McKay]

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.

Comment 2

[Vasilica Tamas, QA [:vtamas]]

(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.

Comment 3

[Vasilica Tamas, QA [:vtamas]]

Attachment: 2017-03-14_1130.png

I’m attaching a new colored screenshot in order to evidence better the differences.

Comment 4

[Andy McKay]

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.

Comment 5

[Andrew Williamson [:eviljeff]]

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.

Comment 6

[Andy McKay]

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

Comment 7

[Swapnil Agarwal]

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.

Comment 8

[Andrew Swan [:aswan]]

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.

Comment 9

[Swapnil Agarwal]

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?

Comment 10

[Andrew Swan [:aswan]]

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.

Comment 11

[Caitlin Neiman [:caitmuenster]]

Hi Swapnil! I just wanted to check in and see how you are doing with this bug.

Comment 12

[Swapnil Agarwal]

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.

Comment 13

[Caitlin Neiman [:caitmuenster]]

Hi Swapnil! I'm needinfo'ing Andrew Swan to see if he might be able to help. :)

Comment 14

[Andrew Swan [: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.

Comment 15

[Caitlin Neiman [:caitmuenster]]

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.

Comment 16

[Swapnil Agarwal]

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

Comment 17

[Caitlin Neiman [:caitmuenster]]

No problem, Swapnil! Hope to see you around sometime soon.

Comment 18

[Aastha]

Attachment: permissions now in same order in AMO and desktop pop-up

Hi!
Kindly review it and let me if know if any changes are required.
Thanks

Comment 19

[Caitlin Neiman [:caitmuenster]]

Hey Aastha! I'm needinfo'ing Andrew Swan, the bug's mentor, for review. :)

Comment 20

[Andrew Swan [:aswan]]

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...

Comment 21

[Aastha]

Attachment: permissions now in same order in AMO and desktop pop-up

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?

Comment 22

[Aastha]

(In reply to Aastha from comment #21)
> Created attachment 8913259 [details] [diff] [review]
Flags: review?([e-mail])

Comment 23

[Andrew Swan [:aswan]]

(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!

Comment 24

[Aastha]

Attachment: changes in v3 of the patch

Comment 25

[Aastha]

Attachment: v3 of the patch

Changes in v3:
- shallow copy of the permissions using array.slice(0)
- fixed the existing failures in mach mochitest browser/base/content/test/webextensions

Comment 26

[Andrew Swan [: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.

Comment 27

[Aastha]

Attachment: v4 of the patch

changes in v4:
- more descriptive comments as suggested

Comment 28

[Andrew Swan [:aswan]]

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?

Comment 29

[Aastha]

Yes I am using hg.

Comment 30

[Andrew Swan [:aswan]]

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.

Comment 31

[Aastha]

Attachment: hg export patch

Comment 32

[Andrew Swan [:aswan]]

Great, please add the string "r=aswan" to the commit message subject and we'll be good to go.

Comment 33

[Aastha]

Attachment: added r=aswan to commit message subject

Comment 34

[Andrew Swan [:aswan]]

Your commit message looks good but now you added a bunch of other unrelated stuff to the patch...

Comment 35

[Aastha]

Attachment: removed unnecessary files from the patch

Sorry about that. I have corrected the patch now.

Comment 36

[Andrew Swan [: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!

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/784028f4bb88
Display permissions in alphabetical order. r=aswan

Comment 38

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/784028f4bb88

Comment 39

[CosminB]

Attachment: 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.

Comment 40

[Caitlin Neiman [:caitmuenster]]

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.

Comment 41

[Aastha]

This is the link to my profile: https://mozillians.org/en-GB/u/AasthaGupta/

Thank you for vouching for me. :)

Comment 42

[Caitlin Neiman [:caitmuenster]]

Vouched! \o/