Closed Bug 954147 Opened 10 years ago Closed 10 years ago

No UI feedback when attempting to install an incompatible add-on

Categories

(Instantbird Graveyard :: Other, defect)

x86
Other
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: clokep)

References

()

Details

(Whiteboard: [0.3-blocking])

Attachments

(2 files, 4 obsolete files)

*** Original post on bio 712 at 2011-02-21 12:45:00 UTC ***

When installing an incompatible add-on, there's absolutely no UI feedback. The dialog saying an add-on is about to be installed doesn't appear. No error/warning message appear.

In the error console, I have:
3 times: Warning: WARN addons.repository: cacheEnabled: Couldn't get pref: extensions.getAddons.cache.enabled
Source File: resource://gre/modules/AddonRepository.jsm
Line: 356 (probably related to bug 953967 (bio 529))

addons.instantbird.org : server does not support RFC 5746, see CVE-2009-3555
(server side bug, but it shows the add-on manager attempts to fetch something from the server at that point. Maybe checking for a compatibility update before notifying the user of the incompatibility?)

Warning: WARN addons.updates: Error: Missing updates property for urn:mozilla:extension:<add-on id>
Source File: resource://gre/modules/AddonUpdateChecker.jsm
Line: 472
*** Original post on bio 712 at 2011-02-21 12:48:59 UTC ***

It's at least the third time today that someone in #instantbird needs support because of this. This would probably confuse lots of users upgrading from 0.2 to 0.3beta or 0.3 final, so I think fixing this blocks 0.3.
Whiteboard: [0.3-blocking]
*** Original post on bio 712 at 2011-03-03 01:14:57 UTC ***

I believe that this is the same issue Thunderbird had: https://bugzilla.mozilla.org/show_bug.cgi?id=585443 pretty much Firefox switched to using doorhanger notifications for this...which Instantbird (and Thunderbird) don't support.
*** Original post on bio 712 at 2011-03-23 12:16:58 UTC ***

I've started working on this and have a pretty good idea of what needs to be done...now it's just doing it. The specific patch that needs porting is https://bugzilla.mozilla.org/attachment.cgi?id=503816&action=diff . Which I have done and is accepting notifications, just working on the UI bits now.
Assignee: nobody → clokep
Severity: normal → major
Status: NEW → ASSIGNED
Attached patch WIP (obsolete) — Splinter Review
*** Original post on bio 712 as attmnt 572 at 2011-03-26 03:39:00 UTC ***

I have a partial patch that contains some errors, but I wanted to get up it up somewhere so I don't lose it and people can check it over.

This should have a copy of themes and needs to clean up some property files.
Attached image Example of WIP patch (obsolete) —
*** Original post on bio 712 as attmnt 573 at 2011-03-26 03:40:00 UTC ***

The colors are pretty bad on this, but it's the results of the current patch.
Attached patch v1.0 (obsolete) — Splinter Review
*** Original post on bio 712 as attmnt 579 at 2011-04-08 22:49:00 UTC ***

Here's a working version that I haven't found any issues with. It probably needs to be checked on Mac and Linux (which I don't have access to right now).
Attachment #8352321 - Flags: review?(florian)
Comment on attachment 8352314 [details] [diff] [review]
WIP

*** Original change on bio 712 attmnt 572 at 2011-04-08 22:49:58 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352314 - Attachment is obsolete: true
Comment on attachment 8352315 [details]
Example of WIP patch

*** Original change on bio 712 attmnt 573 at 2011-04-08 22:49:58 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352315 - Attachment is obsolete: true
Comment on attachment 8352321 [details] [diff] [review]
v1.0

*** Original change on bio 712 attmnt 579 at 2011-04-11 17:15:46 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352321 - Flags: review?(florian) → review-
*** Original post on bio 712 at 2011-04-11 17:15:47 UTC ***

Comment on attachment 8352321 [details] [diff] [review] (bio-attmnt 579)
v1.0

>diff --git a/instantbird/content/extensions.js b/instantbird/content/extensions.js

>+//Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");

Remove :).


>+/**
>+ * This function is global and expected by toolkit to get the notification box
>+ * for the browser for use with items like password manager.
>+ */
>+function getNotificationBox() {
>+  return document.getElementById("addonsNotify");
>+}

I don't think this comment applies for our use case. You can most likely use document.getElementById("addonsNotify") directly in xpInstallObserver.observe. 

>+var addonsRegister = {
>+  onload: function () {
>+    Services.obs.addObserver(xpInstallObserver, "addon-install-disabled",
>+                             false);
[...]
>+  onunload: function () {
[...]
>+    Services.obs.removeObserver(specialTabs.xpInstallObs,
>+                                "addon-install-disabled");
So you are adding the observers on xpInstallObserver and removing them on specialTabs.xpInstallObs (which doesn't seem to exist)?

>+}
Missing semi colon.

>+// xpInstallObserver is taken (unmodified) from
>+// comm-central/source/mail/base/content/specialTabs.js

I'm not sure if this means we should try to modify it as little as possible to make it easy to apply future improvements to this file... but there are several things that don't look great in that code.

>+var xpInstallObserver = {

This object contains a single function? You can include it in your addonsRegister object.

>+  observe: function (aSubject, aTopic, aData) {
>+    const Ci = Components.interfaces;
>+    let brandBundle = document.getElementById("bundle_brand");
>+    let messengerBundle = document.getElementById("bundle_messenger");
The name "messenger" isn't great here. I would prever "extensions"...

>+    let win = installInfo.originatingWindow;
>+    let notificationBox = getNotificationBox(win.top);
If we are sure all notifications will always be displayed in the add-on window, you can just make a getElementsById here.
That won't work well if we start supporting installing an add-on by dragging it on any window of the application, but I guess we can still revert this change in that case.

>+    switch (aTopic) {
>+    case "addon-install-disabled":

>+      if (notificationBox && !notificationBox.getNotificationWithValue(notificationID)) {
It seems this function won't do anything if notificationBox is null. So maybe it should just test it once after getting it, and return early if it's null?

>+        notificationBox.appendNotification(messageString, notificationID,
>+                                           iconURL,
>+                                           notificationBox.PRIORITY_CRITICAL_HIGH,
>+                                           buttons);

It seems the whole notificationBox.appendNotification call could be factored out, with a single additional variable for the notification priority.

>+    case "addon-install-blocked":

>+      if (notificationBox && !notificationBox.getNotificationWithValue(notificationName)) {
>+          notificationBox.appendNotification(messageString, notificationName,
The indent is wrong here (4 spaces instead of 2).

>+    case "addon-install-failed":
>+      // XXX TODO This isn't terribly ideal for the multiple failure case
>+      for (let [, install] in Iterator(installInfo.installs)) {
>+        let host = (installInfo.originatingURI instanceof Ci.nsIStandardURL) &&
>+                    installInfo.originatingURI.host;
>+        if (!host)
>+          host = (install.sourceURI instanceof Ci.nsIStandardURL) &&
>+                  install.sourceURI.host;
This syntax usage is strange. Either test with if[1] or use logical operators[2]...

[1]
let host;
if (installInfo.originatingURI instanceof Ci.nsIStandardURL)
  host = installInfo.originatingURI.host;
if (!host && install.sourceURI instanceof Ci.nsIStandardURL)
  host = install.sourceURI.host;

[2]
let host = (installInfo.originatingURI instanceof Ci.nsIStandardURL) &&
           installInfo.originatingURI.host ||
           (install.sourceURI instanceof Ci.nsIStandardURL) &&
           install.sourceURI.host;

All the 3 are hard to read anyway :(

>+    case "addon-install-complete":
>+      let needsRestart = installInfo.installs.some(function(i) {
>+          return i.addon.pendingOperations != AddonManager.PENDING_NONE;
Wrong indent.

>+              openAddonsMgr("addons://list/" + bestType);
Is this function defined anywhere?

>+  }
>+}
Missing semi colon.

>diff --git a/instantbird/content/extensions.xul b/instantbird/content/extensions.xul

It would be nice to have a menu bar on mac in this window.
For that I think you only need:
#ifdef XP_MACOSX
<?xul-overlay href="chrome://instantbird/content/menus.xul"?>
#endif

(inside the window tag)
#ifdef XP_MACOSX
#include menus.xul.inc
#endif


>+<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
>+
>+<window id="ib-addons-window" title="Add-ons Manager" width="900" height="600"
Should the title attribute be localized?
The string is in chrome://mozapps/locale/extensions/extensions.dtd.
Not sure if we should use that dtd file or copy the title of the document loaded in the browser on the onload event. Either would work I guess.

>+  <popupset id="mainPopupSet">
>+    <!-- for search and content formfill/pw manager -->
>+    <panel type="autocomplete" id="PopupAutoComplete" noautofocus="true"/>
>+  </popupset>
Is this used?

>+  <vbox flex="1">
>+    <notificationbox id="addonsNotify" flex="1">
>+      <browser id="dummychromebrowser" flex="1"
>+               disablesecurity="true" disablehistory="true"
>+               autocompletepopup="PopupAutoComplete"
>+               src="chrome://mozapps/content/extensions/extensions.xul?type=extensions"/>
>+    </notificationbox>
>+  </vbox>
What's the vbox used for?

>diff --git a/instantbird/content/jar.mn b/instantbird/content/jar.mn
>--- a/instantbird/content/jar.mn
>+++ b/instantbird/content/jar.mn
>@@ -28,6 +28,8 @@
> *	instantbird/credits.xhtml
> *	instantbird/engineManager.js
> *	instantbird/engineManager.xul
>+	instantbird/extensions.js
>+	instantbird/extensions.xul
If you decide to go ahead an preprocess the window to add a mac menubar, don't forget to add a * here :).

>diff --git a/instantbird/locales/en-US/chrome/instantbird/messenger.properties b/instantbird/locales/en-US/chrome/instantbird/messenger.properties

I suggested a messenger.properties -> extensions.properties rename above.

In any case, please include a localization note explaining where translators should copy paste these strings from instead of retranslating them. You took them from messenger.properties. It seems most (all?) of them are actually from browser.properties.

You missed all the addon error strings:

# LOCALIZATION NOTE (addonError-1, addonError-2, addonError-3, addonError-4):
# #1 is the add-on name, #2 is the host name, #3 is the application name
# #4 is the application version
addonError-1=The add-on could not be downloaded because of a connection failure on #2.
addonError-2=The add-on from #2 could not be installed because it does not match the add-on #3 expected.
addonError-3=The add-on downloaded from #2 could not be installed because it appears to be corrupt.
addonError-4=#1 could not be installed because #3 cannot modify the needed file.

# LOCALIZATION NOTE (addonLocalError-1, addonLocalError-2, addonLocalError-3, addonLocalError-4, addonErrorIncompatible, addonErrorBlocklisted):
# #1 is the add-on name, #3 is the application name, #4 is the application version
addonLocalError-1=This add-on could not be installed because of a filesystem error.
addonLocalError-2=This add-on could not be installed because it does not match the add-on #3 expected.
addonLocalError-3=This add-on could not be installed because it appears to be corrupt.
addonLocalError-4=#1 could not be installed because #3 cannot modify the needed file.
addonErrorIncompatible=#1 could not be installed because it is not compatible with #3 #4.
addonErrorBlocklisted=#1 could not be installed because it has a high risk of causing stability or security problems.
Attached patch v1.1 (obsolete) — Splinter Review
*** Original post on bio 712 as attmnt 582 at 2011-04-12 01:00:00 UTC ***

I've addressed all of your comments I think.

(In reply to comment #7)
> >+    case "addon-install-complete":
> >+      let needsRestart = installInfo.installs.some(function(i) {
> >+          return i.addon.pendingOperations != AddonManager.PENDING_NONE;
> Wrong indent.
> 
> >+              openAddonsMgr("addons://list/" + bestType);
> Is this function defined anywhere?
So...I did find the function, but I copied it in verbatim cause I honestly can't figure out what it's doing. This is going to need (at least) one more version then; any ideas what this function is doing flo?
Attachment #8352324 - Flags: review?(florian)
Comment on attachment 8352321 [details] [diff] [review]
v1.0

*** Original change on bio 712 attmnt 579 at 2011-04-12 01:00:04 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352321 - Attachment is obsolete: true
*** Original post on bio 712 by Mook <mook.moz+bugs.instantbird AT gmail.com> at 2011-04-12 03:08:09 UTC ***

Driving by, woo! Didn't actually read the patch, just the bug comments, so feel free to ignore me for being silly.

(In reply to comment #7)

> >diff --git a/instantbird/content/extensions.xul b/instantbird/content/extensions.xul
> 
> It would be nice to have a menu bar on mac in this window.
> For that I think you only need:
> #ifdef XP_MACOSX
> <?xul-overlay href="chrome://instantbird/content/menus.xul"?>
> #endif
> 
> (inside the window tag)
> #ifdef XP_MACOSX
> #include menus.xul.inc
> #endif

You can also do this in jar.mn:

% overlay chrome://instantbird/content/extensions.xul chrome://instantbird/content/menus.xul os=Darwin

This has the advantage of not preprocessing and yet being platform-dependent.

(In reply to comment #8)
> Created an attachment (id=582) [details]

> > >+              openAddonsMgr("addons://list/" + bestType);
> > Is this function defined anywhere?
> So...I did find the function, but I copied it in verbatim cause I honestly
> can't figure out what it's doing. This is going to need (at least) one more
> version then; any ideas what this function is doing flo?

It's sending an app-wide observer notification to try to find an already-open about:addons and bring that up; it then tries to switch to the correct <tabbrowser> tab (in #tabmail, so probably not whatever instantbird has), then tells about:addons to switch to the right type of addon (the argument given).

Assuming (ha!) there should only be one about:addons open at a time, consider using Services.wm to find things instead.  You probably don't have openContentTab that function needs anyway ;)
*** Original post on bio 712 at 2011-04-12 20:59:34 UTC ***

(In reply to comment #9)

> (In reply to comment #7)
> 
> > >diff --git a/instantbird/content/extensions.xul b/instantbird/content/extensions.xul
> > 
> > It would be nice to have a menu bar on mac in this window.
> > For that I think you only need:
> > #ifdef XP_MACOSX
> > <?xul-overlay href="chrome://instantbird/content/menus.xul"?>
> > #endif
> > 
> > (inside the window tag)
> > #ifdef XP_MACOSX
> > #include menus.xul.inc
> > #endif
> 
> You can also do this in jar.mn:
> 
> % overlay chrome://instantbird/content/extensions.xul
> chrome://instantbird/content/menus.xul os=Darwin
> 
> This has the advantage of not preprocessing and yet being platform-dependent.

I thought about it too, but it works only to add the overlay, not for the mount points that we need to include, so we still need to preprocess the file, unfortunately...
And by the way, os=Darwin is not really necessary, as the jar.mn file is not going to be preprocessor-free any time soon ;).

[completely off-topic for this bug: I looked at these ugly ifdef XP_MACOSX everywhere too when I reviewed the previous patch here. If we want to simplify things, we can probably remove Mac-only inclusion of instantbird.dtd, as it's included in menus.xul anyway. I think it's a leftover from before your menu cleanup patch.]
Comment on attachment 8352324 [details] [diff] [review]
v1.1

*** Original change on bio 712 attmnt 582 at 2011-04-12 21:45:08 UTC ***

>diff --git a/instantbird/content/extensions.js b/instantbird/content/extensions.js

>+    let win = installInfo.originatingWindow;
This variable isn't used anymore.

>+    let notificationBox = document.getElementById("addonsNotify");

Have you seen this comment?
"It seems this function won't do anything if notificationBox is null. So maybe
it should just test it once after getting it, and return early if it's null?"

Hmm... or maybe we don't need to test it at all, as we are sure there'll be an element with that id in the document. Anyway, the current 4 null checks are excessive.

>+          callback: function() {
>+            // Calculate the add-on type that is most popular in the list of
>+            // installs.
>+            let types = {};
>+            let bestType = null;
>+            for (let [, install] in Iterator(installInfo.installs)) {
>+              if (install.type in types)
>+                types[install.type]++;
>+              else
>+                types[install.type] = 1;
>+
>+              if (!bestType || types[install.type] > types[bestType])
>+                bestType = install.type;
>+
>+              openAddonsMgr("addons://list/" + bestType);
This line was most likely meant to be outside of the for loop.

Mook explained what that function does in comment 9.
We can only load one extension manager at a time, and we are sure this code is executed from the window where the add-on manager is loaded.
So I think you can replace this by just:

document.getElementById("dummychromebrowser").contentWindow.loadView("addons://list/" + bestType);

(wrap it of course... and test it :))



>diff --git a/instantbird/content/extensions.xul b/instantbird/content/extensions.xul

>+<!DOCTYPE page [
What is this "page" doc type? Usually I see "window" here.

>+<!ENTITY % extensionsDTD SYSTEM "chrome://mozapps/locale/extensions/extensions.dtd">
>+%extensionsDTD;
Please indent these 2 lines.



>diff --git a/instantbird/content/menus.js b/instantbird/content/menus.js

>diff --git a/instantbird/content/preferences/main.js b/instantbird/content/preferences/main.js

>     if (!this.focus("Extension:Manager"))

I think this used to work, but it clearly doesn't work with the new add-on manager. Could you please replace "Extension:Manager" by "Addons:Manager" in both preferences/main.js and menus.js?



>diff --git a/instantbird/locales/en-US/chrome/instantbird/extensions.properties b/instantbird/locales/en-US/chrome/instantbird/extensions.properties
>new file mode 100644
>--- /dev/null
>+++ b/instantbird/locales/en-US/chrome/instantbird/extensions.properties

>+# The following are used by the messenger application

This comment doesn't make sense here.

>+# Originally from comm-central/source/mail/locales/en-US/chrome/messenger/messenger.properties

If all the strings are shared, maybe localizers should copy the string from browser.properties (which exists in mozilla-central, without having to look for comm-central).

And I'm afraid the "You missed all the addon error strings" comment of the previous review still applies :(.
Attachment #8352324 - Flags: review?(florian) → review-
Attached patch v1.2Splinter Review
*** Original post on bio 712 as attmnt 584 at 2011-04-13 00:13:00 UTC ***

I think this is the final version. Sorry for skipping a few comments in the last review. This should meet them all though.
Attachment #8352326 - Flags: review?(florian)
Comment on attachment 8352324 [details] [diff] [review]
v1.1

*** Original change on bio 712 attmnt 582 at 2011-04-13 00:13:13 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352324 - Attachment is obsolete: true
Comment on attachment 8352326 [details] [diff] [review]
v1.2

*** Original change on bio 712 attmnt 584 at 2011-04-13 09:37:10 UTC ***

>diff --git a/instantbird/content/preferences/main.js b/instantbird/content/preferences/main.js
>--- a/instantbird/content/preferences/main.js
>+++ b/instantbird/content/preferences/main.js
>@@ -74,8 +74,7 @@
>    */
>   showAddonsMgr: function ()
>   {
>-    const EMURL =
>-      "chrome://mozapps/content/extensions/extensions.xul?type=extensions";
>+    const EMURL = "chrome://instantbird/content/extensions.xul";
> 
>     if (!this.focus("Extension:Manager"))
>       window.open(EMURL, "Addons",

Hmmm... (In comment #11)
> (From update of attachment 8352324 [details] [diff] [review] (bio-attmnt 582) [details])
 
> >diff --git a/instantbird/content/menus.js b/instantbird/content/menus.js
> 
> >diff --git a/instantbird/content/preferences/main.js b/instantbird/content/preferences/main.js
> 
> >     if (!this.focus("Extension:Manager"))
> 
> I think this used to work, but it clearly doesn't work with the new add-on
> manager. Could you please replace "Extension:Manager" by "Addons:Manager" in
> both preferences/main.js and menus.js?

Looks good otherwise :-) (I still need to test it though).
Attachment #8352326 - Flags: review?(florian) → review+
*** Original post on bio 712 at 2011-04-13 10:06:23 UTC ***

So, after testing:
 - You have DOS line endings in the 3 new files.
 - Displaying in the add-on manager a button labeled "Open Add-on manager" is silly.
 - When clicking that button, I have "JavaScript strict warning: chrome://instantbird/content/extensions.js, line 191: reference to undefined property installInfo.installs". I suspect the content of the installInfo object is garbage collected before the callback is called by my click on the button.

When a restartless add-on is installed, I would suggest displaying the notification bar without button and switching to the correct view immediately.
*** Original post on bio 712 at 2011-04-13 10:14:31 UTC ***

And you removed the ifdef XP_MACOSX from extensions.xul :(
*** Original post on bio 712 as attmnt 585 at 2011-04-13 10:35:00 UTC ***

Additional changes to fix issues described in comment 13-15 for attachment 8352326 [details] [diff] [review] (bio-attmnt 584). The line endings also need to be changed, otherwise the preprocessor doesn't work.
*** Original post on bio 712 at 2011-04-13 10:40:31 UTC ***

https://hg.instantbird.org/instantbird/rev/0373f787e44b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.3a2
Blocks: 954188
You need to log in before you can comment on or make changes to this bug.