Rename browser.safebrowsing.enabled to browser.safebrowsing.phishing.enabled

VERIFIED FIXED in Firefox 50

Status

()

defect
P5
normal
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: emorley, Assigned: tnguyen)

Tracking

Trunk
mozilla50
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox50 verified)

Details

Attachments

(2 attachments, 3 obsolete attachments)

In bug 1025358, the reporter believed that setting the pref browser.safebrowsing.enabled to false would disable safebrowsing entirely, whereas in fact it only controls the safebrowsing phishing protection, and browser.safebrowsing.malware.enabled needed to be set to false too.

To avoid this confusion, I suggest we rename browser.safebrowsing.enabled to browser.safebrowsing.phishing.enabled, otherwise it implies that browser.safebrowsing.enabled is a superset of browser.safebrowsing.malware.enabled.

If we do this, we will need to migrate the pref state from old profiles.
I think the problem is too many prefs. If someone doesn't trust safebrowsing, there's no reason to enable it only for phishing and not malware (or vice versa). In fact, only ~1% of users touch these prefs at all (http://monica-at-mozilla.blogspot.com/2013/02/writing-for-98.html).

It would make more sense to consolidate into a single pref, browser.safebrowsing.enabled, and make it easier for people who don't want it to turn it off.
That sgtm as well :-)
I agree, but I do think you want a separate pref for the new download protection. (I suggested making this change in the real UX instead of about:config and you rebuffed me :-)
(In reply to Gian-Carlo Pascutto [:gcp] from comment #3)
> I agree, but I do think you want a separate pref for the new download
> protection. (I suggested making this change in the real UX instead of
> about:config and you rebuffed me :-)

Sorry, I still disagree :) Having yet another pref sounds even more unusable. If you don't trust Google, you shouldn't use their services. Besides, if someone really wants to turn off the download protection then a pref already exists (the apprepURL one) to turn it off.
Thomas, is this something you'd like to take a look at?

I suggest keeping the change to a minimum and simply renaming browser.safebrowsing.enabled to browser.safebrowsing.phishing.enabled. This will be a rather large change because we need to update all of the tests that rely on this. dxr.mozilla.org is your friend :)
Priority: -- → P5
Depends on: 1033450
Sure, I will take it
Assignee: nobody → tnguyen
When you rename a preference just for naming clarity, you also need to add code to nsBrowserGlue.js to migrate the previous value. This ensures that users who had previously disabled the preference don't find it enabled unexpectedly. Example:

http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js?rev=a07ea7fb572f#2090

Firefox for Android may need migration code as well, but I'm not sure how that is handled.
Posted patch Part 2 : migrate pref (obsolete) — Splinter Review
Attachment #8750233 - Flags: review?(francois)
Attachment #8750235 - Flags: review?(francois)
Comment on attachment 8750235 [details] [diff] [review]
Part 2 : migrate pref

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

That looks good to me, but I'll let Paolo have a quick look too since I didn't know about BrowserGlue before he mentioned it.

::: browser/components/nsBrowserGlue.js
@@ +2198,5 @@
>      }
>  
> +    if (currentUIVersion < 38) {
> +      const kOldSafeBrowsingPref = "browser.safebrowsing.enabled";
> +      // Default value is set to true, a has user pref means that the pref was

typo: the word "has" should probably not be there

::: mobile/android/chrome/content/browser.js
@@ +1030,5 @@
>      }
>  
> +    if (currentUIVersion < 3) {
> +      const kOldSafeBrowsingPref = "browser.safebrowsing.enabled";
> +      // Default value is set to true, a has user pref means that the pref was

same typo with the word "has"
Attachment #8750235 - Flags: review?(francois) → review?(paolo.mozmail)
Comment on attachment 8750233 [details] [diff] [review]
Part 1 : Rename pref

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

Looks good to me.

In terms of testing, you can run:

  mach test toolkit/components/url-classifier
  mach test browser/components/safe-browsing

and then all of the browser/base/content/test/general/browser_trackingUI_* tests too, just to make sure.

I would also do a try run on Windows, Mac, Linux and Android to make sure everything is fine there too.
Attachment #8750233 - Flags: review?(francois) → review+
Thanks Francois.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f803013d8b5&selectedJob=20399507
Updated part 2 because there's a failure on try on Android 4.3
Posted patch Part 2 : migrate pref (obsolete) — Splinter Review
Attachment #8750235 - Attachment is obsolete: true
Attachment #8750235 - Flags: review?(paolo.mozmail)
Attachment #8751061 - Flags: review?(paolo.mozmail)
Posted patch Part 2 : migrate pref (obsolete) — Splinter Review
Attachment #8751061 - Attachment is obsolete: true
Attachment #8751061 - Flags: review?(paolo.mozmail)
Attachment #8751062 - Flags: review?(paolo.mozmail)
Comment on attachment 8751062 [details] [diff] [review]
Part 2 : migrate pref

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

The code looks good to me, make sure you also test the migration manually.
Attachment #8751062 - Flags: review?(paolo.mozmail) → review+
I've marked this bug for front-end QA as well, here is an overview of what to test:

1. Create a clean profile with a previous version of the browser
2. Switch the phishing preference from the Preferences window
3. Update to the new version and check the preference remains the same
4. Check that there are no errors from nsBrowserGlue.js in the Browser Console.
5. Repeat the above, but skip step 2.
Flags: qe-verify+
QA Contact: mwobensmith
Posted patch migrate prefSplinter Review
Rebase and carry r+
Attachment #8751062 - Attachment is obsolete: true
Attachment #8764502 - Flags: review+
Flags: needinfo?(paolo.mozmail)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/4284721d82dd
Rename browser.safebrowsing.enabled to browser.safebrowsing.phishing.enabled. r=francios
https://hg.mozilla.org/integration/fx-team/rev/13aad7c0cfda
Migrate browser.safebrowsing.enabled to browser.safebrowsing.phishing.enabled. r=paolo.mozmail
Keywords: checkin-needed
Blocks: 1282506
https://hg.mozilla.org/mozilla-central/rev/4284721d82dd
https://hg.mozilla.org/mozilla-central/rev/13aad7c0cfda
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Verified fixed on Nightly 50.0a1, 2016-07-12.

I checked preferences via about:config, plus used the test in comment 16 (thanks).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.