Closed Bug 1346722 Opened 3 years ago Closed 2 years ago
Display permissions in alphabetical order
109.24 KB, image/png
123.86 KB, image/png
2.05 KB, patch
|Details | Diff | Splinter Review|
159.10 KB, image/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 → ---
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
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.
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
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.
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.
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.
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
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
Hey Aastha! I'm needinfo'ing Andrew Swan, the bug's mentor, for review. :)
Flags: needinfo?(cneiman) → needinfo?(aswan)
Attachment #8906350 - Flags: review-
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
(In reply to Aastha from comment #21) > Created attachment 8913259 [details] [diff] [review] Flags: review?(firstname.lastname@example.org)
(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!
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 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.
changes in v4: - more descriptive comments as suggested
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?
Yes I am using hg.
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.
Great, please add the string "r=aswan" to the commit message subject and we'll be good to go.
Attachment #8916047 - Attachment is obsolete: true
Your commit message looks good but now you added a bunch of other unrelated stuff to the patch...
Sorry about that. I have corrected the patch now.
Attachment #8916200 - Attachment is obsolete: true
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+
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/784028f4bb88 Display permissions in alphabetical order. r=aswan
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.
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. :)
You need to log in before you can comment on or make changes to this bug.