337774 - improve safe browsing preferences panel

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

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

Bug 336273 added basic pref UI to enable or disable safe browsing. This bug covers improvements to those additions, and exposing other options like "advanced mode".

See bug 366273 comment 9 and bug 366273 comment 25.

Comment 1

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

(In reply to comment #0)
> See bug 366273 comment 9 and bug 366273 comment 25.

Make that bug 336273 comment 9 and bug 336273 comment 25.

Comment 2

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

http://screwedbydesign.com/images/sb-pref.png shows the original proposal from bug  337774.

Comment 3

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

Latest mock up here: http://screwedbydesign.com/images/sb-pref2.png
Patch comin' up.

Comment 4

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

Attachment: Patch

Add the remoteLookup options and an anti-phishing help placeholder.

Comment 5

[John D'Agostino]

(In reply to comment #4)
> Created an attachment (id=222440) [edit]
> Patch
> 
> Add the remoteLookup options and an anti-phishing help placeholder.
> 

Any reason why it is Anti-Phishing instead of Safe Browsing?

Comment 6

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

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

>Index: components/preferences/advanced.js

>     this.updateAppUpdateItems();
>     this.updateAutoItems();
>     this.updateModeItems();    
>+    this.sbPrefUpdate();

How about staying consistent with the others and naming this updateSBItems()? Could get rid of the trailing whitespace after updateModeItems, too :). Looks good otherwise, though I think it would be a good idea to wait for bug 338598 to land so this can be implemented fully instead of hard-coding in "Google" in the XUL file. Sounds like we need the names of the providers to be localized prefs. I'm sure Beltzner will advise as to whether "Anti-Phishing" is the correct name to be giving the panel, if you haven't discussed it with him already.

Ben should SR this, he knows the pref panel code better than I do. It would also be a good idea to get Steffen or Jeff to look at the help changes.

Comment 7

[Sherman Dickman]

More marketing-friendly names are being batted around now, and should be implemented by Beta 1.

Comment 8

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

*** Bug 338840 has been marked as a duplicate of this bug. ***

Comment 9

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

Attachment: Patch v2

Comment 10

[u88484]

Me nit picking...

+    <p><em>Using a locally stored blacklist</em><br/>
+      With this option selected &brandShortName; will check the current site
+      against a local blacklist. No data will be transfered to third-party data
+      providers.</p>

Comma after selected

+
+    <p><em>By asking</em><br/>
+      With this option selected, &brandShortName; will send data about the
+      current page to the selected third-party data provider in order to
+      verify its identity.</p>

Apostrophe in "its" (it's). Maybe change 'verify its identity' to 'verify it's integrity'

Comment 11

[Adam Guthrie]

(In reply to comment #10)
> Apostrophe in "its" (it's).

No, "its" is right. http://www.answers.com/its

Comment 12

[u88484]

(In reply to comment #11)
> (In reply to comment #10)
> > Apostrophe in "its" (it's).
> 
> No, "its" is right. http://www.answers.com/its
> 
your right, I didn't even think of saying 'it is' while reading that.

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 222982 [details] [diff] [review]
Patch v2

al">
>           <label id="safe-pref">&safeBrowsing.label;</label>
>           <checkbox id="safe-active"
>                     class="indent"
>                     label="&safeBrowsingEnable.label;"
>                     accesskey="&safeBrowsingEnable.accesskey;"
>-                    preference="browser.safebrowsing.enabled"/>
>+                    preference="browser.safebrowsing.enabled"
>+                    oncommand="gAdvancedPane.updateSBItems();"/>

do this onchange for the preference element instead

>+          <radiogroup id="safe-provider" class="indent"
>+                      preference="browser.safebrowsing.remoteLookups">
>+            <radio id="safe-local" value="true"
>+                   class="indent"
>+                   label="&safeBrowsingLocal.label;"
>+                   accesskey="&safeBrowsingLocal.accesskey;"/>
>+            <hbox>
>+              <radio id="safe-remote" value="false"
>+                     class="indent"
>+                     label="&safeBrowsingRemote.label;"
>+                     accesskey="&safeBrowsingRemote.accesskey;"/>
>+                <menulist id="safe-provider" disabled="true">
>+                  <menupopup>
>+                    <menuitem label="Google"/>
>+                  </menupopup>
>+                </menulist>
>+            </hbox>

wacky indenting here, the menulist should align with the radio

not sure if there's l10n impact to this ordering, but we'll find out!

> <!ENTITY safeBrowsingTab.label          "Safe Browsing">
>-<!ENTITY safeBrowsing.label             "While I'm browsing the internet:">
>+<!ENTITY safeBrowsing.label             "While I'm browsing the Internet:">
> <!ENTITY safeBrowsingEnable.label       "Check to see if the site I'm visiting might be a scam">
> <!ENTITY safeBrowsingEnable.accesskey   "o">
>+<!ENTITY safeBrowsingLocal.label        "Using a locally stored blacklist">

so, blacklist is apparently a bad term.  Let's use "Using a locally stored list" for now.

r+a=me with those things addressed.  Thanks!

Comment 14

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

Attachment: Patch for checkin

Comment 15

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

Er, wait!

 <!ENTITY safeBrowsingTab.label          "Safe Browsing">

That should be "Anti-Phishing" as per bug 336273 comment 28, and comment 7 in this bug. In a nutshell, we don't want to call this "Safe Browsing" because we can't actually guarantee safety. For now let's leave it with a very generic name, and we can think up a better one for later. If we go with this now, it'll be the one we get stuck with forever.

Ryan, can you make the string change? You can carry over the r+a=mconnor.

Comment 16

[Steffen Wilberg]

Please also change the help files accordingly (Safe Browsing -> Anti-Phising).

I'd also like some guidance as to whether one should enable or disable remote lookups in the help doc. I guess this sums up to enhanced protection (the data on the server should be more up-to-date than the locally stored blacklist) vs. privacy (despite sending the url over an encrypted connection, but at least the data provider will know all the sites you are browsing).
This guidance isn't a requirement for a3 of course; we can add it later.

Comment 17

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

Attachment: Back to 'Anti-Phishing' with enhanced help entries

Okay, so that's the last time I listen to mconnor before consulting beltzner; the one true lord of UI. ;)

Comment 18

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

Checked in on the 1.8 branch.
mozilla/browser/locales/en-US/chrome/help/prefs.xhtml 	1.34.2.12
mozilla/browser/locales/en-US/chrome/help/firebird-toc.rdf 	1.34.2.10
mozilla/browser/locales/en-US/chrome/browser/preferences/advanced.dtd 	1.10.2.6
mozilla/browser/components/preferences/advanced.xul 	1.17.2.9
mozilla/browser/components/preferences/advanced.js 	1.15.2.6

Comment 19

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

And on the trunk.
mozilla/browser/locales/en-US/chrome/browser/preferences/advanced.dtd 	1.15
mozilla/browser/locales/en-US/chrome/help/prefs.xhtml 	1.47
mozilla/browser/locales/en-US/chrome/help/firebird-toc.rdf 	1.45
mozilla/browser/components/preferences/advanced.xul 	1.25
mozilla/browser/components/preferences/advanced.js 	1.21

Comment 20

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

Bug 339258 filed to fix up the menupopup.

Comment 21

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

Attachment: Oy vey

Don't know how I managed to overlook this mistake for so long, but this patch is definitely something we don't want to ship a3 without.

Comment 22

[u60234]

Comment on attachment 223244 [details] [diff] [review]
Back to 'Anti-Phishing' with enhanced help entries

>Index: locales/en-US/chrome/browser/preferences/advanced.dtd
>===================================================================
>+<!ENTITY safeBrowsingLocal.label        "Using a locally stored list">
>+<!ENTITY safeBrowsingLocal.accesskey    "a">

"A" is unfortunately already used for "Ask me what I want to do" on the Update tab. Until bug 143065 is fixed we better use one of the few letters that are not used anywhere in the Advanced pane, like "t".

Comment 23

[Marek Stępień [:marcoos, inactive]]

Comment on attachment 223244 [details] [diff] [review]
Back to 'Anti-Phishing' with enhanced help entries

-<!ENTITY safeBrowsingTab.label          "Safe Browsing">
+<!ENTITY safeBrowsingTab.label          "Anti-Phishing">

What happened to the "changed meaning => change entity name" policy? 

This may possibly make the localizers not notice this change, if they translated "Safe Browsing"...

Comment 24

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

Attachment: Patch

This'll fix both the accesskey conflicts and the flipped radio button values.

Comment 25

[Mike Connor [:mconnor]]

landed fixes on the branch

Comment 26

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

Fixes checked into the trunk by Mike:
mozilla/browser/locales/en-US/chrome/browser/preferences/advanced.dtd 	1.16
mozilla/browser/components/preferences/advanced.xul 	1.26