1132971 - Setting the preference extensions.getAddons.showPane to false causes the Add-ons Manager not to work correctly

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Mike Kaply [:mkaply]]

The preference extensions.getAddons.showPane can be set to false to prevent the "discovering addons" pane from displaying.

If you create a new profile, set it and then open the add-ons manager, the extensions pane doesn't display properly and you get this error on the console:

NS_ERROR_FAILURE: Root node doesn't exist for 'discover' view extensions.js:717

This is a regression since this feature was fixed in bug #601442

Comment 1

[Mike Kaply [:mkaply]]

Attachment: MozReview Request: bz://1132971/mkaply

/r/6787 - Bug 1132971 - Don't assume that the add-ons discovery pane will be available

Pull down this commit:

hg pull -r 0aa57515f92bf90991ba4be1107407bbe6ab6754 https://reviewboard-hg.mozilla.org/gecko/

Comment 2

[Dave Townsend [:mossop]]

(In reply to Mike Kaply [:mkaply] from comment #1)
> Created attachment 8590326 [details]
> MozReview Request: bz://1132971/mkaply
> 
> /r/6787 - Bug 1132971 - Don't assume that the add-ons discovery pane will be
> available
> 
> Pull down this commit:
> 
> hg pull -r 0aa57515f92bf90991ba4be1107407bbe6ab6754
> https://reviewboard-hg.mozilla.org/gecko/

This seems like something that is extremely regression prone without us noticing, is it possible to get a test?

Comment 3

[Mike Kaply [:mkaply]]

> This seems like something that is extremely regression prone without us noticing

Well the code wouldn't be any different unless isDiscoverEnabled is false (which isn't the default).

I thought about making the change much smaller by keeping VIEW_DEFAULT and just changing it to a var, but that seemed wrong because in Mozilla, consts are upper case.

> is it possible to get a test?

That's a good question. I honestly don't know. In the past, I've had trouble figuring out how to write tests for Firefox in general. What would I use to write a test for something like this?

Comment 4

[Dave Townsend [:mossop]]

(In reply to Mike Kaply [:mkaply] from comment #3)
> > This seems like something that is extremely regression prone without us noticing
> 
> Well the code wouldn't be any different unless isDiscoverEnabled is false
> (which isn't the default).
> 
> I thought about making the change much smaller by keeping VIEW_DEFAULT and
> just changing it to a var, but that seemed wrong because in Mozilla, consts
> are upper case.
> 
> > is it possible to get a test?
> 
> That's a good question. I honestly don't know. In the past, I've had trouble
> figuring out how to write tests for Firefox in general. What would I use to
> write a test for something like this?

A browser-chrome test would be the best. We have a bunch that open the add-ons manager. You could write one that sets the pref and opens the manager and checks it displays properly. This one already claims to test the discovery pane, maybe add an extra bit to that: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_discovery.js

Comment 5

[Mike Kaply [:mkaply]]

Ironically there is a test for this.

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_discovery.js#581

It clearly doesn't work :)

Comment 6

[Dave Townsend [:mossop]]

(In reply to Mike Kaply [:mkaply] from comment #5)
> Ironically there is a test for this.
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> test/browser/browser_discovery.js#581
> 
> It clearly doesn't work :)

Looks like that forcibly opens the extensions list when opening the manager. You'll want to not open any specific pane (let it open the default) and also clear the last selected pane before opening, I don't recall if that is in localstore though which might be annoying to do.

Comment 7

[Mike Kaply [:mkaply]]

Comment on attachment 8590326 [details]
MozReview Request: bz://1132971/mkaply

/r/6787 - Bug 1132971 - Don't assume that the add-ons discovery pane will be available

Pull down this commit:

hg pull -r da07f23561a53bcad6e1726e75317dfb4542b049 https://reviewboard-hg.mozilla.org/gecko/

Comment 8

[Mike Kaply [:mkaply]]

Now with test!

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 8590326 [details]
MozReview Request: bz://1132971/mkaply

https://reviewboard.mozilla.org/r/6785/#review5695

Awesome, thanks for adding the test.

::: toolkit/mozapps/extensions/test/browser/browser_discovery.js
(Diff revision 2)
> +  Services.prefs.setBoolPref(PREF_DISCOVER_ENABLED, false);

Please reset the pref after the test is done, to make sure if we add later tests we don't confuse later tests.

Comment 10

[Mike Kaply [:mkaply]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b901409f138

With the additional change to reset the pref.

Comment 11

[Mike Kaply [:mkaply]]

FYI, I had to do a second checkin for this. I didn't realize that another test:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_bug679604.js

Had defined PREF_UI_LASTCATEGORY

I didn't find this because I didn't run the entire test suite. Lesson learned.

I removed the definition from that file.

https://hg.mozilla.org/integration/mozilla-inbound/rev/49b6aed6b2e2

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0b901409f138
https://hg.mozilla.org/mozilla-central/rev/49b6aed6b2e2

Comment 14

[Mike Kaply [:mkaply]]

Attachment: MozReview Request: Bug 1132971 - Don't assume that the add-ons discovery pane will be available