886794 - Implement the new crash reporter prompt ui

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Browser, defect)

Comment 1

[Jim Mathies [:jimm]]

Attachment: use the same class for all chrome text links

Comment 2

[Jonathan Wilde [:jwilde]]

Comment on attachment 767179 [details] [diff] [review]
use the same class for all chrome text links

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

Overall, sounds like a good change to me.

::: browser/metro/theme/platform.css
@@ +16,5 @@
>    font-size: @font_normal@;
>  }
>  
>  .text-link {
> +  color: #f08000;

Would it be better to use #ff8000 (@metro_orange@) to be consistent with the orange used elsewhere? Or were there legibility issues with that?

Comment 3

[Jim Mathies [:jimm]]

Attachment: remove about:crash resources

Comment 4

[Jim Mathies [:jimm]]

Attachment: crash reporting prompt v.1

Comment 5

[Jim Mathies [:jimm]]

Attachment: tests v.1

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 767773 [details] [diff] [review]
remove about:crash resources

This removes the old about:crash page.

Comment 7

[Jim Mathies [:jimm]]

Attachment: crash reporting prompt v.1

This adds the new prompt and logic.

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 767777 [details] [diff] [review]
crash reporting prompt v.1

argh, wrong patch.

Comment 9

[Jim Mathies [:jimm]]

Attachment: crash reporting prompt v.1

Comment 10

[Jim Mathies [:jimm]]

Attachment: tests

Comment 11

[Frank Yan (:fryn)]

How do I test this UI?

Comment 12

[Jim Mathies [:jimm]]

(In reply to Frank Yan (:fryn) from comment #11)
> How do I test this UI?

Two things to do:

1) plop this line at the top of startupCrashCheck -

DialogUI.importModal(document, "chrome://browser/content/prompt/crash.xul");
return;

2) comment out the MOZILLA_OFFICIAL ifdef here so Enabled=1 stays around.

http://mxr.mozilla.org/mozilla-central/source/browser/metro/metroapp.ini.in#19

build browser/metro, it should open on startup.

Comment 13

[Jim Mathies [:jimm]]

Actually you don't need that second step if you put that import statement right at the top of startupCrashCheck. You'll need step two if you want to crash the app somehow and test the prompt / pref logic.

Comment 14

[Frank Yan (:fryn)]

Comment on attachment 767780 [details] [diff] [review]
crash reporting prompt v.1

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

For the privacy statement: using the app bar back button's icon instead of the stop button icon would be fine, in my opinion.

::: browser/metro/base/content/browser-ui.js
@@ +1921,5 @@
> +    let back = document.getElementById("dialog-modal-block");
> +    if (!back) {
> +      back = document.createElement("box");
> +    } else {
> +      while(back.hasChildNodes()) {

Nit: add space between `while` and `(`.

Comment 15

[Jim Mathies [:jimm]]

Attachment: tests

minor update, the dialog getter was retrieving the wrong element due to a copy paste error.

Comment 16

[Jim Mathies [:jimm]]

Attachment: tests

while I'm in there, removed the example test link at the top of the test file bbondy pointed me to.

Comment 17

[Jim Mathies [:jimm]]

If you're interested in running these in a local build, add 

Enabled=1

to the crash reporter section of the app ini file @

obj/dist/bin/metro/metroapp.ini

Comment 18

[Jonathan Wilde [:jwilde]]

Comment on attachment 769393 [details] [diff] [review]
tests

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

Issue: if browser_crashprompt.js is run individually, there will be an exception thrown during shutdown from Downloads.uninit() because Downloads.init() does not get called during the time that the test is run.

As jimm discussed in IRC, uninit should return without attempting to call removeObserver if init has not been called yet.

::: browser/metro/base/tests/mochitest/browser_crashprompt.js
@@ +86,5 @@
> +    this.promptedPref = false;
> +
> +    BrowserUI.startupCrashCheck();
> +
> +    yield waitForMs(100);

Can we comment that these waits are for the time taken to set up the dialog? (Just so that future devs know what code depends on these?)

I don't have a problem with using time-based waits here rather than waitForCondition. It seems to me that the point of the ok(...) statements is to check that the dialog is set up and is shown in a reasonable period of time. If we did a waitForCondition on dialog setup, we'd get test timeouts rather than test failures, which seems less desirable.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b88cbf916c82
https://hg.mozilla.org/mozilla-central/rev/73923e44fb3f
https://hg.mozilla.org/mozilla-central/rev/c4b623117894
https://hg.mozilla.org/mozilla-central/rev/eb1f30cd3b3d

Comment 20

[Francesco Lodolo [:flod]]

Hi, some questions about the strings added in this bug.

crashprompt.dtd
Is using hard-coded "Firefox" and "Mozilla" OK?

crashprompt.properties (crashprompt.messagebody)
Localization comment is clearly wrong since there are two variables (and one I guess it's VendorName). Also please use ordered variables in these cases (%1$S, %2$S, etc.), they're less confusing.

I can open a bug for the last part, just wanted to be sure if the first part is a bug or not.

Comment 21

[Jim Mathies [:jimm]]

(In reply to Francesco Lodolo [:flod] from comment #20)
> Hi, some questions about the strings added in this bug.
> 
> crashprompt.dtd
> Is using hard-coded "Firefox" and "Mozilla" OK?

Yes, that long text message doesn't need to be tied to the release channel.

> crashprompt.properties (crashprompt.messagebody)
> Localization comment is clearly wrong since there are two variables (and one
> I guess it's VendorName). Also please use ordered variables in these cases
> (%1$S, %2$S, etc.), they're less confusing.
> 
> I can open a bug for the last part, just wanted to be sure if the first part
> is a bug or not.

Sure, sorry about that. cc me, I'll touch up the comment.