Closed Bug 571759 Opened 9 years ago Closed 9 years ago

Move Add-on Manager to be in a content tab

Categories

(Thunderbird :: Toolbars and Tabs, defect)

defect
Not set

Tracking

(blocking-thunderbird5.0 needed)

RESOLVED FIXED
Thunderbird 3.3a2
Tracking Status
blocking-thunderbird5.0 --- needed

People

(Reporter: standard8, Assigned: standard8)

References

(Blocks 1 open bug)

Details

(Whiteboard: [UXprio])

Attachments

(2 files, 4 obsolete files)

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.
How is this going to affect users who don't use tabs and hide the tab bar?
(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.
Excellent. That will work just fine.
Keywords: helpwanted
Attached patch Partial fix (obsolete) — Splinter Review
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.
Duplicate of this bug: 565476
This needs to fix the notifications as well (xref bug 567127) so that installation works as well.
Flags: blocking-thunderbird-next+
Duplicate of this bug: 591965
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)
Attached patch Part 1 - the basics (obsolete) — Splinter Review
(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.
Assignee: nobody → bugzilla
Attachment #453068 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #476752 - Flags: review?(bwinton)
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.
Attachment #476752 - Attachment is obsolete: true
Attachment #476767 - Flags: review?(bwinton)
Attachment #476752 - Flags: review?(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.
Attachment #476767 - Flags: review?(bwinton) → review+
(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 on attachment 476767 [details] [diff] [review]
[checked in] Part 1 - the basics v2

Checked in: http://hg.mozilla.org/comm-central/rev/1804f0251383
Attachment #476767 - Attachment description: Part 1 - the basics v2 → [checked in] Part 1 - the basics v2
Blocks: 599331
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
Status: ASSIGNED → UNCONFIRMED
Ever confirmed: false
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
blocking-thunderbird5.0: --- → needed
Flags: blocking-thunderbird-next+
Blocks: 612884
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.
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).
Attachment #499390 - Attachment is obsolete: true
Blocks: 550844
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).
Attachment #502508 - Attachment is obsolete: true
Attachment #503816 - Flags: ui-review?(clarkbw)
Attachment #503816 - Flags: review?(sid.bugzilla)
(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 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 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.
Attachment #503816 - Flags: review?(sid.bugzilla) → review+
(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 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+
Attachment #503816 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 503816 [details] [diff] [review]
[checked in] Part 2 - handle notifications v1

Checked in:

http://hg.mozilla.org/comm-central/rev/ebb269ce8610
Attachment #503816 - Attachment description: Part 2 - handle notifications v1 → [checked in] Part 2 - handle notifications v1
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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Keywords: helpwanted
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a2
Depends on: 626019
Blocks: 616176
Blocks: 1006258
You need to log in before you can comment on or make changes to this bug.