Closed Bug 393108 Opened 12 years ago Closed 12 years ago

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

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: twanno, Assigned: twanno)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Depends on: 252830, 270443
Version: unspecified → Trunk
Depends on: 393857
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch patch (obsolete) — Splinter Review
This adds a special notificationbox binding which handles notifications for blocked XPInstalls.
Assignee: nobody → twanno
Status: NEW → ASSIGNED
Attachment #287565 - Flags: superreview?(neil)
Attachment #287565 - Flags: review?(neil)
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().
Attached patch patch (v2) (obsolete) — Splinter Review
Addresses nits.

I'm actually not sure if _stringBundle and _brandStringBundle in notification.xml should be cleared also on destroy().
Attachment #287565 - Attachment is obsolete: true
Attachment #287771 - Flags: superreview?(neil)
Attachment #287771 - Flags: review?(neil)
Attachment #287565 - Flags: superreview?(neil)
Attachment #287565 - Flags: review?(neil)
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...
Attachment #287771 - Flags: superreview?(neil)
Attachment #287771 - Flags: superreview+
Attachment #287771 - Flags: review?(neil)
Attachment #287771 - Flags: review+
Replace "Allow" with "Install..."
Attachment #287771 - Attachment is obsolete: true
Attachment #288202 - Flags: superreview+
Attachment #288202 - Flags: review+
Comment on attachment 288202 [details] [diff] [review]
patch (for check in)

checked in by request of twanno via IRC
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M10
Blocks: 404066
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 :-(
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.