Closed Bug 661410 Opened 10 years ago Closed 10 years ago

Implement UI for out-of-process plugins

Categories

(Thunderbird :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 9.0

People

(Reporter: standard8, Assigned: mconley)

References

Details

Attachments

(1 file, 8 obsolete files)

Bug 617906 implemented out-of-process plugins for Thunderbird, but we haven't got around to implementing the text of the UI for it so that if plugins crash, or if there missing plugins, the user gets notified.

We should do that, we should be able to copy most of it from Firefox, probably looking into browser.js.
Attached patch WIP (obsolete) — Splinter Review
In progress, not-working patch.

What I was doing here was copying the Firefox UI code for the plugin handling and migrating functions from their Firefox equivalents to Thunderbird ones.

There's a possibility this could be made into shared code, but I think for now it is easier and quicker to have our own copy.
Assignee: nobody → mconley
I've marked bug 662960 as blocking because the tests I've written use waitFor, as opposed to waitForEval.
Attached patch WIP Patch v1 (obsolete) — Splinter Review
First run at a patch.  Adds a bunch of Mozmill tests.

I haven't connected with Roland yet about the plugin-crash help page, so that's still left to do.
Attachment #551834 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — Splinter Review
More protection to ensure that the platform allows oop plugins, and expected minidumps are cleaned up.
Attachment #555198 - Attachment is obsolete: true
Attached patch patch v3 (obsolete) — Splinter Review
Includes fixes for API rot introduced by bug 686129 and the minidump issue mconley was seeing. (The issue was that the correct call to remove a file is file.remove(false) but what was instead being called was file.remove(). This being in an observer, the exception was eaten silently. I've also included a fix for that.)

I've kicked off try builds: http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=0466293b5589
Attachment #555537 - Attachment is obsolete: true
The try builds failed because of buildbot issues :(
Sid:

You, sir, are awesome.  Thanks so much for your assistance on this - it was driving me nuts!

Now I just need to determine why the Try build failed out on OSX 10.5.2 (http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1316302549.1316303253.12791.gz).  Or am I correct in remembering that OPP are not enabled/possible for 10.5.2?

-Mike
Yes. OOPP isn't enabled on 10.5.2.
Note that you might want to change the plugins_run_in_separate_processes function to match something more like this test function:

http://hg.mozilla.org/mozilla-central/annotate/5319b0100025/layout/tools/reftest/reftest.js#l490
Attached patch Patch v4 (obsolete) — Splinter Review
Added a more thorough check to ensure that the OS supports OOP plugins (thanks Standard8).

Try builds coming in here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=bef328eb7431
Attachment #560731 - Attachment is obsolete: true
Attached patch Patch v5 (obsolete) — Splinter Review
Argh, last patch wasn't too smart.  This one is better.

Try builds are here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=4def3033b689
Attachment #560921 - Attachment is obsolete: true
Attached patch Patch v6 (obsolete) — Splinter Review
Yay - it looks like my tests finally work! (http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=40a1c58b4722)

Ready for review.
Attachment #560992 - Attachment is obsolete: true
Attachment #561752 - Flags: review?(mbanner)
Comment on attachment 561752 [details] [diff] [review]
Patch v6

Review of attachment 561752 [details] [diff] [review]:
-----------------------------------------------------------------

This is mostly looking good, just need to finish reviewing the test* files.

::: mail/app/profile/all-thunderbird.js
@@ +711,5 @@
> +pref("plugins.use_layers", false);
> +pref("plugins.hide_infobar_for_carbon_failure_plugin", false);
> +#endif
> +
> +pref("plugins.update.url", "https://www.mozilla.com/%LOCALE%/plugincheck/");

Probably not an ideal url, but it'll do for now.

::: mail/base/content/plugins.js
@@ +40,5 @@
> +                                   "@mozilla.org/xre/app-info;1",
> +                                   "nsICrashReporter");
> +#endif
> +
> +function getPluginInfo(pluginElement)

I wonder if we should mention in this file that this is largely copied from browser.js.

@@ +84,5 @@
> + *
> + * Currently supported built-ins are LOCALE, APP, and any value from nsIXULAppInfo, uppercased.
> + */
> +function formatURL(aFormat, aIsPref) {
> +  var formatter = Components.classes["@mozilla.org/toolkit/URLFormatterService;1"]

nit: Can just use Services.urlFormatter (several places)

@@ +110,5 @@
> +  },
> +
> +  get CrashSubmit() {
> +    delete this.CrashSubmit;
> +    Cu.import("resource://gre/modules/CrashSubmit.jsm", this);

When I try this here, I get that Cu is not defined.
Comment on attachment 561752 [details] [diff] [review]
Patch v6

Review of attachment 561752 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the previously mentioned comments fixed.
Attachment #561752 - Flags: review?(mbanner) → review+
Sorry, no level 3 access yet - so checkin-needed.
Checked in: http://hg.mozilla.org/comm-central/rev/9fa12b677b96
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 9.0
You need to log in before you can comment on or make changes to this bug.