508727 - Disable the "Report Phishing Page…" menu item when safebrowsing is off

Status: RESOLVED FIXED

(Camino Graveyard :: Security, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Per discussion at this morning's meeting, the "Report Phishing Page…" menu item should be disabled when safebrowsing is off.  The menu item should also show an appropriate tooltip when _disabled because safebrowsing is off_ (see "Check for Updates…").

Comment 1

[Sean Murphy]

Attachment: Patch

Note that there appears to be a problem on Leopard, with tooltips not displaying on menu items under the new Help menu.  I verified using F-Script that the tooltip does properly set and unset itself as the safe browsing preference changes.

Rather than wait for a workaround, I'm submitting this now to get the strings in for l10n.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

hendy said he's up to the challenge of this code review.

As for the string:

> +"PhishReportingDisabledToolTip" = "Safe browsing must be enabled to report phishing pages.";

Aside from some URL pieces on the website and mentioning on some web pages that we use Google's SB service , we don't use the term "safe browsing" anywhere.

"PhishReportingDisabledToolTip" = "“Warn me when visiting potentially malicious sites” must be enabled to report phishing pages.";

is kind of a mouthful, but that's the only name we have for the feature in the Camino UI (bonus: it's the name of the pref if the user goes poking through the UI, and the pref name will come up in a Google search of cbo once the safebrowsing page gets linked and crawled).

We could also do something like "Phishing and malware protection must be enabled to report phishing pages" but that uses phishing twice and still doesn't help match you with a string in the prefs UI.

Comment 3

[Christopher Henderson]

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

I agree with Smokey wrt the tooltip string.

>   if (action == @selector(reportPhishingPage:)) { ... }
Somewhere in this block there needs to be a check for [browserController shouldSuppressWindowActions]. We don't want to users to be able to report a site while customizing their toolbar.

If I set browser.safebrowsing.enabled to false and browser.safebrowsing.malware.enabled to true in about:config, I'm still able to report a site. Shouldn't that be disabled, since phishing detection is disabled? Maybe use eSafeBrowsingPhishingListType in the check.

r+ with those things addressed.

Comment 4

[Sean Murphy]

Attachment: Patch, v2

Thanks for the review Hendy.  All of your comments were addressed, thanks for catching those aspects.

As for the tooltip, I agree myself about the wording.  Using safe browsing didn't sit totally right with me either, the only reason I talked myself into keeping it was that we use it on the error overlay as the "more information" link title.

I have changed it to the first string Smokey mentioned for now.

Comment 5

[Stuart Morgan]

Comment on attachment 398151 [details] [diff] [review]
Patch, v2

sr=smorgan

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on cvs trunk and 2_0 branch.  Filed bug 514236 on the issue in comment 1.