Closed Bug 886794 Opened 7 years ago Closed 7 years ago

Implement the new crash reporter prompt ui

Categories

(Firefox for Metro Graveyard :: Browser, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: jimm, Assigned: jimm)

References

()

Details

Attachments

(4 files, 5 obsolete files)

No description provided.
Attachment #767179 - Flags: review?(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?
Attachment #767179 - Flags: review?(jwilde) → review+
Attached patch crash reporting prompt v.1 (obsolete) — Splinter Review
Attached patch tests v.1 (obsolete) — Splinter Review
Comment on attachment 767773 [details] [diff] [review]
remove about:crash resources

This removes the old about:crash page.
Attachment #767773 - Flags: review?(fyan)
Attached patch crash reporting prompt v.1 (obsolete) — Splinter Review
This adds the new prompt and logic.
Attachment #767774 - Attachment is obsolete: true
Attachment #767777 - Flags: review?(fyan)
Comment on attachment 767777 [details] [diff] [review]
crash reporting prompt v.1

argh, wrong patch.
Attachment #767777 - Flags: review?(fyan)
Attachment #767775 - Attachment is obsolete: true
Attachment #767777 - Attachment is obsolete: true
Attachment #767780 - Flags: review?(fyan)
Blocks: 887286
Attached patch tests (obsolete) — Splinter Review
Attachment #767860 - Flags: review?(fyan)
Attachment #767860 - Flags: review?(fyan) → review?(jwilde)
Attachment #767773 - Flags: review?(fyan) → review+
How do I test this UI?
(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.
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 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 `(`.
Attachment #767780 - Flags: review?(fyan) → review+
Attached patch tests (obsolete) — Splinter Review
minor update, the dialog getter was retrieving the wrong element due to a copy paste error.
Attachment #767860 - Attachment is obsolete: true
Attachment #767860 - Flags: review?(jwilde)
Attachment #769392 - Flags: review?(jwilde)
Attached patch testsSplinter Review
while I'm in there, removed the example test link at the top of the test file bbondy pointed me to.
Attachment #769392 - Attachment is obsolete: true
Attachment #769392 - Flags: review?(jwilde)
Attachment #769393 - Flags: review?(jwilde)
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
No longer blocks: 887286
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.
Attachment #769393 - Flags: review?(jwilde) → review+
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.
(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.
Blocks: 890529
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.