Show notification bar offering to restart when content tries to load a 32-bit plugin using carbon-based NPAPI

RESOLVED FIXED in seamonkey2.1b3

Status

SeaMonkey
General
RESOLVED FIXED
6 years ago
6 years ago

People

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

Tracking

Trunk
seamonkey2.1b3
x86
Mac OS X
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-seamonkey2.1 b3+)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
Bug 628651 did this for Firefox, I think we should do the same. I think I'll be able to do this, but I will not be able to finish this off until after b3.
(Reporter)

Comment 1

6 years ago
Created attachment 520951 [details] [diff] [review]
Strings

These are the string changes that will need to go in before b3.
Assignee: nobody → stefanh
Status: NEW → ASSIGNED
Attachment #520951 - Flags: review?(kairo)
Attachment #520951 - Flags: approval-seamonkey2.1b3?

Comment 2

6 years ago
Comment on attachment 520951 [details] [diff] [review]
Strings

No approval needed, but this looks good if you really don't manage to get this done in time.
Attachment #520951 - Flags: review?(kairo)
Attachment #520951 - Flags: review+
Attachment #520951 - Flags: approval-seamonkey2.1b3?

Comment 3

6 years ago
Note that this is connected to bug 601493 and bug 521159 as well, I wonder if you guys could work together on getting all of those done in more or less one go?
(Assignee)

Comment 4

6 years ago
To avoid conflicting with attachment 520491 [details] [diff] [review] we should maybe put these strings earlier in the file; I'm not sure whether carbon failure counts as higher priority than an outdated or missing plugin though.
(Reporter)

Comment 5

6 years ago
Well, I'm the last here ;-)

Note to self:
-------------------
Need to take into account that the user has a pure 64-bit build, iotw can't restart in 32-bit mode. So, I'll need to check if we're on a universal build or not - if we're not, I'll show the missing plugin notification.
-------------------
Since this is about informing users that they need to restart in 32-bit mode and not really about a missing plugin, I should probably have a separate pref (as Firefox) rather than the "plugins.hide_infobar_for_missing_plugin" here.
-------------------

Hmm, Neil - looking at bug 521159, should I just call the pluginUnavailable method in attachment #520902 [details] [diff] [review] with different parameter values depending on whether we're universal or not?
(Assignee)

Comment 6

6 years ago
(In reply to comment #5)
> Hmm, Neil - looking at bug 521159, should I just call the pluginUnavailable
> method in attachment #520902 [details] [diff] [review] with different parameter values depending on
> whether we're universal or not?
Well, that's the idea.

I notice that Firefox only adds the click handler for real PluginNotFound events; for npapi-carbon-event-model-failure in the non-universal case this translates into a PluginNotFound but should we then add the click handler?
(Reporter)

Comment 7

6 years ago
(In reply to comment #2)
> No approval needed, but this looks good if you really don't manage to get this
> done in time.

Sorry about that approval request. Not sure what I had in mind - I obviously know that we haven't released 2.1b3 (had a long day, I guess).
(Assignee)

Comment 8

6 years ago
Created attachment 521233 [details] [diff] [review]
Not quite right

I just realised that I forgot the pref observer (if someone turns the pref on then the notifications should go away).
(Reporter)

Comment 9

6 years ago
Neil, judging by attachment #521233 [details] [diff] [review] you should take this.
(Reporter)

Updated

6 years ago
Assignee: stefanh → neil
(Assignee)

Comment 10

6 years ago
Created attachment 521502 [details] [diff] [review]
Proposed patch
Attachment #520951 - Attachment is obsolete: true
Attachment #521233 - Attachment is obsolete: true
Attachment #521502 - Flags: review?(stefanh)
(Reporter)

Comment 11

6 years ago
Comment on attachment 521502 [details] [diff] [review]
Proposed patch

I spotted some errors due to the fact that you don't have any way of testing this:

+                  var carbonfailureNotification = this.getNotificationWithValue("carbon_failure-plugins");

Whoops, should be "carbon-failure-plugins"

+            var nsIAppStartup = Components.interfaces.nsIAppStartup;
+            Components.classes["@mozilla.org/toolkit/app-startup;1"]
+                      .getService(Components.interfaces.nsIAppStartup

As you mentioned, should be nsIAppStartup here since you do "var nsIAppStartup = Components.interfaces.nsIAppStartup;" Also a ")" is missing.

+          if ("@mozilla.org/xpcom/mac-utils;1" in Components.classes &&
+              !Components.classes["@mozilla.org/xpcom/mac-utils;1"]
+                         .getService(Components.interfaces.nsIMacUtils)
+                         .isUniversalBinary) {

And there should be another parenthesis after ".isUniversalBinary)"

+carbonfailurepluginsMessage.restartButton.label=Restart in 32-bit mode
+carbonfailurepluginsMessage.restartButton.accesskey=R

And you want ".button" not ."restartButton" ;-)

Also, as we talked about, we need a notification that an application quit has been requested.
(Reporter)

Comment 12

6 years ago
Created attachment 521684 [details] [diff] [review]
Patch, for interdiff
(Reporter)

Comment 13

6 years ago
(In reply to comment #11)

> +          if ("@mozilla.org/xpcom/mac-utils;1" in Components.classes &&
> +              !Components.classes["@mozilla.org/xpcom/mac-utils;1"]
> +                         .getService(Components.interfaces.nsIMacUtils)
> +                         .isUniversalBinary) {
> 
> And there should be another parenthesis after ".isUniversalBinary)"

The interdiff between attachment #521684 [details] [diff] [review] (my edited version) and attachment #521502 [details] [diff] [review] shows that the above comment is wrong.
(Assignee)

Comment 14

6 years ago
Created attachment 521688 [details] [diff] [review]
Addressed review comments
Attachment #521502 - Attachment is obsolete: true
Attachment #521684 - Attachment is obsolete: true
Attachment #521502 - Flags: review?(stefanh)
Attachment #521688 - Flags: review?(stefanh)
(Reporter)

Comment 15

6 years ago
Comment on attachment 521688 [details] [diff] [review]
Addressed review comments

Works fine now, thanks :-)

JFTR: there is one edge case that this won't work with:
1) Load a carbon plugin in a universal build, note the restart notification (correct)
2) Quit the universal build
2) Get a pure 64-bit build and launch it with the same profile, then load the carbon plugin --> you'll see the missing plugin notification (correct)
3) Launch the universal build again with the same profile and load a carbin plugin --> you'll see the missing plugin notification (not correct)

The cause here seems to be pluginreg.dat in the profile dir, deleting the file fixes the issue. In reality, I doubt that this will happen ;-)
Attachment #521688 - Flags: review?(stefanh) → review+
(Assignee)

Comment 16

6 years ago
Pushed changeset 9c76e9244b20 to comm-central.

I guess the pluginreg.dat issue is a core bug, care to file it? ;-)
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Updated

6 years ago
Target Milestone: --- → seamonkey2.1b3

Comment 17

6 years ago
Comment on attachment 521688 [details] [diff] [review]
Addressed review comments

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

>+          var notification = "carbon-failure-plugins";
>+          var pref = "plugins.hide_infobar_for_carbon_failure_plugin";
>+          if ("@mozilla.org/xpcom/mac-utils;1" in Components.classes &&
>+              !Components.classes["@mozilla.org/xpcom/mac-utils;1"]
>+                         .getService(Components.interfaces.nsIMacUtils)
>+                         .isUniversalBinary) {
>+            pref = null;
>+            notification = "missing-plugins";
>+            callback = this.openURLPref.bind(this, "plugins.update.url");
>+          }
>+
>+          var prefix = notification.replace(/-/g, "");
>+          var message = this._stringBundle.GetStringFromName(prefix + "Message.title");
>+          var buttons = [{
>+            label: this._stringBundle.GetStringFromName(prefix + "Message.button.label"),
>+            accessKey: this._stringBundle.GetStringFromName(prefix + "Message.button.accesskey"),
>+            popup: null,
>+            callback: callback
>+          }];
>+
>+          this.pluginUnavailable(event, "carbon-failure-plugins", message, buttons, pref);
Should this be notification rather than "carbon-failure-plugins"?

Updated

6 years ago
Flags: in-testsuite-
(Assignee)

Comment 18

6 years ago
(In reply to comment #17)
>(From update of attachment 521688 [details] [diff] [review])
>>+          this.pluginUnavailable(event, "carbon-failure-plugins", message, buttons, pref);
>Should this be notification rather than "carbon-failure-plugins"?
Pushed changeset 746ecc2acac1 to comm-central.

Updated

6 years ago
blocking-seamonkey2.1: ? → b3+
You need to log in before you can comment on or make changes to this bug.