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

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Stefan [:stefanh]]

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.

Comment 1

[Stefan [:stefanh]]

Attachment: Strings

These are the string changes that will need to go in before b3.

Comment 2

[Robert Kaiser]

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.

Comment 3

[Robert Kaiser]

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?

Comment 4

[[e-mail]]

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.

Comment 5

[Stefan [:stefanh]]

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?

Comment 6

[[e-mail]]

(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?

Comment 7

[Stefan [:stefanh]]

(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).

Comment 8

[[e-mail]]

Attachment: Not quite right

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

Comment 9

[Stefan [:stefanh]]

Neil, judging by attachment #521233 [details] [diff] [review] you should take this.

Comment 10

[[e-mail]]

Attachment: Proposed patch

Comment 11

[Stefan [:stefanh]]

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.

Comment 12

[Stefan [:stefanh]]

Attachment: Patch, for interdiff

Comment 13

[Stefan [:stefanh]]

(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.

Comment 14

[[e-mail]]

Attachment: Addressed review comments

Comment 15

[Stefan [:stefanh]]

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 ;-)

Comment 16

[[e-mail]]

Pushed changeset 9c76e9244b20 to comm-central.

I guess the pluginreg.dat issue is a core bug, care to file it? ;-)

Comment 17

[Ian Neal]

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"?

Comment 18

[[e-mail]]

(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.