571759 - Move Add-on Manager to be in a content tab

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[Mark Banner (:standard8)]

Now the new add-on manager has landed and is working in Thunderbird, we want to use it in a content tab so that we can easily get to the extra functionality obtained by having it in one of those tabs - e.g. add-on installation, drag n drop etc.

This bug should do that, removing the old window version and swapping to using content tabs.

Comment 1

[u382332]

How is this going to affect users who don't use tabs and hide the tab bar?

Comment 2

[Mark Banner (:standard8)]

(In reply to comment #1)
> How is this going to affect users who don't use tabs and hide the tab bar?

The tab bar will be displayed and the you'll temporarily have a tab whilst the manager is open.

Comment 3

[u382332]

Excellent. That will work just fine.

Comment 4

[Mark Banner (:standard8)]

Attachment: Partial fix

This is a partial fix so far. What needs doing:

- Re-enable the functionality to select a pane when opening the add-ons manager, i.e. handle the aPane parameter to openAddonsMgr. Look at BrowserOpenAddonsMgr in Firefox for some hints on how to do this.
- Hook up the "Manage Add-ons" button in preferences to open in a new tab, possibly via openAddonsMgr.
- Mozmill tests

I'm not sure when the next time I'll be able to do bits of work on this will be. So if anyone wants to pick it up in the meantime, please do.

Comment 6

[Mark Banner (:standard8)]

This needs to fix the notifications as well (xref bug 567127) so that installation works as well.

Comment 8

[Joe Sabash [:JoeS1]]

The inability to selectively disable extensions is becoming extremely painful in determining extension conflicts on trunk builds.
I'm quite sure this will lead to meaningless bug reports soon.
(I nearly reported one such case just today)

Comment 9

[Mark Banner (:standard8)]

Attachment: Part 1 - the basics

(moved to correct bug location from bug 517498)

This gets the basics working. It probably needs a few tweaks to the site regexps and a few notification handling updates, but at least it'll bring trunk's add-on manager into the partially usable range.

Main things in this patch are to: swap to opening add-on manager in a tab, enable history on the content tabs - required for add-ons manager (I've done this in the deferred way like Firefox for start-up times) and to update the prefs to match the new ones required.

Comment 10

[Mark Banner (:standard8)]

Attachment: [checked in] Part 1 - the basics v2

I just found one addition and one correction in the all-thunderbird.js prefs file that we may as well take in this part of the bug/patch.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 476767 [details] [diff] [review]
[checked in] Part 1 - the basics v2

I've run with it for a while, and it looks pretty nice.  :)

>+++ b/mail/app/profile/all-thunderbird.js
>@@ -125,16 +125,29 @@ pref("app.update.idletime", 60);
>+// Enables some extra Extension System Logging (can reduce performance)
>+pref("extensions.logging.enabled", false);

So we set enabled to false to enable the logging?

I think it might be nice if we enabled the "Go » Forward" and "Go » Back" buttons since we're enabling history, but that could be a followup bug.

I also wonder what sort of testing we can do here, but that, too, can be a followup patch.

So, aside from those small questions/suggestions, r=me.

Later,
Blake.

Comment 12

[Mark Banner (:standard8)]

(In reply to comment #11)
> >+++ b/mail/app/profile/all-thunderbird.js
> >@@ -125,16 +125,29 @@ pref("app.update.idletime", 60);
> >+// Enables some extra Extension System Logging (can reduce performance)
> >+pref("extensions.logging.enabled", false);
> 
> So we set enabled to false to enable the logging?

Well, that's just a description of what the pref is meant to do ;-)
 
> I think it might be nice if we enabled the "Go » Forward" and "Go » Back"
> buttons since we're enabling history, but that could be a followup bug.

Well, the little back and forward buttons within the tab are enabled ;-) but yes we could look at Forward and Back for content tabs in general.

> I also wonder what sort of testing we can do here, but that, too, can be a
> followup patch.

I'm assuming that the core of the extension manager is tested in core. However, there's things like installing add-ons and other notifications that we can check we're getting in Thunderbird, but we need to get this usable on trunk first, and I think there will be a few follow-ups required anyway.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 476767 [details] [diff] [review]
[checked in] Part 1 - the basics v2

Checked in: http://hg.mozilla.org/comm-central/rev/1804f0251383

Comment 14

[Joe Sabash [:JoeS1]]

Although the AOM is basically functional now (Thanks), I'm seeing the following error message after opening:
Error: [Exception... "Component returned failure code: 0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED) [nsIDocShellHistory.useGlobalHistory]"  nsresult: "0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED)"  location: "JS frame :: chrome://global/content/bindings/browser.xml ::  :: line 649"  data: no]
Source file: chrome://global/content/bindings/browser.xml
Line: 653

Comment 15

[Mark Banner (:standard8)]

Attachment: Part 2 - handle notifications (WIP)

This is a work in progress patch that should handle the notifications from the add-on manager for blocked installs, complete installs etc. Still needs some tweaks and an icon, also needs mozmill tests.

Comment 16

[Mark Banner (:standard8)]

Attachment: Part 2 - handle notifications (WIP v2)

This one has a couple of fixes plus unit tests. Unfortunately not quite ready yet, I'm still trying to figure out why installInfo.installs.some comes back with reference to undefined property installInfo.installs, when exactly the same code in FF works fine...

(this is currently breaking the install complete notification).

Comment 17

[Mark Banner (:standard8)]

Attachment: [checked in] Part 2 - handle notifications v1

I've finally got this working. There were some bogus javascript strict warnings about installInfo.installs being undefined despite the fact it works fine...

This gets all the notifications working via the notification bar. This code is pretty much a copy-paste and adapt of what Firefox has.

The strings are a direct copy and paste. The notification bar icons I've stolen from 1.9.2 branch as they look the same and they are the right size (Firefox replaced the notification bar with their doorhanger notification so they have different icons now).

Comment 18

[[e-mail]]

(In reply to comment #14)
> Although the AOM is basically functional now (Thanks), I'm seeing the following
> error message after opening:
> Error: [Exception... "Component returned failure code: 0x80040154
> (NS_ERROR_FACTORY_NOT_REGISTERED) [nsIDocShellHistory.useGlobalHistory]" 
> nsresult: "0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED)"  location: "JS frame
> :: chrome://global/content/bindings/browser.xml ::  :: line 649"  data: no]
> Source file: chrome://global/content/bindings/browser.xml
> Line: 653
Removing disablehistory="true" from content tabs makes them try to wire up both session history and global history (nsIGlobalHistory2 - not to be confused with IHistory which is apparently permanently wired up).

Comment 19

[Rain [:rain1] (inactive)]

Comment on attachment 503816 [details] [diff] [review]
[checked in] Part 2 - handle notifications v1

on file: mail/base/content/specialTabs.js line 880
>     get _prefService() {
>       delete this._prefService;
>       return this._prefService =
>         Components.classes["@mozilla.org/preferences-service;1"]
>                   .getService(Components.interfaces.nsIPrefBranch2);
>     },

Do we still need this?


on file: mail/base/content/specialTabs.js line 892
>       let installInfo = aSubject.QueryInterface(Ci.amIWebInstallInfo);

You don't need to actually assign to a different variable here, but keeping it
is fine for clarity's sake.


on file: mail/base/content/specialTabs.js line 950
>         installInfo.installs.forEach(

Can we use for ([, install] in Iterator(installInfo.installs)) instead?


on file: mail/base/content/specialTabs.js line 951
>           function (aInstall) {
>             let host = (installInfo.originatingURI instanceof Ci.nsIStandardURL) &&
>                        installInfo.originatingURI.host;
>             if (!host)
>               host = (aInstall.sourceURI instanceof Ci.nsIStandardURL) &&
>                      aInstall.sourceURI.host;

As the comment in Firefox's browser.js
<https://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js#731>
says, this isn't ideal for the multiple failures case, since we might use the
same host over and over again. Please add that TODO comment here.


on file: mail/base/content/specialTabs.js line 1011
>               installInfo.installs.forEach(function(aInstall) {

for-in-Iterator?


on file: mail/test/mozmill/content-tabs/test-install-xpi.js line 94
>   // The install dialog has a count down that we must wait for before
>   // proceeding.
>   mc.sleep(5000);

Isn't the countdown 5 seconds? I'm worried this is cutting it too fine.


on file: mail/test/mozmill/content-tabs/test-install-xpi.js line 158
>   // After closing the dialog we need to give just a little extra time
>   // before we do things.

Why do we need to do this?


on file: mail/test/mozmill/content-tabs/test-install-xpi.js line 164
>   let prefBranch = Cc["@mozilla.org/preferences-service;1"]
>                      .getService(Ci.nsIPrefBranch);

Import Services.jsm instead, please.


on file: mail/test/mozmill/shared-modules/test-content-tab-helpers.js line 94
>   if (aClickHandler == undefined)
>     aClickHandler = "";

1. (aClickHandler === undefined) please.
2. Please assign null instead.

Looks good in general -- I just need to run with the patch.

Comment 20

[Rain [:rain1] (inactive)]

Comment on attachment 503816 [details] [diff] [review]
[checked in] Part 2 - handle notifications v1

>diff --git a/mail/themes/gnomestripe/mail/update.png b/mail/themes/gnomestripe/mail/update.png
>new file mode 100644
>index 0000000000000000000000000000000000000000..920e7cc108b804bcd409982024d754f29a3d96c4

You need to move this to mail/icons/update.png.

>diff --git a/mail/themes/qute/mail/update.png b/mail/themes/qute/mail/update.png
>new file mode 100644
>index 0000000000000000000000000000000000000000..e2737058f849ffc81b05eb8f2e1963b8f6ce547c

and this.

Looks good to me, modulo the comments above.

Comment 21

[Mark Banner (:standard8)]

(In reply to comment #19)
> on file: mail/test/mozmill/content-tabs/test-install-xpi.js line 94
> >   // The install dialog has a count down that we must wait for before
> >   // proceeding.
> >   mc.sleep(5000);
> 
> Isn't the countdown 5 seconds? I'm worried this is cutting it too fine.

On here I was seeing 3 seconds ish, but I've made it 5.5 just in case.

> on file: mail/test/mozmill/content-tabs/test-install-xpi.js line 158
> >   // After closing the dialog we need to give just a little extra time
> >   // before we do things.
> 
> Why do we need to do this?

I believe that we basically just need to let things happen - i.e. let the core code finish up and tidy itself before we need try and do something else.

Comment 22

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 503816 [details] [diff] [review]
[checked in] Part 2 - handle notifications v1

Inline with FF so there isn't really anything I want to change here.  ui-r+

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 503816 [details] [diff] [review]
[checked in] Part 2 - handle notifications v1

Checked in:

http://hg.mozilla.org/comm-central/rev/ebb269ce8610

Comment 24

[Mark Banner (:standard8)]

The add-on manager is now largely functional, whilst there are a few other issues around it still (e.g. bug 612884), we'll deal with those in follow-up bugs.

Comment 25

[Rain [:rain1] (inactive)]

Pushed a followup: https://hg.mozilla.org/comm-central/rev/88032a4ef838