601442 - Support the extensions.getAddons.showPane pref again in the Add-ons Manager UI

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[matiaswar]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101 Firefox/4.0b6
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101 Firefox/4.0b6

extensions.getAddons.showPane is obsolete for the new addon manager, setting this value in about:config to false will not make the Get Add-ons section dissapear from about:addons.

Reproducible: Always

Steps to Reproduce:
1.Open about:config
2.Set extensions.getAddons.showPane to false
3.Open about:addons and observe the Get Add-ons section is still there.
Actual Results:  
The Get Add-ons section stays.

Expected Results:  
The Get Add-ons section is hidden for a false value of extensions.getAddons.showPane

Comment 1

[skmail]

Confirmed for FF beta 10

Comment 2

[Richard van den Berg]

It is a shame that this useful preference no longer exists. I found a crude work around by adding this to the xul css:

#addons-page  #category-discover {display: none !important;}
#addons-page  #discover-browser {display: none !important;}
#addons-page  #discover-loading {display: none !important;}
#addons-page  #discover-error {display: none !important;}

Comment 3

[Mike Kaply [:mkaply]]

This started in Firefox 4. I wish I'd seen this bug.

Comment 4

[StevenKim]

I can reproduce this bug in Firefox 4.0.
How can I fix this bug? I mean with the option "extensions.getAddons.showPane to false, How can I remove "Get Add-ons" in the menu on Add-ons.

Comment 5

[StevenKim]

In order to solve this bug, you modified "toolkit/mozapps/extensions/content/extensions.css" file?

(In reply to Richard van den Berg from comment #2)
> It is a shame that this useful preference no longer exists. I found a crude
> work around by adding this to the xul css:
> 
> #addons-page  #category-discover {display: none !important;}
> #addons-page  #discover-browser {display: none !important;}
> #addons-page  #discover-loading {display: none !important;}
> #addons-page  #discover-error {display: none !important;}

Comment 6

[Richard van den Berg]

I did not solve this bug, but merely found a work around to hide the Get Addons tab. I added the above CSS statements to a XUL overlay in a custom extension.

Comment 7

[Martin Stránský [:stransky] (ni? me)]

I believe it should be fixed in extensions.xml (installRemote) to provide such functionality we already have in browser.js.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: WiP v1

(In reply to Martin Stránský from comment #7)
> I believe it should be fixed in extensions.xml (installRemote) to provide
> such functionality we already have in browser.js.

No, that would only make the install button for search results do nothing. It wouldn't affect the Get Add-ons pane at all.

gDiscoverView.initialize() in extensions.js needs to check the pref, set gDiscoverView.enabled, and hide the "Get Add-ons" category item via gCategoryManager. I'm attaching an old work-in-progres patch that does that (needs tests), that you're welcome to continue.

However, there are also a couple of places where the Add-ons Manager links to the Get Add-ons pane (via cmd_goToDiscoverPane) - when there are no search results available to install, and when there are no add-ons of a given type installed. On those cases, the button to go to the Get Add-ons pane needs to be hidden. Will need to check if there are any other cases where we either offer to navigate to that pane, or do so automatically.

Comment 9

[Martin Stránský [:stransky] (ni? me)]

(In reply to Blair McBride (:Unfocused) from comment #8)
> No, that would only make the install button for search results do nothing.
> It wouldn't affect the Get Add-ons pane at all.

Get Add-ons pane does not matter much, you can't install addons from the Get Add-ons pane when xpinstall is disabled. But you can search for addons (in the top of about:addons) and install them from the list.

> gDiscoverView.initialize() in extensions.js needs to check the pref, set
> gDiscoverView.enabled, and hide the "Get Add-ons" category item via
> gCategoryManager. I'm attaching an old work-in-progres patch that does that
> (needs tests), that you're welcome to continue.
> 
> However, there are also a couple of places where the Add-ons Manager links
> to the Get Add-ons pane (via cmd_goToDiscoverPane) - when there are no
> search results available to install, and when there are no add-ons of a
> given type installed. On those cases, the button to go to the Get Add-ons
> pane needs to be hidden. Will need to check if there are any other cases
> where we either offer to navigate to that pane, or do so automatically.

Okay, it makes sense to disable the "Get Add-ons pane" but we may hide or disable the addon search too.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Ahh, sorry, It's different bug. But anyway, is still related. I wonder if the Get Add-ons pane should be also disabled when xpinstall.enable is disabled?

Comment 11

[Mike Kaply [:mkaply]]

> I wonder if the Get Add-ons pane should be also disabled when xpinstall.enable is disabled?

Not disabled, locked.

xpinstall.enabled is a pretty useless pref, since when it is set, all that happens is the user gets a prompt saying "do you want to turn it on?"

See bug 668999

Anyone that wants to prevent installation of XPIs has to lock it.

Comment 12

[Martin Stránský [:stransky] (ni? me)]

Attachment: v1

Updated WiP for latest trunk. I don't insist on the xpinstall.enabled but IMHO makes sense. 

Tested on trunk, if the pane is disabled, the buttons which point to the "Get Add-ons pane" (from search and the Extensions pane) are disabled too so the patch seems to be working as expected.

Who would be the right reviewer for that?

Thanks!

Comment 13

[Martin Stránský [:stransky] (ni? me)]

(In reply to Michael Kaply (mkaply) from comment #11)
> Anyone that wants to prevent installation of XPIs has to lock it.

Which is the right way how to do it. The "do you want to turn it on?" dialog is just a shortcut for xpinstall.enable set in about:config. When the xpinstall.enable preference is disabled *and* locked, the dialog is not shown.

Comment 14

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 685128 [details] [diff] [review]
v1

Blair, I see you're a peer of toolkit so can you review it please?

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Martin Stránský from comment #13)
> (In reply to Michael Kaply (mkaply) from comment #11)
> > Anyone that wants to prevent installation of XPIs has to lock it.
> 
> Which is the right way how to do it.

At the moment it is, but I'd call the current behavior broken. Bug 668999 would fix that.

(In reply to Martin Stránský from comment #14)
> Blair, I see you're a peer of toolkit so can you review it please?

Also owner of the Add-ons Manager sub-module, so yes I can review this :)

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 685128 [details] [diff] [review]
v1

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

(In reply to Martin Stránský from comment #12)
> if the pane is disabled, the buttons which point to the
> "Get Add-ons pane" (from search and the Extensions pane) are disabled too

These buttons really need to be hidden, not just disabled (disabled but still visible adds unnecessary visual noise). Best way to do that is probably to add an ID to the buttons, and add a rule in content/extensions.css, hiding them when the disabled attribute is set to "true".

Also, could you write an automated test for this? Can just add to toolkit/mozapps/extensions/test/browser/browser_discover.js
Which is a browser-chrome test: https://developer.mozilla.org/en-US/docs/Browser_chrome_tests
Just need to test that flipping either preference:
* Hides the pane (use CategoryUtilities, defined in head.js)
* The button to navigate to the Get Add-ons pane is hidden no search results are found
* The button to navigate to the Get Add-ons pane is hidden when viewing a category that's empty. Can use the "dictionary" type for that: gViewController.loadView("addons://list/dictionary");

Comment 17

[Martin Stránský [:stransky] (ni? me)]

Attachment: wip

I think the simple way is this one but it may be difficult to write a test for it...so I'll try the css way.

Comment 18

[Martin Stránský [:stransky] (ni? me)]

(In reply to Blair McBride (:Unfocused) from comment #16)
> * Hides the pane (use CategoryUtilities, defined in head.js)
> * The button to navigate to the Get Add-ons pane is hidden no search results
> are found
> * The button to navigate to the Get Add-ons pane is hidden when viewing a
> category that's empty. Can use the "dictionary" type for that:
> gViewController.loadView("addons://list/dictionary");

Blair,

I have done tests for Get Addon pane but I have no luck with the button so far. Could you be more specific how to utilize the gViewController.loadView() function? I have this code:

  open_manager(null, function(aWindow) {
    gManagerWindow = aWindow;
    gManagerWindow.gViewController.loadView("addons://list/dictionary");
    var button = gManagerWindow.document.getElementById("discover-button");
    ok(!is_hidden(button), "It should not be hidden!");
    close_manager(gManagerWindow, end_test);
  });

It runs fine but the Dictionary pane is not shown. It looks like it needs some refresh after gViewController.loadView(), when I throw an exception after it the pane is shown:


  open_manager(null, function(aWindow) {
    gManagerWindow = aWindow;
    gManagerWindow.gViewController.loadView("addons://list/dictionary");

    throw "refresh!";

    var button = gManagerWindow.document.getElementById("discover-button");
    ok(!is_hidden(button), "It should not be hidden!");
    close_manager(gManagerWindow, end_test);
  });

I tried to call wait_for_manager_load()/wait_for_view_load() combo but without luck.

Thanks,
ma.

Comment 19

[Martin Stránský [:stransky] (ni? me)]

Attachment: v2

CSS patch + tests. 

The point it that is_hidden() does not work for the discover button. It calls recursively is_hidden() for upper (parents?) elements but those upper ones are not visible, although the buttons itself is visible.

Comment 20

[Martin Stránský [:stransky] (ni? me)]

Ping. Any update here?

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

My apologies for holding this up - I've had to take an extended break, due to a concussion. I'll try to get to this as soon as I'm able.

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 690825 [details] [diff] [review]
v2

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

My apologies for taking so long to get to this - health issues kept me from work (and catching up) for longer than expected.

(In reply to Martin Stránský from comment #19)
> The point it that is_hidden() does not work for the discover button. It
> calls recursively is_hidden() for upper (parents?) elements but those upper
> ones are not visible, although the buttons itself is visible.

I think this is because you've got two elements with the same ID (IDs need to be unique within a document). You're intending to test for the button in the list-view, but the button in the search-view is first in the DOM so that's what getElementById() will return. When you're testing for the button in list-view, the search-view will be hidden (ie, all the parents of the other button).

They need to be either different IDs, or use a class. Might be easiest to do both - use the IDs (discover-button-search, discover-button-installed) for testing, and the class (.discover-button) to hide it via CSS.

::: toolkit/mozapps/extensions/content/extensions.js
@@ +1758,4 @@
>    _loadListeners: [],
>  
>    initialize: function gDiscoverView_initialize() {
> +    if (Services.prefs.getPrefType(PREF_DISCOVERURL) == Services.prefs.PREF_INVALID)

This belongs with the other checks in isDiscoverEnabled (or move the contents of that function to be inline here - either way).

Comment 23

[Martin Stránský [:stransky] (ni? me)]

Attachment: v3

Thanks, this one address your comments, is_hidden() works as expected now.

Comment 24

[Mike Kaply [:mkaply]]

Is the search field disabled in this?

Even with xpinstall.enabled locked to false, Search allows you to search add-ons on AMO and install them.

Comment 25

[Martin Stránský [:stransky] (ni? me)]

xpinstall.enabled is covered by Bug 815120

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Michael Kaply (mkaply) from comment #24)
> Is the search field disabled in this?

Lets avoid scope creep here.

Comment 27

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 733801 [details] [diff] [review]
v3

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

Good to go :)

Comment 28

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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