947078 - Make the "Include URLs" option for the crash reporter into a checkbox instead of a toggle

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Flyouts, defect, P2)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

In bug 903426, we added the ability to strip URLs in crash reports. To match the mockup, the option in our preferences flyout labeled "Include the address of the page I was on" should be a checkbox that grays out if crash reporting is disabled. It is currently a <settings> element.

Comment 1

[Allison Naaktgeboren :ally]

yoink! looks like a good bug to take out any holiday frustration on.

Comment 2

[Marco Mucci [:MarcoM]]

Hey Ally, can you provide a point value.

Comment 3

[Allison Naaktgeboren :ally]

points = 1 or 2

Comment 4

[Allison Naaktgeboren :ally]

Attachment: crashurlscheckbox

flagged for feedback because the about:config value for autoSubmit doesn't seem to update in about:config but submitURLs behaves as expected. Tim, thoughts?

Comment 5

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 8348428 [details] [diff] [review]
crashurlscheckbox

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

::: browser/metro/base/content/browser.xul
@@ +677,5 @@
>        <settings id="prefs-reporting" label="&optionsHeader.reporting.title;">
>          <setting pref="app.crashreporter.autosubmit"
>                   type="bool"
>                   title="&optionsHeader.reporting.crashes.label;" />
> +        <checkbox pref="app.crashreporter.submitURLs"

"pref" has meaning for the <setting> element but not (I think) for the <checkbox> element. I'm pretty sure that's why you're seeing that the "submitURLs" doesn't get updated. I think you'll have to add logic to update the pref whenever this checkbox gets checked/unchecked.

Comment 6

[Allison Naaktgeboren :ally]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #5)
> Comment on attachment 8348428 [details] [diff] [review]
> crashurlscheckbox
> 
> Review of attachment 8348428 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/base/content/browser.xul
> @@ +677,5 @@
> >        <settings id="prefs-reporting" label="&optionsHeader.reporting.title;">
> >          <setting pref="app.crashreporter.autosubmit"
> >                   type="bool"
> >                   title="&optionsHeader.reporting.crashes.label;" />
> > +        <checkbox pref="app.crashreporter.submitURLs"
> 
> "pref" has meaning for the <setting> element but not (I think) for the
> <checkbox> element. I'm pretty sure that's why you're seeing that the
> "submitURLs" doesn't get updated. I think you'll have to add logic to update
> the pref whenever this checkbox gets checked/unchecked.

You've got them backwards. :) submitURLs is working/updating(the submit urls to crash reporter pref) but not autoSubmit (the general crash reporter pref) which was not altered from its <settings> or even touched in this patch, which is why I am seeking insight. :)

Comment 7

[Matt Brubeck (:mbrubeck)]

(In reply to :Ally Naaktgeboren from comment #6)
> You've got them backwards. :) submitURLs is working/updating(the submit urls
> to crash reporter pref) but not autoSubmit (the general crash reporter pref)
> which was not altered from its <settings> or even touched in this patch,
> which is why I am seeking insight. :)

In a build with this patch, I'm seeing what Tim was seeing -- the toggle switch correctly sets the autosubmit pref, but the checkbox has no effect.

Comment 8

[Allison Naaktgeboren :ally]

talked to mbrubeck about it. He is seeing the opposite behavior of what I am seeing. The plot thickens.

Comment 9

[Allison Naaktgeboren :ally]

Attachment: checkbox_crashpref

Comment 10

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8360095 [details] [diff] [review]
checkbox_crashpref

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

::: browser/metro/base/content/flyoutpanels/PrefsFlyoutPanel.js
@@ +39,5 @@
>        SanitizeUI.init();
>        this._hasShown = true;
>  
>        Services.prefs.addObserver("privacy.donottrackheader.value",
> +                                 this,

Thanks for doing this cleanup!  Hopefully it'll prevent future confusion in this code.

Comment 11

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/integration/fx-team/rev/56c79adf3a3d

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/56c79adf3a3d

Comment 13

[Allison Naaktgeboren :ally]

re-opening so I remember to get this uplifted to aurora.

Comment 14

[Allison Naaktgeboren :ally]

Breaks Marco's workflow, so I guess I'll close it again. :) Sorry for the bugspam!

Comment 15

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/486101f1073a

Comment 16

[Cornel Ionce [:noni] [Hubs QA]]

User Agents:
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0

Verified as fixed on latest Aurora (build ID: 20140123004002) and on latest Nightly (build ID: 20140122030521).
"Include URLs" option is now a checkbox which becomes grayed out if crash report is disabled.