430741 - make "safebrowsing by google" more discoverable

Status: RESOLVED WONTFIX

(Toolkit :: Safe Browsing, enhancement)

Description

[juan becerra [:juanb]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008042304 Minefield/3.0pre

Currently an average user can't tell that Firefox uses google's safebrowsing system. There's no "details" link in the Security preferences, and right now there's no help content mention it.

Comment 1

[Bartłomiej Brzozowiec (BartZilla)]

Attachment: Add " according to Google" in Security options window

Description of options related with so-called "safebrowsing" should mention "Google" somewhere. Proposal: change in Security window in Options/Preferences: add " according to Google". Patch attached.

Comment 2

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

Comment on attachment 318064 [details] [diff] [review]
Add " according to Google" in Security options window

We can't take this for Firefox 3 since we're past string freeze, but we can take it (or something like it) after Firefox 3 ships, for the next major release.

Comment 3

[Reed Loden [:reed]]

shouldn't we dynamically add the word "Google" based on the value of browser.safebrowsing.provider.0.name?

Comment 4

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

Sure.

Comment 5

[Bartłomiej Brzozowiec (BartZilla)]

Actually, Google is hardcoded to being one and only "phishing/malware protection provider" (see eg.  http://mxr.mozilla.org/firefox/source/browser/components/safebrowsing/content/globalstore.js#91 or http://bb.homelinux.org/firefox/sources/3.0.1/mozilla/browser/components/safebrowsing/content/globalstore.js.html#91 if you prefer something more (hopefully) stable), so, in this situation, dynamically adding would be just unnecessary excercise, I guess...

Comment 6

[Bartłomiej Brzozowiec (BartZilla)]

Related bug (about changing wording of options in "Security" window): bug 417687.

Comment 7

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

-<!ENTITY  tellMaybeAttackSite.label     "Tell me if the site I'm visiting is a suspected attack site">
+<!ENTITY  tellMaybeAttackSite.label     "Tell me if the site I'm visiting is a suspected attack site according to Google">

This patch breaks the policy of changing the entity name when the entity content has a different meaning.

Also, we're supposed to be in a string freeze...

Comment 8

[Bartłomiej Brzozowiec (BartZilla)]

Well, this patch is an "old news" now anyway, because of changes in bug 417687...
This patch has been posted long before string freeze in FF3.1, though (in April 2008)...

Comment 9

[Bartłomiej Brzozowiec (BartZilla)]

Attachment: updated patch

Bug 417687 changed strings of options related with so-called "safebrowsing" ("Tell me if the site I'm visiting is a suspected attack site" to "Block reported attack sites" and "Tell me if the site I'm visiting is a suspected forgery" to "Block reported web forgeries"), hence my previous patch is no longer valid.
This patch (against current trunk and 1.9.1 branch) changes "Block reported attack sites" to "Block attack sites reported by Google" and "Block reported web forgeries" to "Block web forgeries reported by Google". Entity names changed per suggestion from comment #7.

Comment 10

[Bartłomiej Brzozowiec (BartZilla)]

Feel free to propose better wording that includes "Google". Perhaps with added "as" it sounds better? I.e. "Block attack sites as reported by Google" and "Block web forgeries as reported by Google"...

Comment 11

[Reed Loden [:reed]]

Comment on attachment 363818 [details] [diff] [review]
updated patch

You can't just change the entity names in the DTD and not update them in the files that use them. Also, I really think you should pull the name from browser.safebrowsing.provider.0.name for this. It would make better sense and be future-proof.

Comment 12

[Aakash Desai [:aakashd]]

Are we looking at this for 3.next?

Comment 13

[Bartłomiej Brzozowiec (BartZilla)]

Attachment: add provider name of the "safebrowsing" service in Options/Preferences UI

Here's a patch that (hopefully) addresses concerns from comment 11. (To be honest, I have no idea what this talking about "future" is about -- chances for changing SB-service provider from Google to something else are virtually nil.)

You may make some complaints about this patch, so I will provide some rationale beforehand.

At first attempt I tried to make it fully "correctly", ie. to make changes in all places where names of these options appear (they also appear in about:rights - there is a section that describes how to disable this "safebrowsing" thing). Unfortunately I quickly learned that access to Components.classes (which is needed to pull the name of the provider and to construct UI label strings dynamically) is denied in about:rights. There is a possibility to allow access to C.c in about:.. page, but it comes with a price -- there is no way to link to it (due to some security issues). And linking to about:rights is necessary -- there are, at least, links to it in Help->About Firefox box and in about:license.
Gavin suggested on IRC that I may leave strings in .dtd (which are used in about:rights) intact, so I did so.

Comment for translators in preferences.properties is copied from security.dtd.

There is also another problem with this patch (and this is a reason why I didn't formally ask for a review yet): it seems that accesskeys in dynamically created labels are not underlined. They work, but they're just not underlined.
I'm not sure how to fix this, so provide some advice, please.

I tried the following approaches, all to no avail:
- removing (and not removing) accesskey= line from security.xul, putting accesskeys in preferences.properties, pulling them when creating the labels in security.js and set them in JS code:
  . before setting the label
  . after setting the label
  . with the syntax: <checkbox-element>.accesskey  = <key>;
  . with the syntax: <checkbox-element>.setAttribute("accesskey", <key>);

Any suggestions how to fix this? It seems there is no other place in Firefox options UI where the label in XUL <checkbox> element is set dynamically in JS code, so there is no example to learn from.

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 520791 [details] [diff] [review]
add provider name of the "safebrowsing" service in Options/Preferences UI

>+   * Initializes master password UI and strings used in so-called Safebrowsing UI.

I'd remove "so-called" as it sounds rather sarcastic here. It is "safebrowsing UI" in the same way as the other one is "master password UI".

>+    // FIXME: it seems that accesskeys are not underlined in dynamically created labels...

So, you're saying that when you manually modify the label, the accesskey disappears?

>+    chboxBlockAttackSites.label  = blockAttackSitesLabel;
>+    chboxBlockWebForgeries.label = blockWebForgeriesLabel;
>+  },

what if you set the accesskey once again with sth like:

chboxBlockAttackSites.setAttribute("accesskey", "k");

?

>+blockAttackSitesLabel=Block attack sites reported by %S
>+blockWebForgeriesLabel=Block web forgeries reported by %S

I'd prefer blockAttackSitesBy.label and blockWebForgeriesBy.label.
Also:

* move accesskeys to .properties as well
* remove the old ones from security.xul/dtd

Comment 15

[Bartłomiej Brzozowiec (BartZilla)]

Attachment: add provider name of the "safebrowsing" service in Options/Preferences UI

I'm attaching a new patch with issues from comment #14 addressed. The problem with a lack of underlining of accesskeys in labels persists, though...
Thanks for a quick review.

(In reply to comment #14)
> Comment on attachment 520791 [details] [diff] [review]
> add provider name of the "safebrowsing" service in Options/Preferences UI
> 
> >+   * Initializes master password UI and strings used in so-called Safebrowsing UI.
> 
> I'd remove "so-called" as it sounds rather sarcastic here. It is "safebrowsing
> UI" in the same way as the other one is "master password UI".

As you wish...

> >+    // FIXME: it seems that accesskeys are not underlined in dynamically created labels...
> 
> So, you're saying that when you manually modify the label, the accesskey
> disappears?

Well, I'm saying that accesskeys are not underlined in labels created by this patch (or the previous one).

> >+    chboxBlockAttackSites.label  = blockAttackSitesLabel;
> >+    chboxBlockWebForgeries.label = blockWebForgeriesLabel;
> >+  },
> 
> what if you set the accesskey once again with sth like:
> 
> chboxBlockAttackSites.setAttribute("accesskey", "k");
> 
> ?

It is still the same (ie. accesskey works, but it is not underlined in label).

> >+blockAttackSitesLabel=Block attack sites reported by %S
> >+blockWebForgeriesLabel=Block web forgeries reported by %S
> 
> I'd prefer blockAttackSitesBy.label and blockWebForgeriesBy.label.
> Also:
> 
> * move accesskeys to .properties as well
> * remove the old ones from security.xul/dtd

Done.
I also removed label= from security.xul, as labels are set from JS code. Entities from security.dtd are only used in about:rights (aboutRights.dtd), so I guess it is better to remove them from security.xul to avoid confusion...

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

cool! that looks good to me. I'm confused on why the accesskey underlining doesn't work for you, but not sure what could be the solution here.

Do you feel the patch is ready for a formal review?

Comment 17

[Bartłomiej Brzozowiec (BartZilla)]

(In reply to comment #16)
> cool! that looks good to me. I'm confused on why the accesskey underlining
> doesn't work for you, but not sure what could be the solution here.

Does it work for you? I don't think this is only my local problem; I suspect there is some bug in code related with handling labels in XUL elements (like <checkbox>).

> Do you feel the patch is ready for a formal review?

I consider the problem with lack of underlining accesskeys as a critical issue, but sure, feel free to ask appropriate people for a review (I'm not sure who should be added as reviewer/superreviewer/ui-reviewer, so fill these fields for me).
Perhaps other people will give some advice how to solve the problem with underlining accesskeys. And if not, we can always revisit this issue in another bug.

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 521106 [details] [diff] [review]
add provider name of the "safebrowsing" service in Options/Preferences UI

r?[e-mail]

Comment 19

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

Comment on attachment 521106 [details] [diff] [review]
add provider name of the "safebrowsing" service in Options/Preferences UI

>diff --git a/browser/components/preferences/security.js b/browser/components/preferences/security.js

>+  _initSafebrowsingUIStrings: function ()

>+    var prefSvc = Components.classes["@mozilla.org/preferences-service;1"]
>+                            .getService(Components.interfaces.nsIPrefService);
>+    var prefs = prefSvc.getBranch(null);

just use:

var prefs = Components.classes["@mozilla.org/preferences-service;1"]
                      .getService(Components.interfaces.nsIPrefBranch);

>+    var blockAttackSitesLabel  = bundlePrefs.getFormattedString("blockAttackSitesBy.label",  [providerName]);
>+    var blockWebForgeriesLabel = bundlePrefs.getFormattedString("blockWebForgeriesBy.label", [providerName]);
>+    var blockAttackSitesAccesskey  = bundlePrefs.getString("blockAttackSitesBy.accesskey");
>+    var blockWebForgeriesAccesskey = bundlePrefs.getString("blockWebForgeriesBy.accesskey");

You could remove all of these local variables and just assign to {attackCheckbox|forgeryCheckbox}.{label|accessKey} directly to make this more concise.

>+    // FIXME: it seems that accesskeys are not underlined in these labels...
>+    chboxBlockAttackSites.setAttribute("accesskey", blockAttackSitesAccesskey);
>+    chboxBlockWebForgeries.setAttribute("accesskey", blockWebForgeriesAccesskey);

I think we stopped underlining accesskeys a while ago. Just assigning to .accessKey works, though (assuming you use the right chrome access key modifier).

>diff --git a/browser/locales/en-US/chrome/browser/preferences/security.dtd b/browser/locales/en-US/chrome/browser/preferences/security.dtd

> <!ENTITY  blockAttackSites.label     "Block reported attack sites">
> <!ENTITY  blockWebForgeries.label     "Block reported web forgeries">

Can you add something along the lines of:

<!-- LOCALIZATION NOTE (blockAttackSites.label, blockWebForgeries.label):
  -  These strings should be more generic versions of the similar strings in
  -  preferences/preferences.properties (blockAttackSitesBy.label and
  -  blockWebForgeriesBy.label). They will be removed when bug 588916 is fixed.
  -->

r=me with those comments addressed - sorry for the delay.

Comment 20

[Bartłomiej Brzozowiec (BartZilla)]

Attachment: add provider name of the "safebrowsing" service in Options/Preferences UI (v.2)

Attached patch addresses comments from comment 19.
I also made some minor fixes in comments.

(In reply to comment #19)
...
> I think we stopped underlining accesskeys a while ago. Just assigning to
> .accessKey works, though (assuming you use the right chrome access key
> modifier).

Yes, it works indeed! As I said in comment 13 I tried
<checkbox-element>.accesskey  = <key>;
syntax before, but I didn't try |accessKey| instead of |accesskey|. And it seems this was the culprit here -- now everything works as it should (ie. access keys works and are underlined in UI). Thanks!

Some general comment: I think that comment 0 and title of this bug suggest that it should be meta-bug ("umbrella" bug) that should cover all fixes related with making "Safebrowsing" by Google more discoverable by users.
I hijacked this bug a little bit with my single proposal, but - obviously - this fix alone doesn't cover the whole issue, so I think it shouldn't be marked as RESOLVED FIXED after landing this patch. Some link to "details" page or privacy policy page would be useful, but it should be a subject for another bug (which should be added to "Depends on" field of this bug).

Comment 21

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

Comment on attachment 531228 [details] [diff] [review]
add provider name of the "safebrowsing" service in Options/Preferences UI (v.2)

>diff --git a/browser/components/preferences/security.js b/browser/components/preferences/security.js

>+  _initSafebrowsingUIStrings: function ()
>+  {
>+    var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                          .getService(Components.interfaces.nsIPrefService);
>+    var providerName = prefs.getCharPref("browser.safebrowsing.provider.0.name");

Did you test this? I doubt this will work, since you need nsIPrefBranch instead of nsIPrefService.

This should get ui-review.

Comment 22

[Bartłomiej Brzozowiec (BartZilla)]

Attachment: screenshot of Preferences->Security window after applying patch from attachment 531228 (Linux)

(In reply to comment #21)
> Did you test this? I doubt this will work, since you need nsIPrefBranch
> instead of nsIPrefService.

Yes, it works perfectly. I tested it on Linux. Screenshot attached.
Should I send another patch?

> This should get ui-review.

OK, so who should be added to ui-review field?

Comment 23

[Bartłomiej Brzozowiec (BartZilla)]

Attachment: previous patch but with |nsIPrefBranch| instead of |nsIPrefService|

Updated patch. Sorry for creating confusion, I just missed nsIPrefService -> nsIPrefBranch change in your comment 19.

Comment 24

[Alex Limi (:limi) — Firefox UX Team]

I don't know the history of this bug, but I'm wary to brand anything in the UI with company names and implementation details. I'm fine with putting a "Read More" link or something in there if we feel there is a need to explain where the data is coming from, how it works, etc — but this just seems like noise in the UI to me. The average user is unlikely to care where we source this data from.

Comment 25

[Alex Limi (:limi) — Firefox UX Team]

(and feel free to educate me on the whys if there's something I've missed! :)

Comment 26

[Bartłomiej Brzozowiec (BartZilla)]

(In reply to comment #24)
> (...) but this just seems like
> noise in the UI to me.

Slight modification of existing strings is "noise in the UI"? I don't buy it.

> The average user is unlikely to care where we source
> this data from.

I strongly disagree with this claim. Many users are interested in things like that. Besides, data fed to Firefox comes directly from Google, Mozilla has very limited influence on it, so users should be informed who decides which sites are blocked (and where data related with visited sites goes to...).

Comment 27

[juan becerra [:juanb]]

(In reply to comment #25)
> (and feel free to educate me on the whys if there's something I've missed! :)

The context for this is that in the beginning of time we used to allow a user to explicitly opt in to the safe browsing system provided by Google. Now it just says "block reported attack sites" or forgeries.

Curious users can find out that we use Google's system, and how it works internally if they click on the SafeBrowsing link the releasenotes, for example. Right now the average user may assume it's just a Firefox thing.

I agree we should not brand anything in the UI with company names, so the "Read More" link somewhere in there is good for me. The point is that the user be aware of this.

Comment 28

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 531236 [details] [diff] [review]
previous patch but with |nsIPrefBranch| instead of |nsIPrefService|

We don't really want branding in the UI, and it's an implementation detail that isn't relevant for most users. I don't think this should be added.

Comment 29

[Bartłomiej Brzozowiec (BartZilla)]

(In reply to comment #27)
> (In reply to comment #25)
> > (and feel free to educate me on the whys if there's something I've missed! :)
> 
> The context for this is that in the beginning of time we used to allow a
> user to explicitly opt in to the safe browsing system provided by Google.

Actually, this is not true. Google's "safebrowsing" was first integrated with Firefox in Firefox 2. It was enabled by default. Here is a screenshot of default options in that browser: http://bb.homelinux.org/en/firefox/antiphishbasic.html (don't be misled by the fact that the first option doesn't mention "Google" anywhere -- it was using protocol and data from Google).

Comment 30

[Bartłomiej Brzozowiec (BartZilla)]

(In reply to comment #28)
> Comment on attachment 531236 [details] [diff] [review] [review]
> previous patch but with |nsIPrefBranch| instead of |nsIPrefService|
> 
> We don't really want branding in the UI, and it's an implementation detail
> that isn't relevant for most users. I don't think this should be added.

How about closing this bug with WONTFIX, then? At least it will be clear that you are not interested in any patches related with this, and someone else will not waste his/her time on preparing patches...

Comment 31

[Bartłomiej Brzozowiec (BartZilla)]

(In reply to comment #28)
> Comment on attachment 531236 [details] [diff] [review] [review]
> previous patch but with |nsIPrefBranch| instead of |nsIPrefService|
> 
> We don't really want branding in the UI, and it's an implementation detail
> that isn't relevant for most users. I don't think this should be added.

Oh, and by the way -- what's the point of keeping browser.safebrowsing.provider.0.name preference with the name of the "safebrowsing" provider, when it is not used anywhere, and there is no intention to use it anywhere?

Comment 32

[Bartłomiej Brzozowiec (BartZilla)]

(In reply to comment #25)
> (and feel free to educate me on the whys if there's something I've missed! :)

For starters, there are some unresolved privacy (cookie)-related issues -- see eg. bug 368255 and bug 660719. For example, when user notices some google.com cookie that reappears even when user hasn't visited any Google-related site (bug 660719), then he/she may wonder what's going on. And mentioning "Google" in options related with "safebrowsing" would be a hint for a user that this is the culprit.

Comment 33

[Gingerbread Man]

(In reply to Bartłomiej Brzozowiec (BartZilla) from comment #32)
> For starters, there are some unresolved privacy (cookie)-related issues --
> see eg. bug 368255 and bug 660719.

Both resolved now.

(In reply to Alex Limi (:limi) — Firefox UX Team from comment #24)
> I'm fine with putting a "Read More" link or something in there if we feel
> there is a need to explain where the data is coming from, how it works, etc
(In reply to juan becerra [:juanb] from comment #27)
> the "Read More" link somewhere in there is good for me.

Bug 1197573.

(In reply to Bartłomiej Brzozowiec (BartZilla) from comment #30)
> How about closing this bug with WONTFIX, then?

That's what I'm wondering as well.

Comment 34

[Bartłomiej Brzozowiec (BartZilla)]

(In reply to Bartłomiej Brzozowiec (BartZilla) from comment #29)
> (...) http://bb.homelinux.org/en/firefox/antiphishbasic.html (don't be misled by (...)

That link doesn't work anymore. :( Here is a working one: http://bebe.freeshell.org/en/firefox/antiphishbasic.html