538910 - Plugins: Need a "plugin crashed" UI

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement)

Description

[Justin Dolske [:Dolske]]

+++ This bug was initially created as a clone of Bug #519541 +++

With out-of-process-plugins [OOPPs], plugins can now crash without taking down the entire browser. As such, we need to add browser and content UI to handle when this happens so the user knows what's going on.

From bug 519541 comment 5 -- limi made some preliminary suggestions for what this UI should be:

* The first time a plugin crashes, we should drop down a doorhanger.

* The doorhanger should have a checkbox, on by default, for submitting the
crash report, and a text, something like "Adobe Flash has crashed." and a
reload button.

* If the user reloads in any way (doorhanger, toolbar, or Ctrl-R), we should
submit the crash report.

* Any subsequent times the plugin crashes, we should check to see
** is the plugin large enough to display a notification directly within it
** and visible

* If the plugin notification can be displayed directly within the plugin frame,
we should do so. If not, we should drop down the doorhanger.


Note that bug 525849 covers UI for submitting crash reports for the crashed plugin.

Comment 1

[Justin Dolske [:Dolske]]

Gavin also noted in 525849:

...we attach XBL using CSS with special -moz-* pseudo-classes:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/plugins/content/missingPluginBinding.css#39

Probably not a strict requirement, but should be change things to that plugins have a native anonymous content frame (ala <video>), for these bindings to attach to, so that web page content can't muck with things? Could help with bug 537998 too...

Comment 2

[Benjamin Smedberg]

UI question: are notification bars associated with particular tabs? If flash crashes and I have three tabs with flash open, will each tab get their own notification bar?

We need to make sure that we only submit the cash report once, while still giving a UI affordance for the user to refresh each page to regain functionality.

Comment 3

[Dave Garrett]

(In reply to comment #2)
> UI question: are notification bars associated with particular tabs?

Yes. Each tab has its own independent <notificationbox>.

Comment 4

[u88484]

(In reply to comment #2)
> UI question: are notification bars associated with particular tabs? If flash
> crashes and I have three tabs with flash open, will each tab get their own
> notification bar?
> 
> We need to make sure that we only submit the cash report once, while still
> giving a UI affordance for the user to refresh each page to regain
> functionality.

Can we some how be notified that flash crashes on n number of tabs?  I don't want to have to dismiss 10 notification boxes.

Also, why does the user even have to refresh the page?  If I don't have flash and come across a page with it I only have to install flash to see the content.  I do not have to refresh the page.

Comment 5

[Benjamin Smedberg]

Kurt, what would the effect of dismissing that notification be? to refresh all N tabs/windows at once?

We don't want to refresh in-place because page script often makes assumptions about the plugin state which will no longer be correct.

Comment 6

[u88484]

(In reply to comment #5)
> Kurt, what would the effect of dismissing that notification be? to refresh all
> N tabs/windows at once?
That is what I would personally like but then again we'd run into the issue of users who may have opened a dozen tabs of youtube videos and paused all but one of them to watch one at a time now having all tabs videos start playing again but for sites that don't have the plugins autoplay, this makes sense.

I just think it would be annoying to dismiss the notification bar N times for N number of tabs that had the crashed plugin loaded.  The users would then have to manually refresh each and every single tab.  This is turn wastes the users time and bandwidth for both the user and site (due to the following comment) just because the page would need to be manually refreshed when we technically don't have to.  Just for conversation sake here, what does chrome do?

> We don't want to refresh in-place because page script often makes assumptions
> about the plugin state which will no longer be correct.
Make a little sense to me even though I don't understand exactly what you mean but I get the jist of it.  Bug 391725 should be WONTFIXed then even though I disagree.

Comment 7

[[not reading bugmail]]

(In reply to comment #6)
> (In reply to comment #5)
> > Kurt, what would the effect of dismissing that notification be? to refresh all
> > N tabs/windows at once?
> That is what I would personally like but then again we'd run into the issue of
> users who may have opened a dozen tabs of youtube videos and paused all but one
> of them to watch one at a time now having all tabs videos start playing again
> but for sites that don't have the plugins autoplay, this makes sense.
> 
> I just think it would be annoying to dismiss the notification bar N times for N
> number of tabs that had the crashed plugin loaded.  The users would then have
> to manually refresh each and every single tab.  This is turn wastes the users
> time and bandwidth for both the user and site (due to the following comment)
> just because the page would need to be manually refreshed when we technically
> don't have to.  Just for conversation sake here, what does chrome do?
> 

Don't forget its just as easy to right click the tab bar and select reload all tabs instead of doing so manually, or in the case stated, the user would see a need to reload individual tabs that broke down or maybe the browser could track the crashed tabs as suggested and offer to 'reload all affected tab(s)' rather than all tabs which use that plugin but didn't crash or all the browser tabs using the tabbar option.  

While I see the significance of knowing how many tabs, its just as easy to globally inform the user that 'plugin x crashed' and the user could 'try reloading all tab(s) running plugin x'.  I don't think many non techie users would understand otherwise unless the browser offers to 'reload all affected tab(s)' for them. 

I don't think there really is a need to tell the user how many tabs are actually affected.  I say for simplicity sake, any notification should just offer either a way to suggest a workaround because of the problem or offer to solve the problem (ie. from asking to reload and/or offer to do the plugin update check).

Comment 8

[(not currently active) Ted Mielczarek]

Here's what I would suggest, after thinking about it for a little bit:
* Plugin crashes, browser UI receives a notification
* On the first tab that the user views that contains the crashing plugin, display the notification bar saying that the plugin crashed, asking the user to submit a crash report, and indicating that the page can be reloaded to restart the plugin
* On subsequent tabs with crashed plugins, display a simpler notification bar simply saying "Plugin X has crashed. Reload this page to restart the plugin."

I believe the idea is also that the XBL attached to the plugin objects on the page will reload the page on click.

Comment 9

[Johnathan Nightingale [:johnath]]

(In reply to comment #8)
> Here's what I would suggest, after thinking about it for a little bit:
> * Plugin crashes, browser UI receives a notification
> * On the first tab that the user views that contains the crashing plugin,
> display the notification bar saying that the plugin crashed, asking the user to
> submit a crash report, and indicating that the page can be reloaded to restart
> the plugin
> * On subsequent tabs with crashed plugins, display a simpler notification bar
> simply saying "Plugin X has crashed. Reload this page to restart the plugin."

It may be too confusing to bother, but I think there's nothing stopping us from offering a "Reload all tabs using this plugin" button which does the obvious thing, and dismisses the remaining notification bars, ya?

As I say, it may be ugly, but it should probably be counted in the realm of the possible, since it's probably (usually) what you actually want to do (autoplay notwithstanding).

Comment 10

[Dão Gottwald [:dao]]

I'm not sure that's the work flow we'd want to promote. It could be pretty annoying if flash videos started to play simultaneously in different tabs.

Comment 11

[u88484]

Is there any way to track which video was actually playing and disable autoplaying (until tab focus) on the tabs that didn't have focus if the user choose to reload all tabs?

We have to remember that most users do not know anything about plugins and probably won't even understand why the video isn't playing on the other pages.  Plus, they might forget that the plugin had crashed by time they get to subsequent tabs.

I see talk that the user will be asked to submit a crash report, do we actually need to ask this?

BTW, what happens when a plugin actually crashes?  Does the tab actually close or will the plugin content just disappear?  I'm wondering because the user might not notice the notification bar.  Is there a plan to replace the area with some kind of message that the plugin crashed?

Comment 12

[(not currently active) Ted Mielczarek]

(In reply to comment #11)
> I see talk that the user will be asked to submit a crash report, do we actually
> need to ask this?

We feel that it's the right balance for privacy. We can always adjust the UI after we see how it works in practice.

> BTW, what happens when a plugin actually crashes?  Does the tab actually close
> or will the plugin content just disappear?  I'm wondering because the user
> might not notice the notification bar.  Is there a plan to replace the area
> with some kind of message that the plugin crashed?

The plugin content will disappear, and the plan is to use an XBL overlay to display some information in its place.

Comment 13

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (WIP)

First rough cut. I'm hitting some crashes and other bugs that are blocking progress on this, filing bugs next.

Comment 14

[Justin Dolske [:Dolske]]

Revised UI plans from limi (which Patch v.1 doesn't do yet)...

* Only show a notification bar when a page has no plugin region large enough to show the in-content UI. Notifications are a bit too heavy/annoying, and keeping the UI in content strengthens the association between the problem and the *plugin*.

* The first time a plugin crashes during a session, the in-content UI will include a checkbox to control crashreport submission. Further crashes of the plugin will not include the checkbox, and will do whatever the user's last selection was. This avoids asking the user to make a decision each time there's a crash.
** The checkbox will persist its value across sessions
** 1st crash ever will be checked by default

* Clicking the in-content UI (or notification bar) will reload the page (and include text to this effect), and deal with submitting the crash report (as will simply clicking reload).
** Will need to update in-content UI / notification bars for other tabs to remove the "submit crash report" checkbox, since it's only submitted once.
** Thought about reloading all pages with the dead plugin, but there's potential for dataloss so we'll not do that.

Reloading the page is the only sure-fire way to get things back into a normal state. At some point it would be interesting to experiment with restarting just the plugin without reloading the page, just to see how often it works.

FTR, all Google Chrome does is replace the plugin with the "sad puzzle-piece" icon and show a notification bar saying "The following plug-in has crashed: Shockwave Flash". Nothing about reloading, no context menus or clickability.

Comment 15

[[not reading bugmail]]

(In reply to comment #14)
> ** Thought about reloading all pages with the dead plugin, but there's
> potential for dataloss so we'll not do that.
> 

Good point.  Could session restore be invoked and restore data if we actually reload the tab from this UI, if we are going to choose to reload the tab(s) anyway?

I can see where not keeping it simple at least for now would present several edge cases to work around otherwise as noted already.  If the plugin can be restarted without reloading then that seems like a decent long term alternative. 

> 
> Reloading the page is the only sure-fire way to get things back into a normal
> state. At some point it would be interesting to experiment with restarting just
> the plugin without reloading the page, just to see how often it works.
> 

For sure.  As in the case of YouTube where it autoplays Flash, I would think that a special build could simulate the work flow here to find out and so on for other plugins.  It seems like it would have to pretend it was (re)loading a new page to start (so whatever code runs to do that normally), but not run code to invalidate the entire page content during the process.

Comment 16

[Alfred Kayser]

Keep it as simple as possible, and only send a crashreport the first time (as described above, with the checkmark).
But don't do any automatic stuff. Reload = Reload. Something on the page has failed, just reload the page. The browser should automatically start/execute a specific part of a page... Make it clear to the end-user that the plugin has caused the crash, and don't take responsibility from the plugin to recover its problems...

Comment 17

[(not currently active) Ted Mielczarek]

(In reply to comment #14)
Overall this sounds good, except:

> * The first time a plugin crashes during a session, the in-content UI will
> include a checkbox to control crashreport submission. Further crashes of the
> plugin will not include the checkbox, and will do whatever the user's last
> selection was. This avoids asking the user to make a decision each time there's
> a crash.
> ** The checkbox will persist its value across sessions
> ** 1st crash ever will be checked by default

If we're going to persist this value, we need to have a way for users to opt-out in the future. It's not okay to just always send crash reports without giving the user a way out.

Comment 20

[Justin Dolske [:Dolske]]

(In reply to comment #17)

> If we're going to persist this value, we need to have a way for users to
> opt-out in the future. It's not okay to just always send crash reports without
> giving the user a way out.

The value would persist, and when the checkbox is shown for the first crash in a session it will use the persisted value and the user has the opportunity to change their mind. I'm not sure giving the option for every crash in a session is useful for most users, or is worth the UI cost.

Comment 21

[(not currently active) Ted Mielczarek]

Oh, sorry, I missed "in a session" there. Yeah, that seems ok.

Comment 22

[u88484]

(In reply to comment #10)
> I'm not sure that's the work flow we'd want to promote. It could be pretty
> annoying if flash videos started to play simultaneously in different tabs.

FWIW, this already happens with session restore.  Has there been many bugs because of this behavior?  Bug 540127 was just filed about session store disabling autoplay and it is the only bug I can find about this.  Maybe if that bug is accepted OOPP can make use of it?  Either way, this is the current behavior for session store so it might not be so bad to allow the user to reload all tabs from the notification bar for tabs that included the crashed plugin.

Comment 23

[Justin Dolske [:Dolske]]

Attachment: Patch v.2 (WIP)

Mostly implemented; still need to see about actually triggering the crash report submission, and need to make it properly deal with multiple instances of a plugin crashing.

Comment 24

[Justin Dolske [:Dolske]]

This UI is turning out to be a bit difficult to implement, so I'd like to suggest a change. Specifically, the problem is that if we show a checkbox, we need to jump through hoops to update the state of all the checkboxes when you change one, and then hide all the checkboxes once the report has been submitted. Otherwise, I think it'll be very confusing when a plugin crashes in multiple tabs and one is reloaded. Or if there are multiple plugin instances on a page, and the user changes the checkboxes to different values. Or when pages in fastback history are revisited, and the plugin-crashed UI is still there to be updated.

So, instead of the checkbox, we would show a message like "A crash report has been submitted." (unless submission was disabled, in which case we show nothing). We should probably show the message for each crash, since it's not requiring a decision to be made (or maybe just the 1st crash? I don't feel strongly). We can linkify "crash report" to go to a SUMO page explaining what it means and how to disable it.

We'd also add a checkbox to the prefs UI (eg prefs -> Advanced -> General -> System defaults, [x] Submit crash reports). I suppose while I'm there I should make that the default state for the regular crash reporter. A little more work, but it's fairly straightforward and will placate those who have already disabled the regular crash reporter.

Limi's ok with the general idea, though suggested we may still want a one-time prompt to set the initial default submit-or-not state. Mconnor thought that wasn't necessary, though.

Comment 25

[Alex Faaborg [:faaborg] (Firefox UX)]

>So, instead of the checkbox, we would show a message like "A crash report has
>been submitted." (unless submission was disabled, in which case we show
>nothing)

I think this is actually a better UI, the user is informed if they bother to read the message, but we don't put extra strain on them by introduce an additional interactive widget for them to think about (the checkbox).  The behavior is also more consistent over time, I didn't really like the earlier proposal where sometimes there was a check box, and other times there wasn't one.

I don't think we need to bother with the one time prompt, the minority of users who want this control shouldn't overwhelm the silent majority of users who don't really care, and would prefer a browser that isn't constantly jumping in their way to ask questions that they don't care about.

Comment 26

[(not currently active) Ted Mielczarek]

FWIW, Chrome implements their crash reporting similarly, with a pref to toggle it, and no crash reporting UI to speak of.

Comment 27

[Justin Dolske [:Dolske]]

Note to self: bsmedberg points out that we should check to see if this requires a privacy policy change or not.

Comment 28

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Good for a review pass. One part I'm not quite sure how to deal with is ensuring that the binding is attached when we poke at it. I first tried a setTimeout(0) to let the event queue empty out first, but that was unreliable. I've just got a bigger timeout for now. :( Not sure if there's way to do this without polling.

The actual crash report submission isn't tied in here yet, but that can happen in a followup if needbe. (Ted says he should have that ready in the next couple days, so that might not matter).

Comment 29

[Justin Dolske [:Dolske]]

Oh, I should note that the mess with CSS styles and DTD entities is because I wrote this patch with landing on 1.9.2 in mind. For trunk I want to hit this code with a big Rewrite Stick, and will clean it up there. Will file a new bug on that before landing.

Comment 30

[Benjamin Smedberg]

> ensuring that the binding is attached when we poke at it. I first tried a


Can you fire a callback or a a trusted DOM event from the XBL constructor?

Comment 31

[Axel Hecht [:Pike]]

Comment on attachment 422445 [details] [diff] [review]
Patch v.3

A few comments from an l10n point-of-view:

The 1,2,3 entity names are not really great, we're commonly using .pre and .post, together with a localization note. For example see http://mxr.mozilla.org/mozilla1.9.2/source/browser/locales/en-US/chrome/browser/preferences/privacy.dtd#63.

An alternative way is to just keep a dummy <html:a class="foo"> markup in a single entity. Not sure how much of the inline style needs to be actually inline in xbl, though.

I'd favour having the new strings in distinct files. That way, I can make the l10n dashboard exclude the new strings independent of what they precisely are.

It also has the benefit that you can use build-magic to code the fallback, for example by using an #include of the en-US DTD file. You could put the .properties file into both content and locale, and use the content-based string bundle as fallback. I expect that to be easier to abstract such that things don't get out of sync or break.

Comment 32

[Boris Zbarsky [:bzbarsky]]

> Can you fire a callback or a a trusted DOM event from the XBL constructor?

You can do that, or you can make pluginCrashed flush out restyles, right?  Given that this is a chrome binding (so loads synchronously once someone asks for it), I'd think that would do the trick.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 422445 [details] [diff] [review]
Patch v.3

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+  doPluginCrashedUI: function (plugin) {

>+    let overlay = doc.getAnonymousElementByAttribute(plugin, "class", "overlay");
>+    let textToShow = doc.getAnonymousElementByAttribute(plugin, "class", showClass);
>+    let link = doc.getAnonymousElementByAttribute(plugin, "class", "reloadLink");
>+        let overlayContents = doc.getAnonymousElementByAttribute(plugin, "class", "overlayContents");

Can you just define fields for these on the binding?

>+    let crashText = doc.getAnonymousElementByAttribute(plugin, "class", "crashText");
>+    crashText.textContent = messageString;

Could perhaps set xbl:inherits="xbl:text=crashtext" on "crashText" and then just plugin.setAttribute("crashtext", messageString)? Not sure that will fall back to the hardcoded attribute value correctly, but worth a shot.

>+    if (isObjectTooSmall) {
>+        // If another plugin on the page was large enough to show our UI, we
>+        // don't want to show a notification bar.
>+        if (!doc.mozNoPluginCrashedNotification)
>+          showNotificationBar();
>+    } else {
>+        // If a previous plugin on the page was too small and resulted in
>+        // adding a notification bar, then remove it because this plugin
>+        // instance it big enough to serve as in-content notification.
>+        hideNotificationBar();
>+        doc.mozNoPluginCrashedNotification = true;
>+    }

Hmm, mozNoPluginCrashedNotification seems hacky, but not sure what would be better. I guess ideally we would only run this code once per document/tab, though not sure how much trouble that would be on the backend...

>+    function hideNotificationBar() {

>+      // removeChild instead of removeNotification to avoid animation
>+      if (notification)
>+        notificationBox.removeChild(notification);

You'll also need to manually clear notificationBox.currentNotification to avoid wonkiness when later adding notifications to this box, I think. Odd that removeNotification doesn't have an "aImmediate" the same way removeAllNotifications does...

>diff --git a/toolkit/mozapps/plugins/content/missingPlugin.xml b/toolkit/mozapps/plugins/content/missingPlugin.xml

>+  <binding id="crashedPlugin" inheritstyle="false">

Can the styling rules for this binding go into missingPlugin.css rather than having them hardcoded in style attributes?

(I think the default sizing rules in missingPlugin.css belong in missingPluginBinding instead, but that's a separate issue.)

>diff --git a/toolkit/themes/pinstripe/mozapps/plugins/missingPlugin.css b/toolkit/themes/pinstripe/mozapps/plugins/missingPlugin.css

>+.crashedPlugin#disabledPluginPlaceholder {

Seems like these should actually be using class selectors rather than ID selectors, but I suppose that's also an existing issue.

Comment 34

[Justin Dolske [:Dolske]]

(In reply to comment #33)

> Can you just define fields for these on the binding?

No, because I'd need to use JS in the binding to initialize the fields... And if the user has JS disabled, things would then break. XBL2, you're our only hope!

> Could perhaps set xbl:inherits="xbl:text=crashtext" on "crashText" and then
> just plugin.setAttribute("crashtext", messageString)? Not sure that will fall
> back to the hardcoded attribute value correctly, but worth a shot.

Well... |plugin| here is the <object> itself, so I'm wary of exposing this to content or the possibility of mucking with attributes content might be using for it's own purposes.

For trunk I'd like to rewrite all this using some native anonymous content -- like videocontrols -- which I think will help with letting us do this a bit more cleanly. This should also let us remove the CSS pseudoclasses used, and just have the plugin code set attributes on the native anonymous content.

> Hmm, mozNoPluginCrashedNotification seems hacky, but not sure what would be
> better.

Yeah, it is hacky. Didn't see an better way of doing this. :/

> Odd that
> removeNotification doesn't have an "aImmediate" the same way
> removeAllNotifications does...

Hmm, maybe I'll just add a new arg/method there. Should be able to do that without any incompatibility concerns for 1.9.2...

> >+  <binding id="crashedPlugin" inheritstyle="false">
> 
> Can the styling rules for this binding go into missingPlugin.css rather than
> having them hardcoded in style attributes?

For trunk, yes, but for 1.9.2 I think it will need to be that way in case the user is using a non-default theme. [There's no perfect solution in that case, but I suspect most themes just copy over the default stuff for this and don't change it.]

> >+.crashedPlugin#disabledPluginPlaceholder {
> 
> Seems like these should actually be using class selectors rather than ID
> selectors, but I suppose that's also an existing issue.

Indeed. As above, I was being clever here so that non-default themes will get the disabledPluginPlaceholder icon instead of nothing.

Comment 35

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 422445 [details] [diff] [review]
Patch v.3

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+  pluginCrashed: function (aEvent) {

>+    // Our XBL binding hasn't attached yet, back to the event queue.
>+    // XXX Ugh, a timeout of 0 doesn't always work. Might need to nest a
>+    // couple of setTimeout()s? I'd guess any hardcoded single time can be
>+    // unreliable under load?
>+    setTimeout(function() { gMissingPluginInstaller.doPluginCrashedUI(plugin); }, 20);

>+if (!overlay)
>+  dump("\n\nCRAP! Our XBL binding didn't attach yet!\n\n\n");

The latest patch in bug 539828 makes these unnecessary, I hope?

>diff --git a/toolkit/mozapps/plugins/content/missingPlugin.xml b/toolkit/mozapps/plugins/content/missingPlugin.xml

>+  <binding id="crashedPlugin" inheritstyle="false">

>+            <html:span>&submitted1;<html:a class="submitLink" href="https://support.mozilla.com/kb/Mozilla+Crash+Reporter" style="display: inline; border: none; padding: 0px; vertical-align: baseline; text-decoration: underline; color: -moz-hyperlinktext; background: transparent">&submitted2;</html:a>&submitted3;</html:span>

Hmm, perhaps we should pref this href (and have the pluginCrashed event fill it in)?

r=me with those and Axel's l10n issues addressed.

Comment 36

[Justin Dolske [:Dolske]]

(In reply to comment #35)

> >+if (!overlay)
> >+  dump("\n\nCRAP! Our XBL binding didn't attach yet!\n\n\n");
> 
> The latest patch in bug 539828 makes these unnecessary, I hope?

Actually, bz's suggestion in comment 32 seems to do the trick.


> >+            <html:span>&submitted1;<html:a class="submitLink" href="https://support.mozilla.com/kb/Mozilla+Crash+Reporter"

> Hmm, perhaps we should pref this href (and have the pluginCrashed event fill it
> in)?

I'll check with the Sumo folks (since this probably ought to be a customized page), but I think it redirects to the localized page as needed.

It can just be an entity, no?

Comment 37

[Axel Hecht [:Pike]]

Sadly, localizers get URLs wrong too often. Including thins like translating "support" to turn that into https://hilfe.mozilla.com etc.

We have a working scheme on how to link to sumo from the product, we should make this go through the same loop, that will make extra cases like Norwegian and such work as it should. David?

Comment 38

[Reed Loden [:reed]]

Comment on attachment 422445 [details] [diff] [review]
Patch v.3

>+          <xul:hbox class="submittedText" style="display: none;">
>+            <html:span>&submitted1;<html:a class="submitLink" href="https://support.mozilla.com/kb/Mozilla+Crash+Reporter" style="display: inline; border: none; padding: 0px; vertical-align: baseline; text-decoration: underline; color: -moz-hyperlinktext; background: transparent">&submitted2;</html:a>&submitted3;</html:span>
>+          </xul:hbox>
>+          <xul:hbox class="notSubmittedText" style="display: none;">
>+            <html:span>&notSubmitted1;<html:a class="notSubmitLink" href="https://support.mozilla.com/kb/Mozilla+Crash+Reporter" style="display: inline; border: none; padding: 0px; vertical-align: baseline; text-decoration: underline; color: -moz-hyperlinktext; background: transparent">&notSubmitted2;</html:a>&notSubmitted3;</html:span>
>+          </xul:hbox>

Yes, these URLs definitely need to be prefs.

Comment 39

[Reed Loden [:reed]]

(In reply to comment #37)
> We have a working scheme on how to link to sumo from the product, we should
> make this go through the same loop, that will make extra cases like Norwegian
> and such work as it should. David?

Indeed, stuff should use app.support.baseURL.

Comment 40

[Alex Faaborg [:faaborg] (Firefox UX)]

dolske, stephen: we are still planning to go with a sad face interlocking brick, for the icon, right?  Just asking since I didn't see it in the last round of design work.

Comment 41

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

Cleaned up, made some more changes. I went ahead and rewhacked the XBL and CSS in bug 539848 (which this is on top of), which simplifies the changes here.

Comment 42

[Alfred Kayser]

bugs 539834, 539836 are allready in the 'depends on' list in bug 539848.

Comment 43

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 424739 [details] [diff] [review]
Patch v.4

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+  // Crashed-plugin observer. Notified once per plugin crash, before events
>+  // are dispatched to individual plugin instnaces.

"instances"

>+  pluginCrashed : function(subject, topic, data) {

>+    let minidumpID = subject.getPropertyAsAString("minidumpID");

>+    gMissingPluginInstaller.CrashSubmit.submit(minidumpID, gBrowser, null, null);
>+    propertyBag.setPropertyAsBool("submittedCrashReport", submitReports);

I think these belong in an if (submitReports) ?

>+  pluginInstanceCrashed: function (aEvent) {

>+    let pluginName = plugin.pluginInfo.name;

This is going to change to getting the name off the event, per bug 541076, right?

>+    function showNotificationBar() {
>+      // If there's already an existing notification bar, don't do anything.

Maybe we should replace the notification to update the crashed plugin name? I suppose it doesn't matter too much...

>diff --git a/toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd b/toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd

>+<!ENTITY crashedPlugin.label                                 "This plugin has crashed.">

Is there any point to having this string given that we set it unconditionally in the browser code? I suppose it could be useful if for some reason that handler isn't called, but that doesn't seem likely.

Comment 44

[Justin Dolske [:Dolske]]

Attachment: Patch v.5

Addressed review comments, removed "hacky UI" from bug 539048.

Comment 45

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 425571 [details] [diff] [review]
Patch v.5

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+    if (!(aEvent instanceof Ci.nsIDOMDataContainerEvent))
>+      return;
>+    let submittedReport = aEvent.getData("submittedCrashReport");
>+    let pluginName      = aEvent.getData("pluginName");

nit: newline after the return?

I just reviewed an interdiff, looks good.

Comment 46

[Justin Dolske [:Dolske]]

Attachment: Patch v.6 [Checked in: Comment 48 & 59 & 63]

Updated with nit and merge fluff from earlier patches in queue.

Comment 47

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 426106 [details] [diff] [review]
Patch v.6
[Checked in: Comment 48 & 59 & 63]

Might want to refine the strings or icons later, but in general this is great and should land.

Nice work on the inset look, look's absolutely fantastic and really establishes that the plugin is thing that overlays part of the content area.  I love how it looks lifted out when it is gone.

Comment 48

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/b2e6a7183e28

Comment 49

[Axel Hecht [:Pike]]

Can I get a heads-up notice before actually landing this on the lorentz branch?

I need to be rather specific on the changes to the l10n logic, so I need to know exactly what we're landing.

Comment 50

[Benjamin Smedberg]

Axel, this will have a trunk alpha before landing on Lorentz, and I think we'll just land the strings directly on 1.9.2, with whatever compare-locales/optional strings magic you need soon after that.

Comment 51

[Axel Hecht [:Pike]]

Clarification from the delivery meeting, in lingo that I understand, and possibly others in the l10n team:

There will be a "just strings" patch to be landed on mozilla-1.9.2, which will be merged onto lorentz, and a "just code" patch to be landed on just lorentz, and then at some point that will merge back to 1.9.2.

Which is a cool alternative to me hacking along the string patch in filter.py.

Comment 52

[Boris Zbarsky [:bzbarsky]]

I'm a little confused by the "html|A" stuff in those stylesheets.  Why is that rule there (hardcoding a color at that) and why is it using a capital "a"?

Comment 53

[Steffen Wilberg]

Adding user-doc-needed keyword for http://support.mozilla.com/1/firefox/3.7a2pre/WINNT/en-US/plugin-crashed/.

Comment 54

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #52)
> I'm a little confused by the "html|A" stuff in those stylesheets. Why is that
> rule there (hardcoding a color at that)

Because the text is in a box whose background is backgroundStripes.png, a dark grey. I suppose that style should just be put where we define that background (on the box), though, instead of on .msg and html|a specifically.

> and why is it using a capital "a"?

Does this matter?

Comment 55

[Boris Zbarsky [:bzbarsky]]

> I suppose that style should just be put where we define that background

That would be preferable, yes.  Also note that when printing you might end up with weird effects here (since we don't print backgrounds by default).  Probably ok.

> Does this matter?

Yes.  "html|A" won't match that <html:a> element if the document the crashed plug-in is in is not text/html.

Comment 56

[Axel Hecht [:Pike]]

Seems that the landing on lorentz has nothing to do with either plan to make compare-locales report OK.

Can't really verify the patch as hg only returns 500s for the firefox-lorentz project branch for me.

Comment 57

[Justin Dolske [:Dolske]]

(In reply to comment #56)
> Seems that the landing on lorentz has nothing to do with either plan to make
> compare-locales report OK.
> 
> Can't really verify the patch as hg only returns 500s for the firefox-lorentz
> project branch for me.

I'm not sure what you saying.

Bsmedberg just started the backport work a day or two ago, and the strings just changed at the same time due to bug 550293. Once he gets things ported over to the project branch (as things exist on trunk), I'm going to go in and modify things to (1) split out the new strings to separate files per comment 31 and (2) make the code changes to deal with fallback (as the first few versions of the patch in this bug did).

At that point we can land the new-string files onto the 1.9.2 branch.

Comment 58

[Axel Hecht [:Pike]]

Landing on the lorentz branch has started, which means that the l10n dashboard now reports strings missing for lorentz, and if localizers catch up on them, it will report them as obsolete on 1.9.2. Which is hell confusing.

Comment 59

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/0a1a29578f51

Comment 60

[Toni Hermoso Pulido]

(In reply to comment #58)
> Landing on the lorentz branch has started, which means that the l10n dashboard
> now reports strings missing for lorentz, and if localizers catch up on them, it
> will report them as obsolete on 1.9.2. Which is hell confusing.

Hi Axel,

where are these strings? Where should they be commited. I found no ref in l10n-dev list. Thanks!

Comment 61

[Axel Hecht [:Pike]]

http://groups.google.com/group/mozilla.dev.l10n/browse_frm/thread/05b4bf36bbb4f6cc#, the strings are both on 1.9.2 (without being reported, though) and on http://hg.mozilla.org/projects/firefox-lorentz/. The localized strings should land on 1.9.2, filter.py does the right thing for both lorentz and 3.6.x.

Comment 62

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 63

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430

Comment 64

[Verdi [:verdi] Best to slack me]

I had a flash plugin crash on one tab today (using Fx 4.0b8pre). I noticed that from a user's point of view the most important thing I want to know is how to get my video to work again but there isn't any information displayed telling me how to do that. That information isn't shown until after you click the send crash reports link. Looking through this bug I noticed that idea in the original post was to have a reload button as part of the initial notification. I think it would be good to reconsider adding some way to reload page (and what that will do) to the initial notification.