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

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

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.

Comment 1

[Paul Rouget [:paul]]

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

Comment 2

[Robert Holt]

Attachment: open-extensions.patch

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.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 4

[Aime Galmi]

Attachment: 1084911.patch

Hi,

Robert doesn't seem to work on this bug anymore, so this is what I've done with your suggestions.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 6

[Aime Galmi]

Attachment: patch.diff

I removed the trailing space and split the line in 2.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/d6f83e2550b9

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d6f83e2550b9