Last Comment Bug 521159 - [SeaMonkey 2.1] Port |Bug 514327 - Detect outdated plugins and offer upgrade path|
: [SeaMonkey 2.1] Port |Bug 514327 - Detect outdated plugins and offer upgrade ...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Startup & Profiles (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
: 536114 573391 (view as bug list)
Depends on: 514327
Blocks: 595810
  Show dependency treegraph
 
Reported: 2009-10-07 18:57 PDT by Serge Gautherie (:sgautherie)
Modified: 2011-08-04 14:41 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wanted


Attachments
patch v1 (9.90 KB, patch)
2011-02-08 13:15 PST, Adrian Kalla [:adriank]
iann_bugzilla: review-
Details | Diff | Splinter Review
One way of avoiding code duplication (5.49 KB, patch)
2011-02-11 06:52 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
One way of avoiding code duplication (5.49 KB, patch)
2011-02-11 06:55 PST, neil@parkwaycc.co.uk
iann_bugzilla: feedback+
Details | Diff | Splinter Review
Move code around to avoid duplication (3.99 KB, patch)
2011-02-11 07:13 PST, neil@parkwaycc.co.uk
iann_bugzilla: feedback+
Details | Diff | Splinter Review
Tweaked patch (5.09 KB, patch)
2011-03-20 09:41 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Splinter Review
With added pluginUnavailable sharing (10.87 KB, patch)
2011-03-20 09:43 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Splinter Review
Fixed patch (8.45 KB, patch)
2011-03-22 06:33 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Splinter Review
Actually handle the event (3.45 KB, patch)
2011-03-22 17:49 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Splinter Review
Pref observer (3.47 KB, patch)
2011-03-23 06:06 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Splinter Review
Startup notification check (2.82 KB, patch)
2011-03-23 10:14 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2009-10-07 18:57:42 PDT

    
Comment 1 Krang 2009-12-21 20:32:40 PST
*** Bug 536114 has been marked as a duplicate of this bug. ***
Comment 2 Bruno 'Aqualon' Escherl 2010-09-09 07:46:58 PDT
This will also fix some of the test warnings we get because of the missing plugins.update.url pref.

I'll look if I can give this a shot next week.
Comment 3 Serge Gautherie (:sgautherie) 2010-09-09 08:45:07 PDT
*** Bug 573391 has been marked as a duplicate of this bug. ***
Comment 4 Robert Kaiser 2010-09-09 09:54:23 PDT
As we're in happy-dupe-to-empty-description-bugs time, please read bug 573391 comment #0 as well, which at least describes something.
Comment 5 Bruno 'Aqualon' Escherl 2010-10-20 07:21:33 PDT
I'm unassigning myself for the moment, since I have too much else to do the next few weeks. Perhaps I can work on it after the developer meeting, but I can't tell for sure.
Comment 6 Adrian Kalla [:adriank] 2011-02-08 13:15:58 PST
Created attachment 510749 [details] [diff] [review]
patch v1

first try to port the patch. I couldn't test it, as no simulated outdated-plugin-situation wants to make that notification appear even on Firefox on my Mac...
Comment 7 Robert Kaiser 2011-02-08 14:38:41 PST
Does Firefox have an automated test for this? If so, 1) we should port it, 2) we should be able to use it for testing this.
Comment 8 Ian Neal 2011-02-09 14:03:59 PST
(In reply to comment #7)
> Does Firefox have an automated test for this? If so, 1) we should port it, 2)
> we should be able to use it for testing this.

Doesn't seem to be, only one for missing and blocked which we have already ported (browser_pluginnotification.js).
Comment 9 neil@parkwaycc.co.uk 2011-02-10 02:19:31 PST
Comment on attachment 510749 [details] [diff] [review]
patch v1

Drive-by review:

>+          hideBarPrefName = event.type == "PluginOutdated" ?
>+                                  "plugins.hide_infobar_for_outdated_plugin" :
>+                                  "plugins.hide_infobar_for_missing_plugin";
We already know what the event type is here.

>+          if (this._prefs.getBoolPref(hideBarPrefName))
>+            return;
I notice that the missing-plugin notification is automatically hidden if someone turns the pref off.

>+          var pluginInfo = this.getPluginInfo(event.target);
>+          this.missingPlugins[pluginInfo.mimetype] = pluginInfo;
I was surprised to see this count as a missing plugin, but maybe that's because of the way Firefox uses one method to try to handle all plugin errors.

>+          var blockedNotification = this.getNotificationWithValue("blocked-plugins");
>+          var missingNotification = this.getNotificationWithValue("missing-plugins");
>+
>+          // Cancel any notification about blocklisting/missing plugins
>+          if (blockedNotification)
>+            blockedNotification.close();
>+          if (missingNotification)
>+            missingNotification.close();
Nit: would like to see these reordered so that you get the notification and then close it, and then get the other notification and then close it.

>+            callback: function getOutdatedPluginsInfo() {
I'll look into how best to avoid code duplication.

>+          const iconURL = "chrome://mozapps/skin/plugins/pluginOutdated-16.png";
>+          this.appendNotification(messageString, "outdated-plugins",
Need to add a Modern version of this icon at some point.

>+      this._showPluginUpdatePage();
Need to pass aWindow in as a parameter...

>+    var formatter = Cc["@mozilla.org/toolkit/URLFormatterService;1"].
>+                    getService(Ci.nsIURLFormatter);
No Cc/Ci in nsSuiteGlue.js, so
Components.classes["@mozilla.org/toolkit/URLFormatterService;1"]
          .getService(Components.interfaces.nsIURLFormatter);

>+
>+    var win = this.getMostRecentBrowserWindow();
>+    var browser = win.gBrowser;
... and use aWindow.gBrowser here instead.

>+    browser.selectedTab = browser.addTab(updateUrl);
Since this opens a tab, should the other notifications (if they happen to fire at the same time) show on this tab, or on the original tab?
Comment 10 Ian Neal 2011-02-10 18:10:50 PST
Comment on attachment 510749 [details] [diff] [review]
patch v1

You'll probably need to add something similar to http://mxr.mozilla.org/comm-central/source/suite/common/bindings/notification.xml?mark=207-214#207 for "plugins.hide_infobar_for_outdated_plugin"

Just repeating what I said on IRC on Tuesday.

>+      <handler event="PluginOutdated">
>+        <![CDATA[
>+          // Since we are expecting also untrusted events, make sure
>+          // that the target is a plugin
>+          if (!(event.target instanceof Components.interfaces.nsIObjectLoadingContent))
>+            return;
Add something like:
var notificationName = "outdated-plugins";
See later for more...

>+          hideBarPrefName = event.type == "PluginOutdated" ?
>+                                  "plugins.hide_infobar_for_outdated_plugin" :
>+                                  "plugins.hide_infobar_for_missing_plugin";
>+          if (this._prefs.getBoolPref(hideBarPrefName))
>+            return;
Already know this is a "PluginOutdated" event, so hideBarPrefName will always be "plugins.hide_infobar_for_outdated_plugin"

>+          // If there is already an outdated plugin notification then do nothing
>+          if (this.getNotificationWithValue("outdated-plugins"))
You can now use notificationName here.

>+            callback: function getOutdatedPluginsInfo() {
I'm glad you've changed the function name :)

Sound's like Neil is sorting out the helper function / removal of duplicate code but not sure if he wants you to leave this patch with duplicate code or wait for him to provide a way of not having the duplicate code...

>+          this.appendNotification(messageString, "outdated-plugins",
>+                                  iconURL, priority, buttons);
And you can use notificationName again here.

>+++ b/suite/locales/en-US/chrome/common/notification.properties
>@@ -1,12 +1,14 @@
> missingpluginsMessage.title=Additional plugins are required to display all the media on this page.
> missingpluginsMessage.button.label=Install Missing Plugins…
> missingpluginsMessage.button.accesskey=I
>-
>+outdatedpluginsMessage.title=Some plugins used by this page are out of date.
>+outdatedpluginsMessage.updateButton.label=Update Plugins…
>+outdatedpluginsMessage.updateButton.accesskey=U
> blockedpluginsMessage.title=Some plugins required by this page have been blocked for your protection.
> blockedpluginsMessage.infoButton.label=Details…
> blockedpluginsMessage.infoButton.accesskey=D
> blockedpluginsMessage.searchButton.label=Update Plugins…
> blockedpluginsMessage.searchButton.accesskey=U

Seems there is some duplication of entries using "Update Plugins..." lables but I guess that gives localisers flexibility in what appears for the two different notifications...

r- for the moment
Comment 11 neil@parkwaycc.co.uk 2011-02-11 06:52:06 PST
Created attachment 511715 [details] [diff] [review]
One way of avoiding code duplication

Since openAsExternal almost does what we want anyway, might as well use it.
Comment 12 neil@parkwaycc.co.uk 2011-02-11 06:55:58 PST
Created attachment 511718 [details] [diff] [review]
One way of avoiding code duplication

Sorry, previous patch was corrupt.
Comment 13 neil@parkwaycc.co.uk 2011-02-11 07:13:24 PST
Created attachment 511723 [details] [diff] [review]
Move code around to avoid duplication

Then the new code need only call self.openURLPref("plugins.update.url");
Comment 14 neil@parkwaycc.co.uk 2011-02-12 04:15:38 PST
Comment on attachment 511723 [details] [diff] [review]
Move code around to avoid duplication

>+        <parameter name="aPref">
Sigh. And this one has a typo. Should be /> of course.
Comment 15 Ian Neal 2011-02-12 15:47:55 PST
Comment on attachment 511723 [details] [diff] [review]
Move code around to avoid duplication

I like this one better, just seems simpler.
Comment 16 Ian Neal 2011-02-12 15:49:56 PST
Comment on attachment 511718 [details] [diff] [review]
One way of avoiding code duplication

Still worth keeping the utilityOverlay.js changes?
Comment 17 neil@parkwaycc.co.uk 2011-02-12 15:59:15 PST
(In reply to comment #16)
> Still worth keeping the utilityOverlay.js changes?
Not sure what you mean there; you need them with this attachment, and you don't need them with the other attachment, and if you did want them anyway then that would be beyond the scope of this bug ;-)
Comment 18 Ian Neal 2011-02-12 16:41:29 PST
(In reply to comment #17)
> (In reply to comment #16)
> > Still worth keeping the utilityOverlay.js changes?
> Not sure what you mean there; you need them with this attachment, and you don't
> need them with the other attachment, and if you did want them anyway then that
> would be beyond the scope of this bug ;-)
Yeah, I meant in another bug :-P
Comment 19 Adrian Kalla [:adriank] 2011-03-08 06:06:14 PST
1) Should we move the "avoid code duplication" to another bug?

2) I'll try to update the patch asap to meet the review comments. The patch will require the code duplication patch applied.
Comment 20 Adrian Kalla [:adriank] 2011-03-08 06:10:11 PST
3) I'd like to land the L10n files-part of the patch asap, so that it won't miss the L10n cut-ff(==2.1b3).
Comment 21 neil@parkwaycc.co.uk 2011-03-20 09:41:21 PDT
Created attachment 520530 [details] [diff] [review]
Tweaked patch
Comment 22 neil@parkwaycc.co.uk 2011-03-20 09:43:04 PDT
Created attachment 520531 [details] [diff] [review]
With added pluginUnavailable sharing

This isn't a separate patch because I'm not an mq user, so you'll have to use Bugzilla to interdiff for you. Sorry about that.
Comment 23 Ian Neal 2011-03-21 14:52:13 PDT
Comment on attachment 520531 [details] [diff] [review]
With added pluginUnavailable sharing

>+++ b/suite/common/bindings/notification.xml	Sun Mar 20 16:42:08 2011 +0000
>+      <method name="pluginUnavailable">
>+        <parameter name="aEvent"/>
>+        <parameter name="aNotification"/>
>+        <parameter name="aMessage"/>
>+        <parameter name="aButtons"/>
>+        <parameter name="aPref"/>
>+        <body>
>+          <![CDATA
>+            // Since we are expecting also untrusted events, make sure
>+            // that the target is a plugin
>+            if (!(event.target instanceof Components.interfaces.nsIObjectLoadingContent))
aEvent even?
>+              return;
>+
>+            if (this._prefs.getBoolPref(aPref || "plugins.hide_infobar_for_missing_plugin"))
>+              return;
>+
>+            var pluginInfo = this.getPluginInfo(event.target);
And again.
>+            this.missingPlugins[pluginInfo.mimetype] = pluginInfo;
>+            
>+            var notification;
>+            var notifications = [
>+              "blocked-plugins",
>+              "missing-plugins",
>+              "carbon-failure-plugins",
>+              "outdated-plugins",
>+            ];
>+            
>+            for (var i = 0; notifications[i] != aNotification; i++) {
>+              notification = this.getNotificationWithValue(notifications[i]);
>+              if (notification)
>+                notification.close();
>+            }
>+
>+            while (i < notifications.length) {
>+              notification = this.getNotificationWithValue(notifications[i++]);
>+              if (notification)
>+                return;
>+            }
Is it worth commenting what these two bits of code do?

r=me with those fixed/addressed.

I presume there is one more patch in the pipeline to do the PluginOutdated bits?
Comment 24 neil@parkwaycc.co.uk 2011-03-22 03:03:35 PDT
Comment on attachment 520530 [details] [diff] [review]
Tweaked patch

Pushed changeset 7662bda83892 to comm-central.
Comment 25 neil@parkwaycc.co.uk 2011-03-22 06:33:43 PDT
Created attachment 520902 [details] [diff] [review]
Fixed patch

Also not containing the changes from attachment 520530 [details] [diff] [review] of course.
Comment 26 neil@parkwaycc.co.uk 2011-03-22 15:21:16 PDT
Comment on attachment 520902 [details] [diff] [review]
Fixed patch

Pushed changeset 1c2bb452e5ec to comm-central.
Comment 27 neil@parkwaycc.co.uk 2011-03-22 15:34:19 PDT
Comment on attachment 520902 [details] [diff] [review]
Fixed patch

>+          <![CDATA
Oops. Fixed in changeset bba933a64ecd
Comment 28 neil@parkwaycc.co.uk 2011-03-22 17:49:23 PDT
Created attachment 521076 [details] [diff] [review]
Actually handle the event

There's a part of attachment 510749 [details] [diff] [review] that this doesn't cover.
If I work out what it's for I'll attach a follow up patch.
Comment 29 Ian Neal 2011-03-22 18:03:55 PDT
Comment on attachment 521076 [details] [diff] [review]
Actually handle the event

>+++ b/suite/common/bindings/notification.xml
>+      <handler event="PluginOutdated">
>+        <![CDATA[
>+          var messageString = this._stringBundle.GetStringFromName("outdatedpluginsMessage.title");
>+          var buttons = [{
>+            label: this._stringBundle.GetStringFromName("outdatedpluginsMessage.button.label"),
>+            accessKey: this._stringBundle.GetStringFromName("outdatedpluginsMessage.button.accesskey"),
>+            popup: null,
>+            callback: this.openURLPref.bind(this, "plugins.update.url")
>+          }];
>+
>+          this.pluginUnavailable(event, "outdated-plugins", messageString, buttons, "plugins.hide_infobar_for_outdated_plugin");
Nit: line is a bit long, can be split over two lines?
Comment 30 neil@parkwaycc.co.uk 2011-03-23 06:01:21 PDT
Comment on attachment 521076 [details] [diff] [review]
Actually handle the event

Pushed changeset 176ec6110ed9 to comm-central.
Comment 31 neil@parkwaycc.co.uk 2011-03-23 06:06:36 PDT
Created attachment 521160 [details] [diff] [review]
Pref observer
Comment 32 neil@parkwaycc.co.uk 2011-03-23 10:14:16 PDT
Created attachment 521226 [details] [diff] [review]
Startup notification check
Comment 33 Ian Neal 2011-03-23 14:08:58 PDT
Comment on attachment 521160 [details] [diff] [review]
Pref observer

>+++ b/suite/common/bindings/notification.xml

>                 if (aData == "plugins.hide_infobar_for_missing_plugin") {
>                   if (!this._prefs.getBoolPref(aData))
>                     return;
> 
>                   var pluginNotification = this.getNotificationWithValue("missing-plugins");
>                   if (pluginNotification)
>                     this.removeNotification(pluginNotification);
>+
>+                  var pluginNotification = this.getNotificationWithValue("blocked-plugins");
Nit: redeclaring pluginNotification!
>+                  if (pluginNotification)
>+                    this.removeNotification(pluginNotification);
>                 }
r=me with that fixed.
Comment 34 neil@parkwaycc.co.uk 2011-03-23 15:17:48 PDT
Comment on attachment 521160 [details] [diff] [review]
Pref observer

Pushed changeset 79d782f7798f to comm-central.
Comment 35 neil@parkwaycc.co.uk 2011-03-23 15:18:24 PDT
Comment on attachment 521226 [details] [diff] [review]
Startup notification check

Pushed changeset 106f131f2573 to comm-central.

I renamed the variables in question.
Comment 36 Vlad [QA] 2011-08-04 06:45:59 PDT
Reproducible on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.20) Gecko/20110803 Firefox/3.6.20

No notification message pops down saying "Some plugins used by this page are out of date." 

I had installed Quik Time plugin 7.5
Comment 37 Justin Wood (:Callek) 2011-08-04 14:41:17 PDT
Vlad, this bug is for SeaMonkey Suite, and is fixed. your UA seems to indicate a Firefox version. And it does look like you found/commented in Bug 514327

Note You need to log in before you can comment on or make changes to this bug.