1767367 - Fix inverted useDownloadDir pref in about:preferences

Status: VERIFIED FIXED

(Firefox :: Preferences, defect, P1)

Description

[Shane Hughes [:aminomancer]]

I forgot to replace value with inverted in bug 1762775. This is just to fix that.

Comment 1

[Shane Hughes [:aminomancer]]

Attachment: Bug 1767367 - Fix inverted useDownloadDir pref. r=dao

I forgot to add a property in bug bug 1762775.
This patch just resolves that.

Comment 2

[Dão Gottwald [::dao]]

How does this manifest in practice? Is the pref actually backwards from the user's perspective? I suppose we will need to uplift this?

Comment 3

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #2)

How does this manifest in practice? Is the pref actually backwards from the user's perspective?

Yes

I suppose we will need to uplift this?

Yes

Comment 4

[:Gijs (he/him)]

Comment on attachment 9274749 [details]
Bug 1767367 - Fix inverted useDownloadDir pref. r=dao

Beta/Release Uplift Approval Request

• User impact if declined: Wrong preference state in user-visible preferences
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Check that the "Always ask you where to save files" checkbox behaves as expected in the Firefox settings
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Trivial equivalent of a missing ! operator addition in markup + JS
• String changes made/needed: No
• Is Android affected?: No

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0946689c7ef
Fix inverted useDownloadDir pref. r=dao,preferences-reviewers,Gijs

Comment 6

[Shane Hughes [:aminomancer]]

Yeah it's just reversed. Thanks and sorry I overlooked this 😳

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Is this something we can add a test for?

Comment 8

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)

Is this something we can add a test for?

I mean, as I noted on phabricator, we don't normally test that a checkbox pref "does what it says on the tin" - the implementation is shared amongst all checkbox, and really the test can't check that the prose English label of the checkbox maps to the "mental model" of the boolean pref "for real". We could add a test specific to this checkbox but it's unlikely to be updated in future, and it'd be just as likely that the entire UI changes and/or the wording gets changed (in which case, the test might pass even if we want to flip the meaning of the checkbox, if we forget to do the implementation part of it). So tbh I don't think this is useful.

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/f0946689c7ef

Comment 10

[Arthur K. [He/Him]]

Would this be a good candidate for a 100.1 release should one come out?

Comment 11

[Ryan VanderMeulen [:RyanVM]]

If the regressing bug was in fact bug 1762775, this wouldn't affect 100. If you're saying that 100 is actually affected, then it sounds like we need to revisit the regressing bug.

Comment 12

[Shane Hughes [:aminomancer]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)

If the regressing bug was in fact bug 1762775, this wouldn't affect 100.

Yeah it is definitely bug 1762775 — prior to that, the value attribute did basically the same thing since it was a radiogroup, so it was semantically correct. I know for sure it was my mistake, I was worried about random testing stuff instead of actually reading the text lol.

Comment 13

[Arthur K. [He/Him]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)

If the regressing bug was in fact bug 1762775, this wouldn't affect 100. If you're saying that 100 is actually affected, then it sounds like we need to revisit the regressing bug.

Ugh, my mistake. I didn't see from comment 9 that 100 is not affected. Sorry for the noise.

Comment 14

[Release mgmt bot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1762775

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9274749 [details]
Bug 1767367 - Fix inverted useDownloadDir pref. r=dao

Approved for 101.0b2.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b2be4dacd884

Comment 17

[Timea Zsoldos [:zstimi/tzsoldos], Desktop Release QA]

I have reproduced this issue using Firefox 101.0b1 on Win 10 x64.
I can confirm this issue is fixed, I verified using Firefox 101.0b2 and Firefox 102.0a1 on Win 10 x64, Ubuntu 20.04 x64 and macOS 10.15.