Clicking on "Open Add-ons Manager" actually leads to the Plugins manager

RESOLVED FIXED in Firefox 40

Status

defect
RESOLVED FIXED
5 years ago
11 months ago

People

(Reporter: bbouvier, Assigned: maxgalmi, Mentored)

Tracking

unspecified
Firefox 40
x86_64
Linux

Firefox Tracking Flags

(firefox40 fixed)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
STR:
- Click on "select runtime"
- Click on "install simulator"
- Click on "open add-ons manager"

Expected:
We're lead to the add-ons tab of about:addons, where simulators are present.

Observed:
We actually get to the plugins tab of about:addons.

Actually, opening "about:addons" in a new tab also leads to the plugins tab, so there might not be so much to do here.
(In reply to Benjamin Bouvier [:bbouvier] from comment #0)
> STR:
> - Click on "select runtime"
> - Click on "install simulator"
> - Click on "open add-ons manager"
> 
> Expected:
> We're lead to the add-ons tab of about:addons, where simulators are present.
> 
> Observed:
> We actually get to the plugins tab of about:addons.
> 
> Actually, opening "about:addons" in a new tab also leads to the plugins tab,
> so there might not be so much to do here.

It's because you visited the plugin section the last time you used about:addons.

We should use BrowserOpenAddonsMgr("addons://list/extension").
Mentor: jryans
Whiteboard: [good first bug][lang=js]

Comment 2

4 years ago
Posted patch open-extensions.patch (obsolete) — Splinter Review
I attached a patch that opens the extensions page regardless of what the user had previously selected, using BrowserOpenAddonsMgr("addons://list/extension") as Paul Rouget suggested.

This is my first patch, so please tell me if I did something wrong or need different/more reviewers.
Attachment #8569367 - Flags: review?(jryans)
Comment on attachment 8569367 [details] [diff] [review]
open-extensions.patch

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

Great, this seems like the right idea overall!

I think your patch is not in quite the right format, as it's missing a commit message for example.  See the FAQ[1] for some help with this.

Also, I will be out next week, so it may take me a bit of time to check your next version.

[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

::: browser/devtools/webide/content/addons.js
@@ +10,5 @@
>  
>  window.addEventListener("load", function onLoad() {
>    window.removeEventListener("load", onLoad);
>    document.querySelector("#aboutaddons").onclick = function() {
> +    var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]

|wm| can be accessed from the |Services| object already imported above, so you can use |Services.wm| instead of this local variable.

@@ +13,5 @@
>    document.querySelector("#aboutaddons").onclick = function() {
> +    var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
> +                   .getService(Components.interfaces.nsIWindowMediator);
> +    var mainWindow = wm.getMostRecentWindow("navigator:browser");
> +    if(mainWindow.gBrowser.currentURI.spec != "about:addons"){

I don't think we need to this kind of specific check here in WebIDE code, so let's remove this if block.  If Firefox team wants to check this before opening a new add-ons tab, it could be added to |BrowserOpenAddonsMgr| that we are calling.

@@ +14,5 @@
> +    var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
> +                   .getService(Components.interfaces.nsIWindowMediator);
> +    var mainWindow = wm.getMostRecentWindow("navigator:browser");
> +    if(mainWindow.gBrowser.currentURI.spec != "about:addons"){
> +        let blanktab = mainWindow.gBrowser.addTab();

|BrowserOpenAddonsMgr| will open a tab as needed, so we don't need to explicitly call |addTab|.
Attachment #8569367 - Flags: review?(jryans) → feedback+
(Assignee)

Comment 4

4 years ago
Posted patch 1084911.patch (obsolete) — Splinter Review
Hi,

Robert doesn't seem to work on this bug anymore, so this is what I've done with your suggestions.
Attachment #8586441 - Flags: review?(jryans)
Attachment #8569367 - Attachment is obsolete: true
Comment on attachment 8586441 [details] [diff] [review]
1084911.patch

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

Looks good overall!

Correct the whitespace in an updated patch, and we should be all set!  Thanks for working on it!

::: browser/devtools/webide/content/addons.js
@@ +10,5 @@
>  
>  window.addEventListener("load", function onLoad() {
>    window.removeEventListener("load", onLoad);
>    document.querySelector("#aboutaddons").onclick = function() {
> +    Services.wm.getMostRecentWindow("navigator:browser").BrowserOpenAddonsMgr("addons://list/extension");  

Nit: Please remove the trailing whitespace at the end of the line.

Also, our typical code style is to stay below 80 characters... but this file already doesn't follow the rule, so I won't make you do it if you don't want to.
Attachment #8586441 - Flags: review?(jryans) → feedback+
Assignee: nobody → maxgalmi
Status: NEW → ASSIGNED
(Assignee)

Comment 6

4 years ago
Posted patch patch.diffSplinter Review
I removed the trailing space and split the line in 2.
Attachment #8587016 - Flags: review?(jryans)
Attachment #8586441 - Attachment is obsolete: true
Comment on attachment 8587016 [details] [diff] [review]
patch.diff

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

Great!

For future patches, mark the old attachments as "obsolete" when uploading the new one.

I've pushed this to our try environment to run tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0fd21ebaba2
Attachment #8587016 - Flags: review?(jryans) → review+
Okay, the try run looks good!  I've marked it to be checked in by a sheriff.  They'll resolve the bug once it lands in mozilla-central.

Thanks for looking at this!  You should check out other mentored[1] bugs for more ideas to work on.  For example, bug 1046049 might be good!

[1]: https://wiki.mozilla.org/DevTools/GetInvolved#Mentored_and_Good_First_Bugs
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/d6f83e2550b9
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d6f83e2550b9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 40

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.