Turn on client-side phishing detection for UMA users with SafeBrowsing enabled.

Turn on client-side phishing detection for UMA users with SafeBrowsing enabled.

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79931

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80120

[noelutz, 2011-03-28 20:55:16]

LGTM

noe.

http://codereview.chromium.org/6708059/diff/4001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/6708059/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_service.cc:18: #include
"chrome/browser/safe_browsing/client_side_detection_service.h"
Is this header necessary?

[Brian Ryner, 2011-03-29 00:04:41]

http://codereview.chromium.org/6708059/diff/4001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/6708059/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_service.cc:18: #include
"chrome/browser/safe_browsing/client_side_detection_service.h"
On 2011/03/28 20:55:16, noelutz wrote:
> Is this header necessary?

Whoops, this was left over from a code reorganization that I ended up deciding
not to do.  Removed.

[noelutz, 2011-03-29 00:11:17]

LGTM

noe.

[Brian Ryner, 2011-03-30 22:51:38]

Lei or Scott, did you have any comments on this?

[lzheng, 2011-03-30 23:19:00]

LGTM.

Cc'ing Matt, who offers help on this topic.

[Scott Hess - ex-Googler, 2011-03-30 23:30:12]

My only comment mirrors one I had to Lei when he made a similar change: Consider
dropping the switch entirely, unless you can think of a good use-case for why
anyone would ever need to use it.  If the code is crashing and stuff, telling
users to flip the flag isn't really a great approach, we'd really need to just
revert and push again.  And this approach saves a later CL to remove the flag
:-).

[One use-case I can think of for the flag would be if you think it's likely that
you'll need to debug a problem with someone one-on-one.]

[Brian Ryner, 2011-03-30 23:35:19]

On 2011/03/30 23:30:12, shess wrote:
> My only comment mirrors one I had to Lei when he made a similar change:
Consider
> dropping the switch entirely, unless you can think of a good use-case for why
> anyone would ever need to use it.  If the code is crashing and stuff, telling
> users to flip the flag isn't really a great approach, we'd really need to just
> revert and push again.  And this approach saves a later CL to remove the flag
> :-).
> 
> [One use-case I can think of for the flag would be if you think it's likely
that
> you'll need to debug a problem with someone one-on-one.]

It's possible that we would, at least as we're initially rolling out.  I'm fine
with doing a follow-up CL to remove the switch once it's had some time to bake
on the dev channel.

[jam, 2011-03-31 00:34:22]

content lgtm

[Brian Ryner, 2011-03-31 19:24:51]

The original change broke ChromeOS because the kMetricsReportingEnabled pref
isn't registered on that platform.  For now, we'll just disable client-side
phishing detection on ChromeOS.  We'll plan to include it later once we're ready
to roll out to a broader user base.

Please take another look.

[noelutz, 2011-03-31 19:26:39]

LGTM

Thanks,
noe.

On 2011/03/31 19:24:51, Brian Ryner wrote:
> The original change broke ChromeOS because the kMetricsReportingEnabled pref
> isn't registered on that platform.  For now, we'll just disable client-side
> phishing detection on ChromeOS.  We'll plan to include it later once we're
ready
> to roll out to a broader user base.
> 
> Please take another look.

[Brian Ryner, 2011-03-31 20:31:36]

Matt, did you want to take a look as well?

[mattm, 2011-03-31 21:08:31]

On 2011/03/31 20:31:36, Brian Ryner wrote:
> Matt, did you want to take a look as well?

My only concern was that mail last week "Command-line flags considered harmful"
about command lines getting too long for the zygote.  But looking at the bugs
(eg crbug.com/77230 comment #1) it seems the problem is mostly flags that are
being used to pass arbitrary amounts of data, not simple switches.  So, LGTM.