Currently these tests time out: 3852 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_crash_submit.xul | Test timed out. 3857 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_hang_submit.xul | Test timed out. This is probably due to this code trying from notification.xml trying to modify some non-existing link: 2111 let pleaseLink = doc.getAnonymousElementByAttribute( 2112 plugin, "class", "pleaseSubmitLink"); 2113 this.addLinkClickCallback(pleaseLink, this.submitReport, 2114 pluginDumpID, browserDumpID); There is no element with the pleaseSubmitLink CSS class.

Basically need to port fixes from Bug 648675.

This patch ports the changes from Firefox. The patch basically adjust the SeaMonkey UI to the toolkit/ changes. It added a comment box and a checkbox enabling the user to send the page URL to Mozilla for sending plugin crash info via crashreporter. This patch adjusts the SeaMonkey JS and CSS code to make sure everything gets displayed fine: It checks if the plugin area is big enough to display the plugin crash UI, if not, it will display a notification bar. The patch also adjusts the Modern theme CSS rules to the Firefox version of pluginProblem.css. And the patch copies the Firefox test to make sure the new UI works as expected (needed to adjust a few lines to make the test work in SeaMonkey). Original FF changeset: http://hg.mozilla.org/mozilla-central/rev/528411b6f628

I did get one error running the tests: TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_pluginCrashCom mentAndURL.js | leaked until shutdown [nsGlobalWindow #20 http://example.com/browser/suite/browser/test/pluginCrashCommentAndURL.html?%7B%7D]

Depends on Bug 798278 in some way as Bug 798278 fixes a JavaScript error that occurs before displaying the plugin crash UI (and currently stops JS execution)

Comment on attachment 747978 [details] [diff] [review] Patch Canceling review for now.

> + <method name="getPluginUI"> File a followup bug to replace (where sensible) calls to Plugin.ownerDocument.getAnonymousElementByAttribute() with this method. > - this.CrashSubmit.submit(pluginDumpID); > + let keyVals = {}; var keyVals = {} > - this.addLinkClickCallback(pleaseLink, this.submitReport, > - pluginDumpID, browserDumpID); > + this.getPluginUI(plugin, "submitButton").addEventListener("click", Hmm I still prefer addLinkClickCallback() assuming that it works. > + pref.setBoolPref("", optInCB.checked); > + }.bind(this)); > + let optInCB = this.getPluginUI(plugin, "submitURLOptIn"); Further up there are two calls to getAnonymousElementByAttribute() which can be replaced by this.getPluginUI(). > + let optInCB = this.getPluginUI(plugin, "submitURLOptIn"); > + let pref = Services.prefs.getBranch("dom.ipc.plugins.reportCrashURL"); > + optInCB.checked = pref.getBoolPref(""); Incorrect indentation in the previous three lines. Please don't introduce a dependency on Services.jsm use this._prefs instead. Also using getBranch just makes the code less readable. Do something like this (untested code): var submitButton = this.getPluginUI(plugin, "submitButton"); var optInCB = this.getPluginUI(plugin, "submitURLOptIn"); optInCB.checked = this._prefs.getBoolPref("dom.ipc.plugins.reportCrashURL"); var submitReport = function(pluginDumpID, browserDumpID, plugin, optInCB) { this.submitReport(pluginDumpID, browserDumpID, plugin); this._prefs.setBoolPref("dom.ipc.plugins.reportCrashURL", optInCB.checked); }; this.addLinkClickCallback(submitButton, submitReport, pluginDumpID, browserDumpID, plugin, optInCB); > + // First try hiding the comment box and related report submission UI. > + var submitDiv = > + doc.getAnonymousElementByAttribute(plugin, "class", > + "msg msgPleaseSubmit"); > + submitDiv.style.display = "none"; Remove this hunk as the final Firefox patch doesn't include this. > + let tab = gBrowser.loadOneTab("about:blank", { inBackground: false }); > + let browser = gBrowser.getBrowserForTab(tab); let browser = tab.linkedBrowser; > + let env = Cc["@mozilla.org/process/environment;1"]. > + getService(Components.interfaces.nsIEnvironment); Either use short cuts (Cc, Ci, Cr) consistently or spell these out in full (Neil prefers no shortcuts). > + let plugin = gBrowser.contentDocument.getElementById("plugin"); > + let elt = plugin.ownerDocument.getAnonymousElementByAttribute.bind(plugin.ownerDocument, plugin, "class"); Please use getBrowser() instead of gBrowser. Isn't gBrowser.contentDocument == plugin.ownerDocument ? I think the following should work: let elt = getBrowser().getNotificationBox().getPluginUI; ... elt(plugin, "submitComment").value = currentRun.comment;

Ratty: I've fixed almost all review comment except the ones where I think there's a better solution :) - Regarding addLinkClickCallback: My code does more than just adding a callback for sending a crash report, it also sets the dom.ipc.plugins.reportCrashURL pref depending on a checkbox in the UI. I'm against modifying the submitReport method so that it will also change the pref. submitReport should just collect the required information and send the crash report. I could also add a new function but that would be even more overhead. - In the test I used Components.results in one line as Cr does not work yet in SeaMonkey tests, I'll file a follow-up bug on that - I'm against using "let elt = getBrowser().getNotificationBox().getPluginUI;" in the test, that looks like a bad hack to me. If I use getNotificationBox(), I expect the code to do something regarding notifications. If I just use it to call some random function inside notification.xml...I certainly don't like that. Let's keep the code clean and just use two function calls to get to the plugin object. - All other review comments have been fixed

Comment on attachment 760189 [details] [diff] [review] Patch [Triage Comment] > +++ b/suite/common/bindings/notification.xml > - var crashText = doc.getAnonymousElementByAttribute(plugin, "class", "msg msgCrashed"); > + var crashText = doc.getAnonymousElementByAttribute(plugin, "class", "msgCrashedText"); Any reason you aren't using getPluginUI() here? > +++ b/suite/browser/test/browser/browser_pluginCrashCommentAndURL.js > + getService(Ci.nsIEnvironment); Firefox uses: getService(Components.interfaces.nsIEnvironment); I think we should be consistent. Either all in full, or all using Cu/Cc/Ci/Cr. > +++ b/suite/themes/modern/mozapps/plugins/pluginProblem.css Please file a followup bug to port: Bug 831921 - Make the plugin UI less broken-looking for all plugins except for crashed plugins, including visual tweaks by shorlander. Bug 842692 - Plugin click-to-play play button has default cursor. Sorry for the extreme delay. r=me a=me for comm-aurora

Fixed the getPluginUI thing. I want to use Ci/Cc/... everywhere, but we can't use Cr yet, need to file a bug on that/fix that (that is why I used Components.results in the test file).

Comment on attachment 769642 [details] [diff] [review] Patch for checkin Pushed to comm-central: https://hg.mozilla.org/comm-central/rev/223efdc91d32

Wrong dependency.

Patch pushed to comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/73fe0c8b8409

Fixed the line endings of the test file (from DOS to Unix): https://hg.mozilla.org/comm-central/rev/06afc3f31ba6

Fixed the line endings of the test file (from DOS to Unix): https://hg.mozilla.org/releases/comm-aurora/rev/bd8eabc368e4

All fixed here.