Closed Bug 336273 Opened 16 years ago Closed 16 years ago

create preferences panel for managing safe browsing settings

Categories

(Toolkit :: Safe Browsing, defect)

2.0 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 2 alpha2

People

(Reporter: beltzner, Assigned: rflint)

References

()

Details

(Keywords: fixed1.8.1)

Attachments

(3 files, 6 obsolete files)

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
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.
Attached patch Patch (obsolete) — Splinter Review
Basic safe browsing preferences with data provider management disabled for now.
Assignee: bugs → rflint
Status: NEW → ASSIGNED
Attachment #221245 - Flags: ui-review?(beltzner)
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.
Attachment #221245 - Flags: ui-review?(beltzner)
Attachment #221245 - Flags: ui-review+
Attachment #221245 - Flags: review?(gavin.sharp)
Also, as mentioned in today's Alpha 2 triage, please make sure that the default setting for Alpha 2 is browser.safebrowsing.enabled=false
Ok, tracking on bug 336832.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Depends on: 336832
Resolution: --- → FIXED
Blocks: 337193
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #221245 - Attachment is obsolete: true
Attachment #221374 - Flags: review?(gavin.sharp)
Attachment #221245 - Flags: review?(gavin.sharp)
not fixed, from what I can tell!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'm not sold on this approach yet. 
Let's go for a pure toggle for a2, circle back around here afterwards. 
Attached patch Toggle only (obsolete) — Splinter Review
Attachment #221374 - Attachment is obsolete: true
Attachment #221382 - Flags: review?(bugs)
Attachment #221374 - Flags: review?(gavin.sharp)
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/ ;)
Attachment #221382 - Flags: review?(bugs) → review+
Attached patch Toggle only v2Splinter Review
I make that mistake all too often :/
Thanks Gavin!

Carrying r=gavin.sharp sr+a=bugs forward.
Attachment #221382 - Attachment is obsolete: true
Attachment #221398 - Flags: superreview+
Attachment #221398 - Flags: review+
Attachment #221398 - Flags: approval-branch-1.8.1+
Checked in on the 1.8 branch. Leaving open for trunk checkin.
Keywords: fixed1.8.1
Whiteboard: [checkin needed]
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.
Status: REOPENED → ASSIGNED
Flags: blocking-firefox2?
Target Milestone: Firefox 2 alpha2 → Firefox 2 beta1
Version: Trunk → 2.0 Branch
Attachment #221518 - Flags: superreview?(bugs)
Attachment #221518 - Flags: review?(gavin.sharp)
Attachment #221518 - Flags: approval-branch-1.8.1?(bugs)
Attached patch Let's try that again (obsolete) — Splinter Review
Final exams have melted my brain :/
Attachment #221518 - Attachment is obsolete: true
Attachment #221520 - Flags: superreview?(bugs)
Attachment #221520 - Flags: review?(gavin.sharp)
Attachment #221520 - Flags: approval-branch-1.8.1?(bugs)
Attachment #221518 - Flags: superreview?(bugs)
Attachment #221518 - Flags: review?(gavin.sharp)
Attachment #221518 - Flags: approval-branch-1.8.1?(bugs)
Attached patch Preprocess advanced.dtd (obsolete) — Splinter Review
Last time, I swear ;)
Attachment #221520 - Attachment is obsolete: true
Attachment #221529 - Flags: superreview?(bugs)
Attachment #221529 - Flags: review?(gavin.sharp)
Attachment #221529 - Flags: approval-branch-1.8.1?(bugs)
Attachment #221520 - Flags: superreview?(bugs)
Attachment #221520 - Flags: review?(gavin.sharp)
Attachment #221520 - Flags: approval-branch-1.8.1?(bugs)
Comment on attachment 221529 [details] [diff] [review]
Preprocess advanced.dtd

Works fine, thanks!
Attachment #221529 - Flags: review?(gavin.sharp) → review+
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
(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.
removing the fixed1.8.1 keyword since safe browsing was backed out, and I don't want to forget this.
Keywords: fixed1.8.1
Attachment #221529 - Flags: superreview?(bugs)
Attachment #221529 - Flags: superreview+
Attachment #221529 - Flags: approval-branch-1.8.1?(bugs)
Attachment #221529 - Flags: approval-branch-1.8.1+
Attachment #221529 - Attachment is obsolete: true
Checked in attachment 221774 [details] [diff] [review] on the branch for tomorrow's nightly (and a2).
Keywords: fixed1.8.1
(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.
Target Milestone: Firefox 2 beta1 → Firefox 2 alpha3
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.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago16 years ago
Flags: blocking-firefox2?
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: Firefox 2 alpha3 → Firefox 2 alpha2
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.
(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.
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.