336273 - create preferences panel for managing safe browsing settings

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Mike Beltzner [:beltzner, not reading bugmail]]

For matters of expedience, this should probably go into a new Advanced > Safe Browsing tab for now. I've got a long-standing to-do to figure out how to better consolidate some of these advanced prefs, so I'll tackle that for B1.

 Safe Browsing ____________________________________

 When I'm browsing the internet

 [x] Check to see if the site might be a scam  (<- turns safe browsing on/off)
      (o) using a locally stored list          (<- basic mode)
      ( ) by asking [ Google |v]               (<- enhanced mode, data provider)


cc'ing tony for the real pref/attrib names

Comment 1

[Tony Chang (Google)]

Turning safe browsing on/off: safebrowsing.enabled
Local list vs remote: safebrowsing.remoteLookups

Picking provider hasn't been implemented yet, but the plan is to put it into safebrowsing.provider.0., safebrowsing.provider.1., etc.  It requires having multiple pref values set.

Comment 2

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

Basic safe browsing preferences with data provider management disabled for now.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 221245 [details] [diff] [review]
Patch

Get rid of the "Manage..." button, since we have no capability to add/remove these providers without an application update, and then you've got ui-r from me.

Gavin, can you do a quick review for coding style, etc?

Tony, these prefs should probably have a namespace prefix (probably browser.safeBrowsing.*) - this change should be made before this prefpanel is checked in, then put the new names here and Ryan can update this patch with them.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Also, as mentioned in today's Alpha 2 triage, please make sure that the default setting for Alpha 2 is browser.safebrowsing.enabled=false

Comment 5

[Tony Chang (Google)]

Ok, tracking on bug 336832.

Comment 6

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch v2

Comment 7

[:Gavin Sharp [email: [e-mail]]]

not fixed, from what I can tell!

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

I'm not sold on this approach yet. 

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

Let's go for a pure toggle for a2, circle back around here afterwards. 

Comment 10

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Toggle only

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 221382 [details] [diff] [review]
Toggle only

>Index: advanced.xul

>+          <checkbox id="safe-active"
>+                    class="indent"
>+                    label="&safeBrowsingEnable.label;"
>+                    accesskey="&safeBrowsingEnable.accesskey;"
>+                    pref="browser.safebrowsing.enabled"/>

s/pref/preference/ ;)

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 221382 [details] [diff] [review]
Toggle only

sr+a=[e-mail]

Comment 13

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Toggle only v2

I make that mistake all too often :/
Thanks Gavin!

Carrying r=gavin.sharp sr+a=bugs forward.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Checked in on the 1.8 branch. Leaving open for trunk checkin.

Comment 15

[u60234]

Comment on attachment 221398 [details] [diff] [review]
Toggle only v2

>Index: advanced.dtd
>===================================================================

>+<!ENTITY safeBrowsingEnable.label       "Check to see if the site I'm visiting might be a scam">
>+<!ENTITY safeBrowsingEnable.accesskey   "C">

This access key is already used for the "View Certificates" button on the Security tab. "t" or "o" could be used instead.

Comment 16

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Fix accesskey conflict and ifdef all changes

Comment 17

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Let's try that again

Final exams have melted my brain :/

Comment 18

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Preprocess advanced.dtd

Last time, I swear ;)

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 221529 [details] [diff] [review]
Preprocess advanced.dtd

Works fine, thanks!

Comment 20

[Steffen Wilberg]

Comment on attachment 221529 [details] [diff] [review]
Preprocess advanced.dtd

>Index: locales/en-US/chrome/browser/preferences/advanced.dtd
>+#ifdef MOZ_SAFE_BROWSING

I'm not sure if #ifdefing a .dtd file is a good idea, since it might break localizers' tools and has usually been avoided for that reason, see http://developer.mozilla.org/en/docs/Writing_localizable_code

Comment 21

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #20)
> I'm not sure if #ifdefing a .dtd file is a good idea

You`re right, and Axel also agrees. I'll remove the dtd ifdeffing and jar.mn change before checkin.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

removing the fixed1.8.1 keyword since safe browsing was backed out, and I don't want to forget this.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch for checkin

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Checked in attachment 221774 [details] [diff] [review] on the branch for tomorrow's nightly (and a2).

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #9)
> Let's go for a pure toggle for a2, circle back around here afterwards. 

SafeBrowsing is now retargetted at Bon Echo Alpha 3, so this seems like a good time to circle back. Can I ask why you're nervous about adding in this option to allow users to turn on the use of the "active mode"? At Alpha 3 they'll only have the single choice of provider, but I think it's important to expose both this mode and the fact that we're hoping to have multiple data providers.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Attachment: trunk patch

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Checked in on the trunk.
mozilla/browser/locales/en-US/chrome/browser/preferences/advanced.dtd 	1.14
mozilla/browser/components/preferences/advanced.xul 	1.24

Filed bug 337774 on making improvements to the additions from this bug.

Comment 28

[Sherman Dickman]

Could someone elaborate on why the UI outlined at: http://screwedbydesign.com/images/sb-pref.png was changed to the toggle?  Was there a technical reason for doing so or was this from a user experience perspective?  We really need to settle on a UI for A3.

From http://screwedbydesign.com/images/sb-pref.png, is it possible to implement the following:
* Remove "Manage Providers" button
* Rename tab to "Anti-Phishing" for now
* Keep multi-provider pull-down in place for now, with Google being the only provider listed for now.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #28)
> Could someone elaborate on why the UI outlined at:
> http://screwedbydesign.com/images/sb-pref.png was changed to the toggle?

Sherman: see comment 9. Since Safe Browsing is not going to be enabled for a2 the changes made were only temporary. Bug 337774 was filed on implementing the other things you mention that were in the original image.