The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

SeaMonkey
Startup & Profiles
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: sgautherie, Assigned: neil@parkwaycc.co.uk)

Tracking

Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.1 wanted)

Details

Attachments

(6 attachments, 4 obsolete attachments)

Comment hidden (empty)

Updated

7 years ago
Duplicate of this bug: 536114
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
(Reporter)

Updated

7 years ago
Duplicate of this bug: 573391
(Reporter)

Updated

7 years ago
status-seamonkey2.1: --- → ?

Comment 4

7 years ago
As we're in happy-dupe-to-empty-description-bugs time, please read bug 573391 comment #0 as well, which at least describes something.

Updated

7 years ago
status-seamonkey2.1: ? → wanted
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
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...
Assignee: nobody → akalla
Status: NEW → ASSIGNED
Attachment #510749 - Flags: review?
Attachment #510749 - Flags: review? → review?(iann_bugzilla)

Comment 7

6 years ago
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

6 years ago
(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).
(Reporter)

Updated

6 years ago
Blocks: 595810
(Assignee)

Comment 9

6 years ago
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

6 years ago
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-
(Assignee)

Comment 11

6 years ago
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.
Attachment #511715 - Flags: feedback?(iann_bugzilla)
(Assignee)

Comment 12

6 years ago
Created attachment 511718 [details] [diff] [review]
One way of avoiding code duplication

Sorry, previous patch was corrupt.
Attachment #511715 - Attachment is obsolete: true
Attachment #511718 - Flags: feedback?(iann_bugzilla)
Attachment #511715 - Flags: feedback?(iann_bugzilla)
(Assignee)

Comment 13

6 years ago
Created attachment 511723 [details] [diff] [review]
Move code around to avoid duplication

Then the new code need only call self.openURLPref("plugins.update.url");
Attachment #511723 - Flags: feedback?(iann_bugzilla)
(Assignee)

Comment 14

6 years ago
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

6 years ago
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 16

6 years ago
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+
(Assignee)

Comment 17

6 years ago
(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

6 years ago
(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).
(Assignee)

Comment 21

6 years ago
Created attachment 520530 [details] [diff] [review]
Tweaked patch
Attachment #511718 - Attachment is obsolete: true
Attachment #511723 - Attachment is obsolete: true
Attachment #520530 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 22

6 years ago
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.
Attachment #520531 - Flags: review?(iann_bugzilla)

Updated

6 years ago
Attachment #520530 - Flags: review?(iann_bugzilla) → review+

Comment 23

6 years ago
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+
(Assignee)

Comment 24

6 years ago
Comment on attachment 520530 [details] [diff] [review]
Tweaked patch

Pushed changeset 7662bda83892 to comm-central.
(Assignee)

Comment 25

6 years ago
Created attachment 520902 [details] [diff] [review]
Fixed patch

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)

Updated

6 years ago
Attachment #520902 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 26

6 years ago
Comment on attachment 520902 [details] [diff] [review]
Fixed patch

Pushed changeset 1c2bb452e5ec to comm-central.
(Assignee)

Comment 27

6 years ago
Comment on attachment 520902 [details] [diff] [review]
Fixed patch

>+          <![CDATA
Oops. Fixed in changeset bba933a64ecd
(Assignee)

Comment 28

6 years ago
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.
Assignee: akalla → neil
Attachment #521076 - Flags: review?(iann_bugzilla)

Comment 29

6 years ago
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+
(Assignee)

Comment 30

6 years ago
Comment on attachment 521076 [details] [diff] [review]
Actually handle the event

Pushed changeset 176ec6110ed9 to comm-central.
(Assignee)

Comment 31

6 years ago
Created attachment 521160 [details] [diff] [review]
Pref observer
Attachment #521160 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 32

6 years ago
Created attachment 521226 [details] [diff] [review]
Startup notification check
Attachment #521226 - Flags: review?(iann_bugzilla)

Comment 33

6 years ago
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+

Updated

6 years ago
Attachment #521226 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 34

6 years ago
Comment on attachment 521160 [details] [diff] [review]
Pref observer

Pushed changeset 79d782f7798f to comm-central.
(Assignee)

Comment 35

6 years ago
Comment on attachment 521226 [details] [diff] [review]
Startup notification check

Pushed changeset 106f131f2573 to comm-central.

I renamed the variables in question.
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 36

6 years ago
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.