648675 - Allow comments in content/plugin crash UI

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Robert Kaiser]

Once we have e10s set up, we need to ensure we still are able to get comments and email addresses o people crashing in content.

Not sure what component the current plugin crash UI that e10s also seems to use belong to, actually, but we need to get something going there.

Comment 1

[(not currently active) Ted Mielczarek]

There is no e10s content crash UI yet in Firefox. Fennec has one (which I don't believe allows comments currently), but Firefox's will likely be different. I originally filed bug 525850 on the Firefox UI, but it looks like it got stolen for the Fennec UI work.

Comment 2

[Benjamin Smedberg]

This is first about plugins. The content crash handling will be very similar.

I'm not convinced we should try to shove a comment field into te existing UI: perhaps allow a comment after the submission?

Comment 3

[Justin Dolske [:Dolske]]

Post-submission sounds preferable to me. We're looking at overhauling about:crashes to make it more usable for, err, end-users... Might want to ponder this in that context.

Comment 4

[chris hofmann]

A recent security study showed a pretty high pct. of users liked the idea of providing comments and crash reports to help improve firefox.   maybe we should ask that question on first crash and retain the preference.  that would allow us to get out of the way of people that don't want to participate, and empower and employ those that do.

Comment 5

[(not currently active) Ted Mielczarek]

Post-crash may be preferable, but keep in mind that that entails Socorro backend changes. Socorro is not currently setup to add additional information to existing crash reports. Also it adds a layer of complexity. Do you allow anyone to comment on a crash report? Only the original submitter? How do you authenticate that?

Comment 6

[Benjamin Smedberg]

That's not hard to solve with a collector-generated security token. But I do think we need to talk to jboriss to do some end-to-end UI design here, since there are a bunch of seemingly conflicting goals.

Comment 7

[Robert Kaiser]

(In reply to comment #3)
> Post-submission sounds preferable to me.

The problem I'm seeing there is that if people will not get to the comment entry textbox by default, most of them will never even bother to add a comment.

An option to do something post-crash would be interesting though, esp. in the light of bug 648444, where a mechanism to communicate back from our side to the clients out there would be needed as well.

Comment 8

[Robert Kaiser]

(In reply to comment #7)
>  esp. in the
> light of bug 648444

Er, sorry, forget that part of the note, the bug is about something totally different. The project I was thinking of only has a wiki page so far and not much info entered into it. More will come when we figure out more details and who to pull in, right now there's only https://wiki.mozilla.org/CrashKill/Plan/User_Notifications

Comment 9

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Plug-in crash screen with a text field added for user comments

Sounds like a first iteration could just display a text field, giving users the option to describe what was going on before the crash.

What to say in the instructional text really depends on what information you'd like from users.  The Mozilla Crash Reporter currently says "Add a comment (comments are publicly visible)."  Would plug-in crash report comments be publicly visible?

Giving this field before submission as opposed to after is a better user experience and more likely to get responses.  The act of submitting a report is likely to be viewed as the concluding step of the process, making the user feel they've finished.  Adding a step on the end could feel annoying at best and be ignored and worst.  Also, it takes more steps to complete the task if comments are given at the end:

Submitting a comment *before* the submission is 5 steps:

(mouse to text field)+(click to target text field)+(type comment)+(mouse to submit link)+(click submit)

Submitting a comment *after* the submission is 7 steps:

(mouse to submit link)+(click submit)+(mouse to text field)+(click to target text field)+(type comment)+(mouse to second submit link)+(click submit)

Another question here is at what sizes of crashed plug-ins to display the text field, and if we do something different in the cases when the crashed plug-in is too small for a text field.  Smedburg tells me that just displaying this field in crashed plug-ins large enough to display it should give good enough data in the first iteration.  So, maybe we could show the current crashed plug-in message for crashed plug-ins less than about 388 x 217 pixels.

Comment 10

[(not currently active) Ted Mielczarek]

(In reply to comment #9)
> What to say in the instructional text really depends on what information you'd
> like from users.  The Mozilla Crash Reporter currently says "Add a comment
> (comments are publicly visible)."  Would plug-in crash report comments be
> publicly visible?

Yes, plug-in crash reports are treated virtually identically to browser crash reports. I think reusing this string is the best thing to do for now.

Comment 11

[Robert Kaiser]

I fully agree with comment #9 (including the comments on sending before/after), and the mockup looks nice!
I also agree to only show this where the area is large enough, as any comments we get are better than none - in that case, I just wonder slightly if we'd might want to send something along with the report that indicates that no comment field was shown or if we should just completely ignore it.

Comment 12

[Benjamin Smedberg]

Just ignore it. Now who can we get to build this UI?

Comment 13

[Sheila Mooney]

I am going to start a feature tracking page for this one.

Comment 15

[Rob]

I was duped here via Comment 14.

My complaint (RFE) in Bug 700880 was that we should add a comment that states the crash was caused by a Plugin (to distingush the crash in the about:crashes URLs from Reports where the User either did not enter a comment OR a comment was entered but not recorded due to some error).

While we wait to add the feature of 'allowing the User to add a comment to Plugin crashes' could we have a 'default comment'?

It could state that the "Crash was in the background from a Plugin" (and that the "Mozilla Error Reporter" did not pop up for the User to see). 


We have had problems like:
1. in Bug 700880 where it was unclear (to me) why my comments where not being recorded when I am careful to always try to provide a useful comment.
2. in Bug 692149 (and Bug 692685) the npmozax.dll Plugin crashed constantly and filled the User's about:crashes with _thousands_ of Reports, (in Bug 692685 Comment 1 through 4 there was extra work for people (it was unclear (to them)) at mozilla.com why we had this problem.

In both those cases a 'default comment' (that the crash was from a plugin and did not involve the "Mozilla Error Reporter" (a hidden crash)) would have saved a lot of people's time with only a little extra effort (a couple of lines of Code).


Thanks.

Comment 16

[Robert Kaiser]

(In reply to Rob from comment #15)
> My complaint (RFE) in Bug 700880 was that we should add a comment that
> states the crash was caused by a Plugin (to distingush the crash in the
> about:crashes URLs from Reports where the User either did not enter a
> comment OR a comment was entered but not recorded due to some error).


That's something different and already covered by the "process type" that is shown in the crash reports. It's not the job of about:crashes itself to show that, but the crash reports linked from there do.

Comment 17

[Drew Willcoxon :adw]

Bug 676719 needs a similar UI change.  I'll work from Jennifer's mockup.

Comment 18

[Drew Willcoxon :adw]

Attachment: WIP patch

This also fixes bug 676719.  To do:

* Style, and I haven't checked how things look on Windows and Linux.
* An automated test I guess, but I don't know how to go about it.

Comment 19

[Drew Willcoxon :adw]

Attachment: WIP screenshot

Jennifer, what do you think?  What should we do about the "(comments are publicly visible)" part of your mockup?  Page URLs are publicly visible too (I assume), but parentheticals on both the textbox and the checkbox look bad.  I tried adding a similar statement above the textbox and alternatively above the Send button: above the textbox looks all right, but it makes the UI much busier.

Also, could you give me the hex color codes for all the new parts in your mockup?  Are they the same for all platforms?

Comment 20

[Drew Willcoxon :adw]

... my retina MacBook takes retina screenshots.

Comment 21

[Benjamin Smedberg]

Page URLs are *not* publicly visible; comments are.

Maybe "comments are publicly visible" could go with the existing grey "Add a comment"?

Comment 22

[Drew Willcoxon :adw]

Oh, in that case I'll just follow the mockup.

Comment 23

[Drew Willcoxon :adw]

Attachment: patch

Comment 24

[Drew Willcoxon :adw]

Comment on attachment 672633 [details] [diff] [review]
patch

Benjamin asked Ted for a second review in bug 676719, so I'll move the request over here.  Gavin told me not to block on it, though.

Comment 25

[(not currently active) Ted Mielczarek]

Comment on attachment 672633 [details] [diff] [review]
patch

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

::: browser/base/content/test/browser_pluginCrashCommentAndURL.js
@@ +21,5 @@
> +           'width="400" height="400"' +
> +           'drawmode="solid" color="FF00FFFF">' +
> +    '</embed>' +
> +  '</body>' +
> +  '</html>';

Is there any compelling reason to not just put this in a file and load it via http?

@@ +38,5 @@
> +            getService(Components.interfaces.nsIEnvironment);
> +  let noReport = env.get("MOZ_CRASHREPORTER_NO_REPORT");
> +  let serverURL = env.get("MOZ_CRASHREPORTER_URL");
> +  env.set("MOZ_CRASHREPORTER_NO_REPORT", "");
> +  env.set("MOZ_CRASHREPORTER_URL", SERVER_URL);

You can also just set nsICrashReporter.serverURL here.

::: toolkit/crashreporter/CrashSubmit.jsm
@@ +119,5 @@
>  }
>  
>  // the Submitter class represents an individual submission.
> +function Submitter(id, submitSuccess, submitError, noThrottle,
> +                   extraExtraKeyVals) {

extraExtraKeyVals is a pretty awkward variable name.

@@ +279,5 @@
>  
> +    let extraKeyVals = parseKeyValuePairsFromFile(extra);
> +    for (let key in extraKeyVals) {
> +      if (!(key in this.extraKeyVals)) {
> +        this.extraKeyVals[key] = extraKeyVals[key];

You should document the fact that values passed in extraExtraKeyVals will override values from the .extra file.

Comment 26

[Justin Dolske [:Dolske]]

Comment on attachment 672633 [details] [diff] [review]
patch

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

Some small comments on the code, but I'm mainly concerned about two UI issues caused by adding a comment box:

1) On pages with multiple plugins, the user would now get a barrage of busy UI. (That's also one of the reasons the current UI has links instead of graphical buttons)

2) Addition of large UI means isTooSmall() [in browser-plugins.js] will trigger more often (which results in a notification bar instead of in-content UI).

A few options to consider (Boriss/UX, thoughts?)
 - Do nothing, suck it up due to cost/benefit.
 - Current UI + "Add comment" link to reveal extra UI
 - Like previous, but make the (optional) comment part of a required 2-step process. EG, main action link becomes "Help Mozilla...", which shows the extra UI and reveals a "Submit" button.
 - Like previous options, but put the "extra UI" be a tab overlay (like tab modal prompts), to get around in-content space limitations. This could also be used for the notification bar UI, which is even harder to cram a comment box into. ;-)

The last of these would be the most extra work (though still pretty small), but is particularly appealing as it gives us more space to engage / explain / encourage.

One mitigation specifically for #2 would be to try hiding the textfield+checkbox and then see if it's still too big (before finally falling back to the notification bar). We do something like this for video controls -- we start hiding pieces of the UI as the media gets smaller and smaller (bug 462117).

Last comment: haven't thought about this as much given the other issues, but I'd want to consider having the "send url" checkbox be a pref observer. That would avoid the potential for the user unchecking it in one place and is then surprised when it's still checked for some other submission. But I suspect this is edge case stuff that's not worth doing.

::: browser/base/content/browser-plugins.js
@@ +262,5 @@
> +  submitReport: function submitReport(pluginDumpID, browserDumpID, plugin) {
> +    let keyVals = {};
> +    if (plugin) {
> +      let doc = plugin.ownerDocument;
> +      let elt = doc.getAnonymousElementByAttribute.bind(doc, plugin, "class");

Mmm, we sure do use this all over (even in existing code).

Hows about we just add a helper member? EG:

getPluginUI : function(plugin, name) {
  doc = plugin.ownerDocument;
  elt = doc.blahblah(doc, plugin, "class", name);
  return elt;
}

...
  let comment = this.getPluginUI(plugin, "userComment");
...

Bonus points if you want to do the trivial followup bug to make all the existing call sites do this. :)

@@ +269,5 @@
> +        keyVals.PluginUserComment = userComment;
> +      if (elt("submitURLOptIn").checked)
> +        keyVals.PluginContentURL = doc.URL;
> +    }
> +    this.CrashSubmit.submit(pluginDumpID, { extraExtraKeyVals: keyVals });

Rather than adding extraExtraKeyVals (extra! extra! read all about it! ;), let's just leave this a flat object, i.e. pass |{ comment: text, url: blah}|.

And if I may bikeshed slightly, I'd just go with "userCommand" and "contentURL".

@@ +668,5 @@
> +      optInCB.addEventListener("click", function (event) {
> +        if (optInCB.checked)
> +          pref.setBoolPref("", true);
> +        else
> +          pref.deleteBranch("");

Why use a branch here? Seems like it would be simpler to add it to all.js, and simply get/set by name as needed.

::: toolkit/crashreporter/CrashSubmit.jsm
@@ +119,5 @@
>  }
>  
>  // the Submitter class represents an individual submission.
> +function Submitter(id, submitSuccess, submitError, noThrottle,
> +                   extraExtraKeyVals) {

I suggested in the front-end code to flatten the xtraxtra thing, but here it probably makes sense to start taking a single xtraParams arg. Dunno if that's easy to do just do here and now, or if would be better to do the ugly thing for now (add userComment / pluginURL args), and refactor in a followup.

Comment 27

[Justin Dolske [:Dolske]]

(Bug 665196's history may or may not be slightly relevant here too)

Comment 28

[Robert Kaiser]

(In reply to Justin Dolske [:Dolske] from comment #26)
>  - Like previous, but make the (optional) comment part of a required 2-step
> process. EG, main action link becomes "Help Mozilla...", which shows the
> extra UI and reveals a "Submit" button.

I'd fear that this would lead to significantly fewer submissions of reports.

Comment 29

[Benjamin Smedberg]

Attachment: Possible UI

At this point, I think that getting URL and comment information is probably more important to us than making the UI as low-friction as possible. Here's one other possibility: when a plugin crashes, overlay the UI kinda like this.

Do this on every tab, but only until the dialog has been submitted or cancelled. Then show the current reload UI (either in-page or as a notification bar).

Alternately, we could show an entirely separate modeless window with this type of UI when the plugin crashes, and make the in-page UI as streamlined as we want.

Comment 30

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

(In reply to Justin Dolske [:Dolske] from comment #26)
> A few options to consider (Boriss/UX, thoughts?)
>  - Do nothing, suck it up due to cost/benefit.

I'm leaning towards this option - seems like we shouldn't be caring too much about the "noisiness" of pages where plugins have crashed, and I'm not sure how common the noisiness would even be an issue (only on pages with multiple copies of the same plugin, right?).

>  - Current UI + "Add comment" link to reveal extra UI
>  - Like previous, but make the (optional) comment part of a required 2-step
> process. EG, main action link becomes "Help Mozilla...", which shows the
> extra UI and reveals a "Submit" button.

These seem like too high of a bar to submit comments.

The whole-page overlay seems like overkill to me (and more complicated to implement).

> One mitigation specifically for #2 would be to try hiding the
> textfield+checkbox and then see if it's still too big (before finally
> falling back to the notification bar). We do something like this for video
> controls -- we start hiding pieces of the UI as the media gets smaller and
> smaller (bug 462117).

... but this mitigation probably makes sense. It would skew the comment submissions towards crashes of plugins that were large enough to show the comment UI, but that's probably fine since in theory those are the crashes we care most about (when the plugin is the primary content on the page, e.g. youtube, it's likely to be big enough to show the comment UI).

Comment 31

[Jennifer Morrow [:Boriss] (UX)]

I agree with Gavin in Comment 30 in support of dolske's idea in Comment 26 - let's scale this box down as the size of the content area reduces.

The problem with overlays and page-modal dialogs, like Smedberg's attachment in Comment 29, is that the page may still be entirely usable one a plugin's gone belly up.  The user could still be interacting with the page.  Hell, I have a tab open now where the crashed plugin was an advertisement.  Free Adblock Plus!  Well I guess AP is free.  You know what I mean.

To make the user halt their actions and focus on our crash report is disruptive to the user's task and also draws more attention to a problem - one they may not care about.

It's true that this solution means that crash reports will be skewed towards large media, but Gavin's again correct that those are probably the most important reports as well.

Comment 32

[Justin Dolske [:Dolske]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #30)
> >  - Do nothing, suck it up due to cost/benefit.
> 
> I'm leaning towards this option - seems like we shouldn't be caring too much
> about the "noisiness" of pages where plugins have crashed, and I'm not sure
> how common the noisiness would even be an issue (only on pages with multiple
> copies of the same plugin, right?).

I'm not quite able to parse that. I meant that option as a WONTFIX of this bug, but I think you're reading it as taking this patch (ie, do nothing about UI problems)?

> The whole-page overlay seems like overkill to me (and more complicated to
> implement).

Hmm, another idea around whole-page overlay and the general interruptiveness (as Boriss notes in comment 31)...

Would it be useful to consider adding a high-interaction UI (ala attachment 677427 [details]) _only_ in abnormal cases? E.G. start with no change from today, but if the user is crashing frequently (X times in Y minutes) then show a popup asking for them to help us help them? That would go a long way towards making it not annoying, and even outright helpful.

Comment 33

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

(In reply to Justin Dolske [:Dolske] from comment #32)
> I'm not quite able to parse that. I meant that option as a WONTFIX of this
> bug, but I think you're reading it as taking this patch (ie, do nothing
> about UI problems)?

Oh, yes. I think the value we'd get from these comments outweighs the "annoyingness" of them on page where there are many small-ish plugins.

Comment 34

[Drew Willcoxon :adw]

Attachment: patch

This addresses review comments and hides the comment box, URL checkbox, submit button, and help button when the embed's too small.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #25)
> @@ +38,5 @@
> > +            getService(Components.interfaces.nsIEnvironment);
> > +  let noReport = env.get("MOZ_CRASHREPORTER_NO_REPORT");
> > +  let serverURL = env.get("MOZ_CRASHREPORTER_URL");
> > +  env.set("MOZ_CRASHREPORTER_NO_REPORT", "");
> > +  env.set("MOZ_CRASHREPORTER_URL", SERVER_URL);
> 
> You can also just set nsICrashReporter.serverURL here.

Good to know, but it was easier to keep using the environment variable since at this point I've already gotten the nsIEnvironment component for the other thing, and to use nsICrashReporter, I'd have to get the nsICrashReporter component, and I'd have to wrap the URL in an nsIURL.

(In reply to Justin Dolske [:Dolske] from comment #26)
> @@ +269,5 @@
> > +        keyVals.PluginUserComment = userComment;
> > +      if (elt("submitURLOptIn").checked)
> > +        keyVals.PluginContentURL = doc.URL;
> > +    }
> > +    this.CrashSubmit.submit(pluginDumpID, { extraExtraKeyVals: keyVals });
> 
> Rather than adding extraExtraKeyVals (extra! extra! read all about it! ;),
> let's just leave this a flat object, i.e. pass |{ comment: text, url: blah}|.

The signature of CrashSubmit.submit is function CrashSubmit_submit(id, params), where params is an object that can take various meaningful properties.  Mingling extra extra properties among the other properties would make it unnecessarily hard to pluck them out.  It doesn't make sense to ungroup them.

Comment 35

[Justin Dolske [:Dolske]]

Apologies for taking so long to get back to this. :(

I just finished taking this patch for a spin on a bunch of random sites, killing plugin-container as I went along. I think the addition of the isTooSmall() progressive-fallback makes this UI acceptable. Ads, in particular, were almost always small enough that the comment field was suppressed (which seems desirable).

I'd also been a bit concerned about having this UI show up in random places in a page, and touch on the issue of "don't train users to input stuff for the browser in an (insecure, maybe malicious) web page". I think we're ok here too, especially due to the "comments are publicly visible" bit.

Comment 36

[Justin Dolske [:Dolske]]

Comment on attachment 694180 [details] [diff] [review]
patch

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

Patch had only some trivial bitrot, so this should be easy to get landed. Just a few modest review comments. I'll promise a faster review for the revised (and hopefully final) patch. ;-)

One general issue:

Someone needs to run this through a privacy review (or has it been?). I don't see anything to be concerned about, just want to make sure they've have a chance to look and are not surprised. Well, I suppose one new quirk is that Firefox Health Report has now landed... And so it might make sense to have FHR's policy stuff track the "send URL" pref. Might even be able to get rid of it from in-content? Ask mconnor/gps.

::: browser/base/content/browser-plugins.js
@@ +56,5 @@
>  var gPluginHandler = {
>  
> +  getPluginUI: function (plugin, className) {
> +    return plugin.ownerDocument.getAnonymousElementByAttribute(plugin, "class",
> +                                                               className);

Nit: I'd either ignore the line length, or break it right after .ownerDocument. |className| is so lonely out there on the line by itself...

@@ +906,5 @@
>        status = "please";
> +      this.getPluginUI(plugin, "submitButton").addEventListener(
> +        "click",
> +        this.submitReport.bind(this, pluginDumpID, browserDumpID, plugin),
> +        false

The false is optional now.

@@ +913,5 @@
> +      let pref = Services.prefs.getBranch("dom.ipc.plugins.reportCrashURL");
> +      optInCB.checked = pref.getBoolPref("");
> +      optInCB.addEventListener("click", function (event) {
> +        pref.setBoolPref("", optInCB.checked);
> +      }, false);

Since the checkboxes don't observe pref changes, there'd be a bit less potential for oddity by having submit() handle the pref setting (ie, pref becomes the value of the checkbox in the UI you're submitting, not whatever checkbox you happened to click last).

@@ +977,5 @@
> +        let submitDiv =
> +            doc.getAnonymousElementByAttribute(plugin, "class",
> +                                               "msg msgPleaseSubmit");
> +        submitDiv.style.display = "none";
> +        statusDiv.removeAttribute("status");

Why this? Seems like the status attribute shouldn't be changing.

@@ +987,5 @@
> +          // 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(pluginDumpID, browserDumpID);
> +        }

The |else| clause below ("If a previous plugin on the page was too small...") should be handled here.

That is, if isTooSmall() == true the 1st time, but not after removing the comment box, then remove any notification bar.

Might be simplest to refactor a bit here, so that the existing "else" becomes a "if (isShowingInConent) { ... }", so that all cases can hit it.

::: modules/libpref/src/init/all.js
@@ +1743,5 @@
>  // conflicts with our implementation, at least on Windows).
>  pref("dom.ipc.plugins.java.enabled", false);
>  
>  pref("dom.ipc.plugins.flash.subprocess.crashreporter.enabled", true);
> +pref("dom.ipc.plugins.reportCrashURL", true);

Err, is there already code that's using this pref? I don't see anything actually making use of it in this patch...

::: toolkit/crashreporter/CrashSubmit.jsm
@@ +145,5 @@
>  }
>  
>  // the Submitter class represents an individual submission.
> +function Submitter(id, submitSuccess, submitError, noThrottle,
> +                   extraExtraKeyVals) {

Ugh, I still dislike having extraExtraKeyVals around, but the way this code passes stuff around is already kind of a mess.

I think ted already reviewed this, so I'm just going to close my eyes and ignore the whole .jsm. :)

::: toolkit/mozapps/plugins/content/pluginProblemContent.css
@@ +100,5 @@
>  
> +.submitStatus[status] {
> +  display: -moz-box;
> +  -moz-box-align: center;
> +  -moz-box-pack: center;

Use some Flexbox hotness instead?

https://developer.mozilla.org/en-US/docs/CSS/Using_CSS_flexible_boxes

@@ +121,5 @@
> +  border: none;
> +  border-radius: 5px;
> +  resize: none;
> +  font-family: inherit;
> +  font-size: inherit;

Why the inherits?

This whole rule should go into pinstripe+winstripe. (ie, not here in pluginProblemContent.css)

@@ +143,5 @@
> +}
> +
> +.mainBox[chromedir="rtl"] .submitButton {
> +  float: left;
> +}

These additions should go into pinstripe+winstripe too.

::: toolkit/themes/pinstripe/mozapps/plugins/pluginProblem.css
@@ +89,5 @@
> +  margin-right: -1px;
> +}
> +
> +.submitButtonBox {
> +  margin-top: 7px;

Add these to the winstripe .css too? I think they're currently both identical...

Comment 37

[Drew Willcoxon :adw]

Attachment: patch

(In reply to Justin Dolske [:Dolske] from comment #36)
> @@ +977,5 @@
> > +        let submitDiv =
> > +            doc.getAnonymousElementByAttribute(plugin, "class",
> > +                                               "msg msgPleaseSubmit");
> > +        submitDiv.style.display = "none";
> > +        statusDiv.removeAttribute("status");
> 
> Why this? Seems like the status attribute shouldn't be changing.

It causes the submitStatus div to be hidden, which is what should happen in this case.

> ::: modules/libpref/src/init/all.js
> @@ +1743,5 @@
> >  // conflicts with our implementation, at least on Windows).
> >  pref("dom.ipc.plugins.java.enabled", false);
> >  
> >  pref("dom.ipc.plugins.flash.subprocess.crashreporter.enabled", true);
> > +pref("dom.ipc.plugins.reportCrashURL", true);
> 
> Err, is there already code that's using this pref? I don't see anything
> actually making use of it in this patch...

The URL opt-in checkbox sets its initial checked state based on that pref.

> ::: toolkit/mozapps/plugins/content/pluginProblemContent.css
> @@ +100,5 @@
> >  
> > +.submitStatus[status] {
> > +  display: -moz-box;
> > +  -moz-box-align: center;
> > +  -moz-box-pack: center;
> 
> Use some Flexbox hotness instead?

That's what I tried at first, but I couldn't get it to work, and MDN says flexbox is hidden behind a pref anyway.

> @@ +121,5 @@
> > +  border: none;
> > +  border-radius: 5px;
> > +  resize: none;
> > +  font-family: inherit;
> > +  font-size: inherit;
> 
> Why the inherits?

The textbox uses the usual textbox font (Courier or whatever it is) otherwise.

> @@ +143,5 @@
> > +}
> > +
> > +.mainBox[chromedir="rtl"] .submitButton {
> > +  float: left;
> > +}
> 
> These additions should go into pinstripe+winstripe too.

But these define layout of the controls, not their superficial, platform-specific visual styling.

Comment 38

[Justin Dolske [:Dolske]]

Comment on attachment 709894 [details] [diff] [review]
patch

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

Home stretch. Woo!

::: browser/base/content/browser-plugins.js
@@ +922,5 @@
>      else { // doPrompt
>        status = "please";
> +      this.getPluginUI(plugin, "submitButton").addEventListener("click",
> +        function () {
> +          this.submitReport(pluginDumpID, browserDumpID, plugin);

Oh, you'll also want to add a check for (aEvent.button == 0 && aEvent.isTrusted). I missed this in earlier review passes, sorry.

@@ +986,5 @@
>      let notificationBox = gBrowser.getNotificationBox(browser);
>  
>      // Is the <object>'s size too small to hold what we want to show?
>      if (this.isTooSmall(plugin, overlay)) {
> +      // First try hiding the comment box and related report submission UI.

This still isn't quite right. The purpose here is is make sure we either:

A) Hide all UI here, and instead show a notification bar (unless mozNoPluginCrashedNotification is set)

B) Show some/all UI here, and remove any notification bar shown by another plugin instance that was too small.

In other words, the user should see a notification bar only if there was no plugin instance on the page big enough to use in-content UI.


So, this should be something like:

  let isShowing = true;

  if (isTooSmall) {
    // hide some, try again
    if (isTooSmall) {
      // hide all the things
      isShowing = false
    }
  }

  if (isShowing) {
    hideNotificationBar();
    doc.mozNoPluginCrashedNotification = true;
  } else {
    if (!doc.mozNoPluginCrashedNotification)
      showNotificationBar();
  }

::: toolkit/mozapps/plugins/content/pluginProblemContent.css
@@ +106,5 @@
>  
> +.submitStatus[status] {
> +  display: -moz-box;
> +  -moz-box-align: center;
> +  -moz-box-pack: center;

>> Use some Flexbox hotness instead?
> That's what I tried at first, but I couldn't get it to work,
> and MDN says flexbox is hidden behind a pref anyway.

It's on now (for the 21 train), and FHR has started using it.

No big deal here, though.

@@ +138,5 @@
> +}
> +
> +.mainBox[chromedir="rtl"] .submitButton {
> +  float: left;
> +}

>> These additions should go into pinstripe+winstripe too.
> But these define layout of the controls, not their superficial,
> platform-specific visual styling.

The existing usage here (and in many places) is for /content stuff to control functional aspects (like attributes driving message display), and the /themes stuff to control visual design aspects (be they superficial or not, platform-specific or not).

MattN is starting some work that will let us stop duplicating style that's shared across themes, but for now I want to continue with the current clear split.

::: toolkit/themes/pinstripe/mozapps/plugins/pluginProblem.css
@@ +88,5 @@
> +  border: none;
> +  border-radius: 5px;
> +  resize: none;
> +  font-family: inherit;
> +  font-size: inherit;

>> Why the inherits?
> The textbox uses the usual textbox font otherwise.

Ah, ok.

But I'm nervous about this leading to the textbox picking up the page's font; it won't right now but is fragile in that it subtly relies on the textbox being in a <div class=msg>.

So either cut'n'paste that rule so the font is explicitly set for the textbox here (instead of |inherit|), or hoist up the .msg font rules to .mainBox, so I can sleep soundly knowing the page's font won't leak in here. :)

Comment 39

[Drew Willcoxon :adw]

Attachment: patch

Comment 40

[Justin Dolske [:Dolske]]

Comment on attachment 710327 [details] [diff] [review]
patch

Bam. Thanks!

Comment 41

[Justin Dolske [:Dolske]]

Landing ping? There's other plugin UI changes that will bitrot you soon (if not already).

Comment 42

[Drew Willcoxon :adw]

I'm trying to figure out why a Windows test is failing.

Comment 43

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/mozilla-inbound/rev/528411b6f628
https://tbpl.mozilla.org/?tree=Try&rev=a3bd594806dd

The failing test was test_idle_hang.xul.  It couldn't find the crash minidump and extra files, which were supposed to exist.  I finally realized I ought to look at the several plugin crash tests that run before it, and it turns out that two of them were silently failing because they were clicking the crash-submit link, which my patch replaced with a new button.  (Actually they were TEST-KNOWN-FAIL'ing for some reason with a null pointer exception originating in specialpowersAPI.js.)  Their failure to complete left the plugin crash directory in an unexpected state.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/528411b6f628