Make safe browsing work in a multi-profile environment.

Make safe browsing work in a multi-profile environment.

A single safe browsing service will be triggered per-profile, by passing the profile's safe browsing preference to the ResourceDispatcherHost through the ResourceContext.

References to the Default Profile used in starting and configuring the SafeBrowsingService have also been replaced with references to the appropriate profile for each situation.

BUG=83770
TEST=safe browsing is triggered correctly.

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

[Joao da Silva, 2011-06-22 13:38:03]

Just a couple of comments after a quick glance. I made an attempt at a similar
CL to fix http://crbug.com/82905 and hit the same problems.

http://codereview.chromium.org/7134017/diff/12105/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

http://codereview.chromium.org/7134017/diff/12105/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:118: if
(resource_context.safe_browsing_enabled())
Probably missing a "not" here:
if (!resource_context.safe_browsing_enabled())

http://codereview.chromium.org/7134017/diff/12105/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7134017/diff/12105/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:715: if (delegate_ &&
context.safe_browsing_enabled())
Isn't the check performed at the delegate too? It isn't the case now, but the
delegate could also take other actions even when safe browsing is disabled.

[Miranda Callahan, 2011-06-22 16:25:18]

Thanks, Joao -- fixed these items and re-uploaded. Still more has to go into
this patch to make safe browsing multi-profile aware -- working up another set
for later this afternoon.

http://codereview.chromium.org/7134017/diff/12105/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

http://codereview.chromium.org/7134017/diff/12105/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:118: if
(resource_context.safe_browsing_enabled())
On 2011/06/22 13:38:03, joaodasilva wrote:
> Probably missing a "not" here:
> if (!resource_context.safe_browsing_enabled())

Whoops, oh yes -- thanks!

http://codereview.chromium.org/7134017/diff/12105/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7134017/diff/12105/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:715: if (delegate_ &&
context.safe_browsing_enabled())
On 2011/06/22 13:38:03, joaodasilva wrote:
> Isn't the check performed at the delegate too? It isn't the case now, but the
> delegate could also take other actions even when safe browsing is disabled.

Done.

[Miranda Callahan, 2011-06-22 16:26:12]

On 2011/06/22 13:38:03, joaodasilva wrote:
> Just a couple of comments after a quick glance. I made an attempt at a similar
> CL to fix http://crbug.com/82905 and hit the same problems.

Also, thanks for mentioning this bug -- ran into this yesterday when I was
testing this patch, and had the same quick reaction you did -- "this should be
easy to fix!" :-/

[Miranda Callahan, 2011-06-22 20:28:18]

Ready for review. There are many files here, but only a few lines changed in
each file, so hopefully the review isn't too onerous. Thanks.

[mattm, 2011-06-23 01:24:25]

You'll also need to do something about the client side phishing detection. 
Currently this is gated by
BrowserProcessImpl::CreateSafeBrowsingDetectionService calling
BrowserProcessImpl::IsSafeBrowsingDetectionServiceEnabled.

Also, might be too late for this to be helpful but I've been working on making a
safe browsing doc, still incomplete but the big graph may be of interest:
https://sites.google.com/a/chromium.org/dev/developers/design-documents/safeb...

http://codereview.chromium.org/7134017/diff/27039/chrome/browser/download/dow...
File chrome/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/7134017/diff/27039/chrome/browser/download/dow...
chrome/browser/download/download_file_manager.cc:150:
prefs::kSafeBrowsingEnabled));
Why do some things check the pref directly and others go through
Profile::SafeBrowsingEnabled()?

http://codereview.chromium.org/7134017/diff/27039/chrome/browser/download/dow...
File chrome/browser/download/download_safe_browsing_client.cc (right):

http://codereview.chromium.org/7134017/diff/27039/chrome/browser/download/dow...
chrome/browser/download/download_safe_browsing_client.cc:78: if
(sb_service_.get() && !sb_service_->CheckDownloadUrl(url_chain, this)) {
Should be checking safe_browsing_enabled_ here.

http://codereview.chromium.org/7134017/diff/27039/chrome/browser/download/dow...
chrome/browser/download/download_safe_browsing_client.cc:102: if
(sb_service_.get() && !sb_service_->CheckDownloadHash(hash, this)) {
And here.

http://codereview.chromium.org/7134017/diff/27039/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/7134017/diff/27039/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:168: Start();
This means we'll still be spending bandwidth updating the safebrowsing database
even if the user has it disabled.  Would be more work, but it would be
preferable to only start once a profile that needs it is used.

http://codereview.chromium.org/7134017/diff/27039/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:996:
pref_service->GetBoolean(prefs::kSafeBrowsingEnabled)) {
Why check SafeBrowsingEnabled here?  We shouldn't be able to get here is
safebrowsing is disabled.

http://codereview.chromium.org/7134017/diff/27039/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:1035: if
(!(CanReportStats() && safe_browsing_enabled))
Same as above

http://codereview.chromium.org/7134017/diff/27039/content/browser/resource_co...
File content/browser/resource_context.h (right):

http://codereview.chromium.org/7134017/diff/27039/content/browser/resource_co...
content/browser/resource_context.h:75: void set_safe_browsing_enabled(bool
safe_browsing_enabled);
since safebrowsing is under chrome/ I guess this might also fall under the TODO
of things that don't belong in content/ ?

[Miranda Callahan, 2011-06-23 14:05:26]

Thanks for the review -- please take another look.

http://codereview.chromium.org/7134017/diff/27039/chrome/browser/download/dow...
File chrome/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/7134017/diff/27039/chrome/browser/download/dow...
chrome/browser/download/download_file_manager.cc:150:
prefs::kSafeBrowsingEnabled));
On 2011/06/23 01:24:25, mattm wrote:
> Why do some things check the pref directly and others go through
> Profile::SafeBrowsingEnabled()?

That's a really good question. :-P I'm going to remove the Profile method as
this should always be a question of checking the pref.

http://codereview.chromium.org/7134017/diff/27039/chrome/browser/download/dow...
File chrome/browser/download/download_safe_browsing_client.cc (right):

http://codereview.chromium.org/7134017/diff/27039/chrome/browser/download/dow...
chrome/browser/download/download_safe_browsing_client.cc:78: if
(sb_service_.get() && !sb_service_->CheckDownloadUrl(url_chain, this)) {
On 2011/06/23 01:24:25, mattm wrote:
> Should be checking safe_browsing_enabled_ here.

Done.

http://codereview.chromium.org/7134017/diff/27039/chrome/browser/download/dow...
chrome/browser/download/download_safe_browsing_client.cc:102: if
(sb_service_.get() && !sb_service_->CheckDownloadHash(hash, this)) {
On 2011/06/23 01:24:25, mattm wrote:
> And here.

Done.

http://codereview.chromium.org/7134017/diff/27039/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/7134017/diff/27039/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:168: Start();
On 2011/06/23 01:24:25, mattm wrote:
> This means we'll still be spending bandwidth updating the safebrowsing
database
> even if the user has it disabled.  Would be more work, but it would be
> preferable to only start once a profile that needs it is used.

I agree -- do you mind if I put this in a separate CL once this one has landed?

http://codereview.chromium.org/7134017/diff/27039/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:996:
pref_service->GetBoolean(prefs::kSafeBrowsingEnabled)) {
On 2011/06/23 01:24:25, mattm wrote:
> Why check SafeBrowsingEnabled here?  We shouldn't be able to get here is
> safebrowsing is disabled.

Yes, good point. I had to remove the check from CanReportStats(), so added it
back everywhere that CanReportStats was called. Here, though, it's not
necessary. Removed.

http://codereview.chromium.org/7134017/diff/27039/content/browser/resource_co...
File content/browser/resource_context.h (right):

http://codereview.chromium.org/7134017/diff/27039/content/browser/resource_co...
content/browser/resource_context.h:75: void set_safe_browsing_enabled(bool
safe_browsing_enabled);
On 2011/06/23 01:24:25, mattm wrote:
> since safebrowsing is under chrome/ I guess this might also fall under the
TODO
> of things that don't belong in content/ ?

Done.

[mattm, 2011-06-23 21:40:40]

http://codereview.chromium.org/7134017/diff/27039/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/7134017/diff/27039/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:168: Start();
On 2011/06/23 14:05:26, Miranda Callahan wrote:
> On 2011/06/23 01:24:25, mattm wrote:
> > This means we'll still be spending bandwidth updating the safebrowsing
> database
> > even if the user has it disabled.  Would be more work, but it would be
> > preferable to only start once a profile that needs it is used.
> 
> I agree -- do you mind if I put this in a separate CL once this one has
landed?

That's fine, as long as it doesn't make it into a release.

http://codereview.chromium.org/7134017/diff/33005/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/7134017/diff/33005/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:1005:
profile_manager()->GetDefaultProfile() : NULL;
Did you see my comment about the client side phishing detection?  This will need
some changes too.  

I guess what you'd want to do is remove the pref check from here and then
somehow pipe the value into ChromeContentRendererClient::RenderViewCreated to
choose whether to do the PhishingClassifierDelegate::Create. 
(ChromeContentBrowserClient::AppendExtraCommandLineSwitches does this right now
by passing kDisableClientSidePhishingDetection when the service object hasn't
been created, so I think you'd just have to move the pref check there.)


(And like SafeBrowsingService, change this in the next CL to only start the
ClientSideDetectionService when needed.)

http://codereview.chromium.org/7134017/diff/33005/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:1013: model_file_dir,
profile->GetRequestContext()));
I think this should use the system request context.

http://codereview.chromium.org/7134017/diff/33005/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/7134017/diff/33005/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:1030: bool
safe_browsing_enabled) {
Doesn't need this arg anymore.

[Joao da Silva, 2011-06-24 14:32:04]

While trying to make the SafeBrowsingService react to changes to the preference
I stumbled upon similar problems, and I've given this issue some thought. After
discussing with Mattias and Dominic (@mnissler, @battre) we arrived at the
solution proposed in the comments inline, please have a look.

I hope you don't mind these little changes; as a bonus, it'll also fix
http://crbug.com/82905 :-)

Please let me know what you think. Thanks!

http://codereview.chromium.org/7134017/diff/33005/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/7134017/diff/33005/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:498:
profile_params_->safe_browsing_enabled);
This won't handle changes to the preference. For example, changing it in the
preferences UI won't enable/disable safe browsing until the browser is
restarted. I've had lots of trouble trying to fix this issue :-) I'm afraid this
solution won't make it easy to handle changes to the preference. I also dislike
that it makes ResourceContext, which lives in content, aware of safe browsing,
which is a browser feature.

I thought of having a BooleanPrefMember in ProfileIOData that follows the
kSafeBrowsingEnabled preference. The BooleanPrefMember is then moved to the IO
thread (like enable_referrers_). The BooleanPrefMember already observes the
preference, so every time it is queried it returns the current value. The
ProfileIOData just exposes a boolean method to get the current value.

Notice on the next line (499) that the resource_context has a reference to the
ProfileIOData. Other places get the ProfileIOData from the ResourceContext using
resource_context.GetUserData(NULL) (search for GetUserData in
chrome_content_browser_client.cc for examples).

I guess the same could be done for safe browsing: get the ProfileIOData in
resource_dispatcher_host.cc using GetUserData(NULL) and an ugly cast, and get
the preference value from there.

I tend to prefer this solution since it doesn't change ResourceContext, and
since it is also already used in other places. WDYT?

http://codereview.chromium.org/7134017/diff/33005/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/7134017/diff/33005/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:168: Start();
I guess the SafeBrowsingService should only run as long as at least one profile
has it enabled. This requires listening for pref changes on all the profiles,
and then checking on each profile whether any has the preference still enabled.
If none have it, the service should shutdown, and start again if the pref is
enabled again too. Let me know if you plan to do it in another CL, otherwise I
can give it a try :-)

http://codereview.chromium.org/7134017/diff/33005/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7134017/diff/33005/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:498: sub, child_id,
route_id);
resource_context is a local reference to filter_->resource_context(), so the
parameter can be shortened to resource_context.safe_browsing_enabled().

Line 498 should have the same indentation as line 497.

[jam, 2011-07-07 14:31:03]

http://codereview.chromium.org/7134017/diff/49008/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7134017/diff/49008/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:27: #include
"chrome/browser/renderer_host/download_resource_handler.h"
You should be getting a deps failure for this.  Please don't add any chrome
includes to content, we're trying to remove all the existing ones..

[jam, 2011-07-07 14:35:18]

http://codereview.chromium.org/7134017/diff/48002/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host_delegate.h (right):

http://codereview.chromium.org/7134017/diff/48002/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_delegate.h:48: bool
safe_browsing_enabled,
This is a layering violation since an interface in content shouldn't know about
SB, which is in chrome. You should be able to cast to profileiodata in the
chrome impl of this interface to get this setting.

[Miranda Callahan, 2011-07-07 17:35:42]

On 2011/07/07 14:35:18, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/7134017/diff/48002/content/browser/renderer_ho...
> File content/browser/renderer_host/resource_dispatcher_host_delegate.h
(right):
> 
>
http://codereview.chromium.org/7134017/diff/48002/content/browser/renderer_ho...
> content/browser/renderer_host/resource_dispatcher_host_delegate.h:48: bool
> safe_browsing_enabled,
> This is a layering violation since an interface in content shouldn't know
about
> SB, which is in chrome. You should be able to cast to profileiodata in the
> chrome impl of this interface to get this setting.

Thanks for comments, all -- my apologies for taking a week to get back to this;
should be ready for review again.

[Miranda Callahan, 2011-07-07 17:35:50]

http://codereview.chromium.org/7134017/diff/33005/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/7134017/diff/33005/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:1005:
profile_manager()->GetDefaultProfile() : NULL;
On 2011/06/23 21:40:40, mattm wrote:
> Did you see my comment about the client side phishing detection?  This will
need
> some changes too.  
> 
> I guess what you'd want to do is remove the pref check from here and then
> somehow pipe the value into ChromeContentRendererClient::RenderViewCreated to
> choose whether to do the PhishingClassifierDelegate::Create. 
> (ChromeContentBrowserClient::AppendExtraCommandLineSwitches does this right
now
> by passing kDisableClientSidePhishingDetection when the service object hasn't
> been created, so I think you'd just have to move the pref check there.)
> 
> 
> (And like SafeBrowsingService, change this in the next CL to only start the
> ClientSideDetectionService when needed.)

Done.

http://codereview.chromium.org/7134017/diff/33005/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:1013: model_file_dir,
profile->GetRequestContext()));
On 2011/06/23 21:40:40, mattm wrote:
> I think this should use the system request context.

Done.

http://codereview.chromium.org/7134017/diff/33005/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/7134017/diff/33005/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:498:
profile_params_->safe_browsing_enabled);
On 2011/06/24 14:32:04, joaodasilva wrote:
> This won't handle changes to the preference. For example, changing it in the
> preferences UI won't enable/disable safe browsing until the browser is
> restarted. I've had lots of trouble trying to fix this issue :-) I'm afraid
this
> solution won't make it easy to handle changes to the preference. I also
dislike
> that it makes ResourceContext, which lives in content, aware of safe browsing,
> which is a browser feature.
> 
> I thought of having a BooleanPrefMember in ProfileIOData that follows the
> kSafeBrowsingEnabled preference. The BooleanPrefMember is then moved to the IO
> thread (like enable_referrers_). The BooleanPrefMember already observes the
> preference, so every time it is queried it returns the current value. The
> ProfileIOData just exposes a boolean method to get the current value.
> 
> Notice on the next line (499) that the resource_context has a reference to the
> ProfileIOData. Other places get the ProfileIOData from the ResourceContext
using
> resource_context.GetUserData(NULL) (search for GetUserData in
> chrome_content_browser_client.cc for examples).
> 
> I guess the same could be done for safe browsing: get the ProfileIOData in
> resource_dispatcher_host.cc using GetUserData(NULL) and an ugly cast, and get
> the preference value from there.
> 
> I tend to prefer this solution since it doesn't change ResourceContext, and
> since it is also already used in other places. WDYT?

Excellent idea -- done.

http://codereview.chromium.org/7134017/diff/33005/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/7134017/diff/33005/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:168: Start();
On 2011/06/24 14:32:04, joaodasilva wrote:
> I guess the SafeBrowsingService should only run as long as at least one
profile
> has it enabled. This requires listening for pref changes on all the profiles,
> and then checking on each profile whether any has the preference still
enabled.
> If none have it, the service should shutdown, and start again if the pref is
> enabled again too. Let me know if you plan to do it in another CL, otherwise I
> can give it a try :-)

Feel free to give it a try! :-)

http://codereview.chromium.org/7134017/diff/33005/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:1030: bool
safe_browsing_enabled) {
On 2011/06/23 21:40:40, mattm wrote:
> Doesn't need this arg anymore.

Done.

http://codereview.chromium.org/7134017/diff/33005/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7134017/diff/33005/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:498: sub, child_id,
route_id);
On 2011/06/24 14:32:04, joaodasilva wrote:
> resource_context is a local reference to filter_->resource_context(), so the
> parameter can be shortened to resource_context.safe_browsing_enabled().
> 
> Line 498 should have the same indentation as line 497.

Indentation fixed; safe_browsing_enabled call changed as you suggested earlier.

http://codereview.chromium.org/7134017/diff/49008/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:27: #include
"chrome/browser/renderer_host/save_file_resource_handler.h"
On 2011/07/07 14:31:03, John Abd-El-Malek wrote:
> You should be getting a deps failure for this.  Please don't add any chrome
> includes to content, we're trying to remove all the existing ones..

Ah, of course -- fixed, I hope.

http://codereview.chromium.org/7134017/diff/48002/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host_delegate.h (right):

http://codereview.chromium.org/7134017/diff/48002/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_delegate.h:48: bool
safe_browsing_enabled,
On 2011/07/07 14:35:18, John Abd-El-Malek wrote:
> This is a layering violation since an interface in content shouldn't know
about
> SB, which is in chrome. You should be able to cast to profileiodata in the
> chrome impl of this interface to get this setting.

Done.

[mattm, 2011-07-07 21:34:55]

http://codereview.chromium.org/7134017/diff/33005/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/7134017/diff/33005/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:498:
profile_params_->safe_browsing_enabled);
On 2011/07/07 17:35:50, Miranda Callahan wrote:
> On 2011/06/24 14:32:04, joaodasilva wrote:
> > This won't handle changes to the preference. For example, changing it in the
> > preferences UI won't enable/disable safe browsing until the browser is
> > restarted. I've had lots of trouble trying to fix this issue :-) I'm afraid
> this
> > solution won't make it easy to handle changes to the preference. I also
> dislike
> > that it makes ResourceContext, which lives in content, aware of safe
browsing,
> > which is a browser feature.
> > 
> > I thought of having a BooleanPrefMember in ProfileIOData that follows the
> > kSafeBrowsingEnabled preference. The BooleanPrefMember is then moved to the
IO
> > thread (like enable_referrers_). The BooleanPrefMember already observes the
> > preference, so every time it is queried it returns the current value. The
> > ProfileIOData just exposes a boolean method to get the current value.
> > 
> > Notice on the next line (499) that the resource_context has a reference to
the
> > ProfileIOData. Other places get the ProfileIOData from the ResourceContext
> using
> > resource_context.GetUserData(NULL) (search for GetUserData in
> > chrome_content_browser_client.cc for examples).
> > 
> > I guess the same could be done for safe browsing: get the ProfileIOData in
> > resource_dispatcher_host.cc using GetUserData(NULL) and an ugly cast, and
get
> > the preference value from there.
> > 
> > I tend to prefer this solution since it doesn't change ResourceContext, and
> > since it is also already used in other places. WDYT?
> 
> Excellent idea -- done.

Note that the client side detection still wouldn't respond immediately to
changes, you'd have to close any tabs that had been open before the change.  

On the browser side, I guess TabContentsWrapper can observe the pref and
create/delete safebrowsing_detection_host_ as necessary.  It could then send an
IPC to the renderer to have ChromeRenderViewObserver create/delete the
phishing_classifier_.

http://codereview.chromium.org/7134017/diff/53011/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/7134017/diff/53011/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.cc:278:
feature_extractor_.reset(new BrowserFeatureExtractor(tab, NULL));
I don't think we need to create the BrowserFeatureExtractor in this case.

[Joao da Silva, 2011-07-08 09:21:02]

LGTM, regarding reacting to preference updates. Thanks for working this out!

[Miranda Callahan, 2011-07-08 14:48:46]

Thanks, Matt -- I implemented the pref observation and message passing you
suggested; please take another look.

http://codereview.chromium.org/7134017/diff/53011/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/7134017/diff/53011/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.cc:278:
feature_extractor_.reset(new BrowserFeatureExtractor(tab, NULL));
On 2011/07/07 21:34:55, mattm wrote:
> I don't think we need to create the BrowserFeatureExtractor in this case.

Done.

[mattm, 2011-07-08 22:15:24]

http://codereview.chromium.org/7134017/diff/57008/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

http://codereview.chromium.org/7134017/diff/57008/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:131:
safe_browsing::ClientSideDetectionHost::Create(contents));
Oops, I think I gave some odd advice since looking now it's a bit strange we
always create a CSDH to start with and it checks the pref itself, then we
observe the pref to create/delete the CSDH.

I guess we should also move the SB pref check from the CSDH constructor to here
to keep things consistent (and check
g_browser_process->safe_browsing_detection_service() too)

If we do that we could remove the comments in CSDH about csd_service and
sb_service possibly being null and remove those if statements or change them to
DCHECKs

http://codereview.chromium.org/7134017/diff/57008/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:460:
UpdateSafebrowsingDetectionHost();
Just thought of one thing.. this is fine with this CL.  But wanted to mention
while I'm thinking of it that with 82905 and/or 88661 we would have to be sure
that when enabling safebrowsing the csd service and sb service exist before any
tabcontents pref observers create their ClientSideDetectionHost.  Just something
to keep in mind for later.

http://codereview.chromium.org/7134017/diff/57008/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:636: bool safe_browsing =
prefs->GetBoolean(prefs::kSafeBrowsingEnabled);
Should also check g_browser_process->safe_browsing_detection_service() isn't
NULL (related to my comment in chrome_render_view_observer.cc).

http://codereview.chromium.org/7134017/diff/57008/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_view_observer.cc (right):

http://codereview.chromium.org/7134017/diff/57008/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_view_observer.cc:348:
switches::kDisableClientSidePhishingDetection)) {
Hm, since we use the switch to set the initial state of the renderer, we should
only check it in the constructor, not in the ipc handler, otherwise you could
never enable it in a renderer that started with it disabled.

[Miranda Callahan, 2011-07-12 16:24:08]

Thanks, Matt -- here's another set of changes in response to your comments.

http://codereview.chromium.org/7134017/diff/57008/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

http://codereview.chromium.org/7134017/diff/57008/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:131:
safe_browsing::ClientSideDetectionHost::Create(contents));
On 2011/07/08 22:15:24, mattm wrote:
> Oops, I think I gave some odd advice since looking now it's a bit strange we
> always create a CSDH to start with and it checks the pref itself, then we
> observe the pref to create/delete the CSDH.
> 
> I guess we should also move the SB pref check from the CSDH constructor to
here
> to keep things consistent (and check
> g_browser_process->safe_browsing_detection_service() too)
> 
> If we do that we could remove the comments in CSDH about csd_service and
> sb_service possibly being null and remove those if statements or change them
to
> DCHECKs

Strangeness fixed.

http://codereview.chromium.org/7134017/diff/57008/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:460:
UpdateSafebrowsingDetectionHost();
On 2011/07/08 22:15:24, mattm wrote:
> Just thought of one thing.. this is fine with this CL.  But wanted to mention
> while I'm thinking of it that with 82905 and/or 88661 we would have to be sure
> that when enabling safebrowsing the csd service and sb service exist before
any
> tabcontents pref observers create their ClientSideDetectionHost.  Just
something
> to keep in mind for later.

Good point, yes -- made a note of this in 88661.

http://codereview.chromium.org/7134017/diff/57008/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:636: bool safe_browsing =
prefs->GetBoolean(prefs::kSafeBrowsingEnabled);
On 2011/07/08 22:15:24, mattm wrote:
> Should also check g_browser_process->safe_browsing_detection_service() isn't
> NULL (related to my comment in chrome_render_view_observer.cc).  

Done.

http://codereview.chromium.org/7134017/diff/57008/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_view_observer.cc (right):

http://codereview.chromium.org/7134017/diff/57008/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_view_observer.cc:348:
switches::kDisableClientSidePhishingDetection)) {
On 2011/07/08 22:15:24, mattm wrote:
> Hm, since we use the switch to set the initial state of the renderer, we
should
> only check it in the constructor, not in the ipc handler, otherwise you could
> never enable it in a renderer that started with it disabled.

Done.

[mattm, 2011-07-12 21:41:58]

http://codereview.chromium.org/7134017/diff/75003/chrome/browser/download/dow...
File chrome/browser/download/download_file_manager.cc (left):

http://codereview.chromium.org/7134017/diff/75003/chrome/browser/download/dow...
chrome/browser/download/download_file_manager.cc:147: #if
defined(ENABLE_SAFE_BROWSING)
dropped merge

http://codereview.chromium.org/7134017/diff/75003/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/7134017/diff/75003/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:112:
io_data_->safe_browsing_enabled()->Init(prefs::kSafeBrowsingEnabled,
think these all need #if defined(ENABLE_SAFE_BROWSING)

http://codereview.chromium.org/7134017/diff/75003/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(left):

http://codereview.chromium.org/7134017/diff/75003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:121:
#if defined(ENABLE_SAFE_BROWSING)
dropped merges

http://codereview.chromium.org/7134017/diff/75003/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

http://codereview.chromium.org/7134017/diff/75003/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:79:
prefs::kSafeBrowsingEnabled,
may need the ifdef?

http://codereview.chromium.org/7134017/diff/75003/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:641: void
TabContentsWrapper::UpdateSafebrowsingDetectionHost() {
ifdef

http://codereview.chromium.org/7134017/diff/75003/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (left):

http://codereview.chromium.org/7134017/diff/75003/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:148: #if
defined(ENABLE_SAFE_BROWSING)
Looks like some merges got squashed

http://codereview.chromium.org/7134017/diff/75003/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_view_observer.cc (right):

http://codereview.chromium.org/7134017/diff/75003/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_view_observer.cc:178:
OnSetClientSidePhishingDetection(true);
Should be checking the switches::kDisableClientSidePhishingDetection here

[Miranda Callahan, 2011-07-13 18:13:50]

Sorry for missing these obvious merge failures -- I think everything's fixed
now.  I had to redo one of the unit tests a tiny bit because the unit test
browser process returns a NULL safe_browsing_detection_service(); things are
running green for me now.

PTAL.

http://codereview.chromium.org/7134017/diff/75003/chrome/browser/download/dow...
File chrome/browser/download/download_file_manager.cc (left):

http://codereview.chromium.org/7134017/diff/75003/chrome/browser/download/dow...
chrome/browser/download/download_file_manager.cc:147: #if
defined(ENABLE_SAFE_BROWSING)
On 2011/07/12 21:41:58, mattm wrote:
> dropped merge

Done.

http://codereview.chromium.org/7134017/diff/75003/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/7134017/diff/75003/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:112:
io_data_->safe_browsing_enabled()->Init(prefs::kSafeBrowsingEnabled,
On 2011/07/12 21:41:58, mattm wrote:
> think these all need #if defined(ENABLE_SAFE_BROWSING)

Done.

http://codereview.chromium.org/7134017/diff/75003/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(left):

http://codereview.chromium.org/7134017/diff/75003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:121:
#if defined(ENABLE_SAFE_BROWSING)
On 2011/07/12 21:41:58, mattm wrote:
> dropped merges

Done.

http://codereview.chromium.org/7134017/diff/75003/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

http://codereview.chromium.org/7134017/diff/75003/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:79:
prefs::kSafeBrowsingEnabled,
On 2011/07/12 21:41:58, mattm wrote:
> may need the ifdef?

Done.

http://codereview.chromium.org/7134017/diff/75003/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:641: void
TabContentsWrapper::UpdateSafebrowsingDetectionHost() {
On 2011/07/12 21:41:58, mattm wrote:
> ifdef

Done.

http://codereview.chromium.org/7134017/diff/75003/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (left):

http://codereview.chromium.org/7134017/diff/75003/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:148: #if
defined(ENABLE_SAFE_BROWSING)
On 2011/07/12 21:41:58, mattm wrote:
> Looks like some merges got squashed

Wow, did they ever. :-/  Fixed.

http://codereview.chromium.org/7134017/diff/75003/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_view_observer.cc (right):

http://codereview.chromium.org/7134017/diff/75003/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_view_observer.cc:178:
OnSetClientSidePhishingDetection(true);
On 2011/07/12 21:41:58, mattm wrote:
> Should be checking the switches::kDisableClientSidePhishingDetection here

Done.

[mattm, 2011-07-13 19:11:44]

LGTM!

[commit-bot: I haz the power, 2011-07-13 19:14:22]

Presubmit check for 7134017-79005 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/renderer_host/resource_dispatcher_host_delegate.cc,content/browser/renderer_host/resource_dispatcher_host.cc,content/browser/renderer_host/resource_dispatcher_host_delegate.h

Presubmit checks took 1.6s to calculate.

[Miranda Callahan, 2011-07-13 19:18:51]

John and Randy, could you give an OWNERS review for content/browser and 
chrome/browser/download?

[Miranda Callahan, 2011-07-13 19:20:13]

On 2011/07/13 19:18:51, Miranda Callahan wrote:
> John and Randy, could you give an OWNERS review for content/browser and 
> chrome/browser/download?

Scott, could you give an OWNERS review for chrome/browser/ui?

[Randy Smith (Not in Mondays), 2011-07-13 19:23:14]

chrome/browser/download LGTM>

[sky, 2011-07-13 20:21:30]

chrome/browser/ui LGTM

http://codereview.chromium.org/7134017/diff/79005/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

http://codereview.chromium.org/7134017/diff/79005/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:644: #if
defined(ENABLE_SAFE_BROWSING)
Does this really need to be ifdefd?

[Miranda Callahan, 2011-07-13 21:13:13]

http://codereview.chromium.org/7134017/diff/79005/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

http://codereview.chromium.org/7134017/diff/79005/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:644: #if
defined(ENABLE_SAFE_BROWSING)
On 2011/07/13 20:21:30, sky wrote:
> Does this really need to be ifdefd?

No, good point -- considering that we have ifdefd off the observation of the
prefs by removing it from kPrefsToObserve, this is not necesary.

[commit-bot: I haz the power, 2011-07-13 22:58:52]

Change committed as 92438