Last Comment Bug 643799 - Show notification bar offering to restart when content tries to load a 32-bit plugin using carbon-based NPAPI
: Show notification bar offering to restart when content tries to load a 32-bit...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: seamonkey2.1b3
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-22 10:17 PDT by Stefan [:stefanh]
Modified: 2011-03-31 09:52 PDT (History)
7 users (show)
iann_bugzilla: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
b3+


Attachments
Strings (1.15 KB, patch)
2011-03-22 10:21 PDT, Stefan [:stefanh]
kairo: review+
Details | Diff | Splinter Review
Not quite right (4.55 KB, patch)
2011-03-23 10:40 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Proposed patch (7.45 KB, patch)
2011-03-24 08:09 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Patch, for interdiff (7.41 KB, patch)
2011-03-24 16:41 PDT, Stefan [:stefanh]
no flags Details | Diff | Splinter Review
Addressed review comments (8.02 KB, patch)
2011-03-24 16:51 PDT, neil@parkwaycc.co.uk
stefanh: review+
Details | Diff | Splinter Review

Description Stefan [:stefanh] 2011-03-22 10:17:10 PDT
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] 2011-03-22 10:21:22 PDT
Created attachment 520951 [details] [diff] [review]
Strings

These are the string changes that will need to go in before b3.
Comment 2 Robert Kaiser 2011-03-22 12:33:50 PDT
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 2011-03-22 12:34:54 PDT
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 neil@parkwaycc.co.uk 2011-03-22 13:01:48 PDT
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] 2011-03-22 15:28:34 PDT
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 neil@parkwaycc.co.uk 2011-03-22 17:12:50 PDT
(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] 2011-03-23 08:14:04 PDT
(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 neil@parkwaycc.co.uk 2011-03-23 10:40:16 PDT
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).
Comment 9 Stefan [:stefanh] 2011-03-23 11:35:40 PDT
Neil, judging by attachment #521233 [details] [diff] [review] you should take this.
Comment 10 neil@parkwaycc.co.uk 2011-03-24 08:09:29 PDT
Created attachment 521502 [details] [diff] [review]
Proposed patch
Comment 11 Stefan [:stefanh] 2011-03-24 16:34:06 PDT
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] 2011-03-24 16:41:33 PDT
Created attachment 521684 [details] [diff] [review]
Patch, for interdiff
Comment 13 Stefan [:stefanh] 2011-03-24 16:44:16 PDT
(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 neil@parkwaycc.co.uk 2011-03-24 16:51:20 PDT
Created attachment 521688 [details] [diff] [review]
Addressed review comments
Comment 15 Stefan [:stefanh] 2011-03-25 16:02:34 PDT
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 neil@parkwaycc.co.uk 2011-03-25 17:37:35 PDT
Pushed changeset 9c76e9244b20 to comm-central.

I guess the pluginreg.dat issue is a core bug, care to file it? ;-)
Comment 17 Ian Neal 2011-03-25 19:11:21 PDT
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 neil@parkwaycc.co.uk 2011-03-26 09:13:03 PDT
(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.

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