393108 - Make use of notification bar in SeaMonkey when an XP Install has been blocked

Status: RESOLVED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Teune van Steeg]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007082003 SeaMonkey/2.0a1pre

When bug #270443 has been fixed, SeaMonkey can use the notification bar to notify users when an XP Install has been blocked.  

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Teune van Steeg]

Attachment: patch

This adds a special notificationbox binding which handles notifications for blocked XPInstalls.

Comment 2

[[e-mail]]

Comment on attachment 287565 [details] [diff] [review]
patch

I wasn't able to trigger a notification when xpinstall was disabled. Bug?

>+.browser-notificationbox {
>+  -moz-binding: url("chrome://navigator/content/browserNotification.xml#browser-notificationbox");
>+}
By comparison with our other toolkit overrides this should be chrome://communicator/content/bindings/notification.xml

>+      <field name="_stringBundleURI" readonly="true">
>+        "chrome://navigator/locale/browserNotification.properties"
>+      </field>
>+
>+      <field name="_brandStringBundleURI" readonly="true">
>+        "chrome://branding/locale/brand.properties"
>+      </field>
Because of a recent crash fix, fields now only evaluate on first use. This means that you can inline the code to get the string bundle into the field. If you don't like that idea, then I'd prefer separate properties for the two bundles, rather than the method and fields approach.

>+                var brandStringBundle = this.getStringBundle(this._brandStringBundleURI);
>+                var brandShortName = brandStringBundle.GetStringFromName("brandShortName");
You only use this once, no point getting it in the other cases.

>+                    callback: function() {
Interestingly you didn't name this callback ;-)

>+      <destructor>
>+        <![CDATA[
>+          var os = Components.classes["@mozilla.org/observer-service;1"]
>+                             .getService(Components.interfaces.nsIObserverService);
>+          os.removeObserver(this, "xpinstall-install-blocked");
>+        ]]>
>+      </destructor>
I'm not sure you can actually rely on the destructor running;
removeTab has to call destroy().

Comment 3

[Teune van Steeg]

Attachment: patch (v2)

Addresses nits.

I'm actually not sure if _stringBundle and _brandStringBundle in notification.xml should be cleared also on destroy().

Comment 4

[[e-mail]]

Comment on attachment 287771 [details] [diff] [review]
patch (v2)

Sorry for the delay.

>+xpinstallPromptAllowButton=Allow
I've decided that Allow is too confusing. Please use use this instead:
xpinstallPromptInstallButton=Install...

Comment 5

[Teune van Steeg]

Attachment: patch (for check in)

Replace "Allow" with "Install..."

Comment 6

[Robert Kaiser]

Comment on attachment 288202 [details] [diff] [review]
patch (for check in)

checked in by request of twanno via IRC

Comment 7

[[e-mail]]

Comment on attachment 288202 [details] [diff] [review]
patch (for check in)

>Index: suite/browser/navigator.css
>===================================================================
>RCS file: /cvsroot/mozilla/suite/browser/navigator.css,v
>retrieving revision 1.15
>diff -u -8 -p -r1.15 navigator.css
>--- suite/browser/navigator.css	29 Jul 2007 23:15:31 -0000	1.15
>+++ suite/browser/navigator.css	10 Nov 2007 23:35:30 -0000
>@@ -17,16 +17,22 @@ tabbrowser {
> .tabbrowser-tabs > .tabbrowser-tab {
>   -moz-binding: url("chrome://navigator/content/tabbrowser.xml#tabbrowser-tab");
> }
> 
> .tabs-closebutton-box > .tabs-closebutton {
>   -moz-binding: url("chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton");
> }
> 
>+/* ::::: notification box ::::: */
>+
>+.browser-notificationbox {
>+  -moz-binding: url("chrome://communicator/content/bindings/notification.xml#browser-notificationbox");
>+}
>+
Whoops, this belongs in communicator.css - sidebars in other windows break :-(