1359289 - Add "Learn more" link next to Safe Browsing options in about:preferences

Status: VERIFIED FIXED

(Firefox :: Settings UI, enhancement, P3)

Description

[François Marier [:francois]]

The Safe Browsing options in about:preferences don't link to SUMO like the other security preferences.

We should add  "Learn more" link pointing to https://support.mozilla.org/kb/how-does-phishing-and-malware-protection-work

Comment 1

[Ethan Tseng [:ethan]]

Where is the Safe Browsing option in about:preferences page?
I couldn't find it in Privacy or Security settings.

Comment 2

[François Marier [:francois]]

Attachment: safebrowsing-options.png

In the Privacy & Security section, then scroll down to security.

Comment 3

[Ethan Tseng [:ethan]]

(In reply to François Marier [:francois] from comment #2)
> In the Privacy & Security section, then scroll down to security.

Beta and Nightly have different layout for preference pages, which confused me a bit.
Thank you for making it clear for me!

Comment 4

[bitout.cecelia]

Hi , I would like to work on this bug . But this is my first project and I can not find the files to remove this bug. So if you can please provide some more information on this?

Comment 5

[François Marier [:francois]]

(In reply to bitout.cecelia from comment #4)
> Hi , I would like to work on this bug . But this is my first project and I
> can not find the files to remove this bug. So if you can please provide some
> more information on this?

I'm not an expert on the preference pages, but it looks like it's defined here: https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/privacy.xul

Comment 6

[Johann Hofmann [:johannh]]

Sorry, I missed this bug in my bugmail. Preferences is a bit more complex right now because there are two versions that we maintain (in-content and in-content-new), for testing purposes.

The places where you'd need to add a learn more link are here:

https://searchfox.org/mozilla-central/search?q=checkbox+id%3D%22enableSafeBrowsing%22&path=

This is how tracking protection does its learn more link:

https://searchfox.org/mozilla-central/search?q=trackingProtectionPBMLearnMore&path=

we should probably just copy that model for safe browsing.

Comment 7

[Hossain Al Ikram [:ikram] (QA Contact)]

I am not sure, If someone is working on this (Confused with Comment 4). If not, I would like to work on this and submit a patch.

Comment 8

[Johann Hofmann [:johannh]]

I think it's safe to assume no one is working on this. See comment 6 for info on how to fix this. If you need any help, let me know here or via IRC :)

Thanks!

Comment 9

[Johann Hofmann [:johannh]]

Hi ikram, are you still working on this? Let me know if you need any more help!

Thanks!

Comment 10

[Johann Hofmann [:johannh]]

Unassigning for now. Let me know if you'd still like to work on this! :)

Comment 11

[Johann Hofmann [:johannh]]

Note that we removed the old preferences now so

> Preferences is a bit more complex right now because there are two versions that we maintain (in-content and in-content-new), for testing purposes.

from comment 6 is no longer valid. You'll only have to make the change once. The rest of that comment should still be helpful for solving this.

Comment 12

[Vedant Chakravadhanula(:vedantc98)]

I'm confused as to where I should put the corresponding Javascript function to activate the Learn More link, can you help me out with that?

Comment 13

[Vedant Chakravadhanula(:vedantc98)]

Attachment: bug_1359289.patch, temporary

I've made a functional fix, but I'm not sure as to where exactly the Javascript additions should go, so I've put them in the _initSafeBrowsing function.
Also, I'm not exactly sure on how to build the url, so I've hard-coded it for now.
Please advise on further changes.

Thank you. :)

Comment 14

[Johann Hofmann [:johannh]]

Comment on attachment 8906015 [details] [diff] [review]
bug_1359289.patch, temporary

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

I think placing it there works for me, but please see my comment on how to build the URL. Thanks!

::: browser/components/preferences/in-content/privacy.js
@@ +1081,5 @@
>      let blockUnwantedPref = document.getElementById("browser.safebrowsing.downloads.remote.block_potentially_unwanted");
>      let blockUncommonPref = document.getElementById("browser.safebrowsing.downloads.remote.block_uncommon");
>  
> +    let learnMoreLink = document.getElementById("enableSafeBrowsingLearnMore");
> +    let phishingUrl = "https://support.mozilla.org/kb/how-does-phishing-and-malware-protection-work";

We should avoid hardcoding the base URL, please do it like this instead:
https://searchfox.org/mozilla-central/rev/d29c832536839b60a9ee5b512eeb934274b879b0/browser/components/preferences/in-content/privacy.js#76

Comment 15

[Vedant Chakravadhanula(:vedantc98)]

Attachment: bug_1359289.patch

Added the URL fix, although on my machine that code redirects to a 404.

Comment 16

[Johann Hofmann [:johannh]]

(In reply to Vedant Chakravadhanula from comment #15)
> Created attachment 8906321 [details] [diff] [review]
> bug_1359289.patch
> 
> Added the URL fix, although on my machine that code redirects to a 404.

Ah, good catch. Joni, we need a new special link for this, right?

Comment 17

[Johann Hofmann [:johannh]]

Hi Vedant,

lacking a response from Joni, if you could just set the URL to "https://support.mozilla.org/kb/how-does-phishing-and-malware-protection-work" again (we can open a bug for changing it to the correct version later), I will r+ the patch.

Sorry for the delay.

Thank you!

Comment 18

[Vedant Chakravadhanula(:vedantc98)]

Attachment: bug_1359289.patch

I've done as you requested.
Thanks. :)

Comment 19

[Johann Hofmann [:johannh]]

Comment on attachment 8908996 [details] [diff] [review]
bug_1359289.patch

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

There's a merge conflict from bug 1394058, you'll need to run "hg pull central" and "hg rebase -d central" and resolve the merge conflict in privacy.xul.

Let me know if you need help with that.

Thank you!

Comment 20

[Vedant Chakravadhanula(:vedantc98)]

I'm having some trouble resolving the conflicts.
After pulling from central, I tried to import the patch locally, and mercurial generated a .rej file for privacy.xul as expected. I fixed the changes manually, and then tried to import the patch, but it still fails to import the patch, giving the same .rej file.

Thanks.

Comment 21

[Johann Hofmann [:johannh]]

(In reply to Vedant Chakravadhanula from comment #20)
> I'm having some trouble resolving the conflicts.
> After pulling from central, I tried to import the patch locally, and
> mercurial generated a .rej file for privacy.xul as expected. I fixed the
> changes manually, and then tried to import the patch, but it still fails to
> import the patch, giving the same .rej file.
> 
> Thanks.

Hm, did you commit again? Can you just upload the latest patch after fixing the conflicts? Feel free to ping me on IRC (johannh) and we can go through it together. Again, I'd recommend to use hg rebase -d central instead of importing. :)

Comment 22

[Vedant Chakravadhanula(:vedantc98)]

Attachment: bug_1359289.patch

I've just made the patch on top of the latest changes.

Hopefully this solves the issue. Thanks.

Comment 23

[Johann Hofmann [:johannh]]

Comment on attachment 8909650 [details] [diff] [review]
bug_1359289.patch

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

Looks great! I'm setting the checkin-needed flag to get this landed now.

In the future you can do this yourself, since you now have permission to edit bugs. Congrats!

If you want to keep contributing, you should also consider getting access to our try server (https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server), which allows you to run automated tests to make sure your commits are not breaking anything. I'd happily vouch for you.

Thank you!

Comment 24

[Johann Hofmann [:johannh]]

Filed bug 1401137 to track the SUMO link.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9016e255c36c
Add a "Learn More" link to Safe Browsing checkbox in about:preferences. r=jhofmann

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9016e255c36c

Comment 27

[roxleitan]

Verified fixed using the latest Nightly (2017-09-19) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.