Closed Bug 521159 Opened 15 years ago Closed 13 years ago

[SeaMonkey 2.1] Port |Bug 514327 - Detect outdated plugins and offer upgrade path|

Categories

(SeaMonkey :: Startup & Profiles, defect)

defect
Not set
normal

Tracking

(seamonkey2.1 wanted)

RESOLVED FIXED
Tracking Status
seamonkey2.1 --- wanted

People

(Reporter: sgautherie, Assigned: neil)

References

Details

Attachments

(6 files, 4 obsolete files)

      No description provided.
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.
Assignee: nobody → aqualon
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
As we're in happy-dupe-to-empty-description-bugs time, please read bug 573391 comment #0 as well, which at least describes something.
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.
Status: ASSIGNED → NEW
Assignee: aqualon → nobody
Attached patch patch v1Splinter Review
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...
Assignee: nobody → akalla
Status: NEW → ASSIGNED
Attachment #510749 - Flags: review?
Attachment #510749 - Flags: review? → review?(iann_bugzilla)
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.
(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).
Blocks: 595810
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 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
Attachment #510749 - Flags: review?(iann_bugzilla) → review-
Since openAsExternal almost does what we want anyway, might as well use it.
Attachment #511715 - Flags: feedback?(iann_bugzilla)
Sorry, previous patch was corrupt.
Attachment #511715 - Attachment is obsolete: true
Attachment #511718 - Flags: feedback?(iann_bugzilla)
Attachment #511715 - Flags: feedback?(iann_bugzilla)
Then the new code need only call self.openURLPref("plugins.update.url");
Attachment #511723 - Flags: feedback?(iann_bugzilla)
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 on attachment 511723 [details] [diff] [review]
Move code around to avoid duplication

I like this one better, just seems simpler.
Attachment #511723 - Flags: feedback?(iann_bugzilla) → feedback+
Comment on attachment 511718 [details] [diff] [review]
One way of avoiding code duplication

Still worth keeping the utilityOverlay.js changes?
Attachment #511718 - Flags: feedback?(iann_bugzilla) → feedback+
(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 ;-)
(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
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.
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).
Attached patch Tweaked patchSplinter Review
Attachment #511718 - Attachment is obsolete: true
Attachment #511723 - Attachment is obsolete: true
Attachment #520530 - Flags: review?(iann_bugzilla)
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.
Attachment #520531 - Flags: review?(iann_bugzilla)
Attachment #520530 - Flags: review?(iann_bugzilla) → review+
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?
Attachment #520531 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 520530 [details] [diff] [review]
Tweaked patch

Pushed changeset 7662bda83892 to comm-central.
Attached patch Fixed patchSplinter Review
Also not containing the changes from attachment 520530 [details] [diff] [review] of course.
Attachment #520531 - Attachment is obsolete: true
Attachment #520902 - Flags: review?(iann_bugzilla)
Attachment #520902 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 520902 [details] [diff] [review]
Fixed patch

Pushed changeset 1c2bb452e5ec to comm-central.
Comment on attachment 520902 [details] [diff] [review]
Fixed patch

>+          <![CDATA
Oops. Fixed in changeset bba933a64ecd
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.
Assignee: akalla → neil
Attachment #521076 - Flags: review?(iann_bugzilla)
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?
Attachment #521076 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 521076 [details] [diff] [review]
Actually handle the event

Pushed changeset 176ec6110ed9 to comm-central.
Attached patch Pref observerSplinter Review
Attachment #521160 - Flags: review?(iann_bugzilla)
Attachment #521226 - Flags: review?(iann_bugzilla)
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.
Attachment #521160 - Flags: review?(iann_bugzilla) → review+
Attachment #521226 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 521160 [details] [diff] [review]
Pref observer

Pushed changeset 79d782f7798f to comm-central.
Comment on attachment 521226 [details] [diff] [review]
Startup notification check

Pushed changeset 106f131f2573 to comm-central.

I renamed the variables in question.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: