Start and stop the safe browsing service depending on whether any profile is using it.

Start and stop the safe browsing service depending on whether any profile is using it.
It is currently always running, even if no profiles uses it.

BUG=88661
TEST=Safe browsing should still work as intended.


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

[Joao da Silva, 2011-07-15 18:21:56]

@mirandac: here's a fix, as we discussed. Please review profiles/ in particular,
thanks!

@shess: please review the changes to SafeBrowsingService. Currently it is always
running, even if it is disabled for all profiles; this patch makes it run only
if a profile wants to use it.

@mattm: as far as I understand your comments linked from http://crbug.com/88661,
it should only be an issue if g_browser_process->safe_browsing_service()
returned NULL when the service is not used; however the service is always there,
it's just not running. Am I missing something?

[Miranda Callahan, 2011-07-15 18:45:44]

LGTM for profiles (the other stuff looks good, but Scott should look more
closely as the expert here).  Thanks, this is great!!

[Scott Hess - ex-Googler, 2011-07-15 20:58:26]

LGTM.

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

http://codereview.chromium.org/7383012/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_service.cc:1223: std::string* pref;
Can these not be declared inline at point of use?

http://codereview.chromium.org/7383012/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_service.cc:1234:
RemovePrefService(profile->GetPrefs());
I see that NOTIFICATION_PROFILE_CREATED is not symmetrical with
NOTIFICATION_PROFILE_DESTROYED.  It makes this code seem broken at first glance.
 A comment is probably worthwhile, or a DCHECK(!otr) in the created case, or
both.

[mattm, 2011-07-15 21:30:50]

On 2011/07/15 18:21:56, Joao da Silva wrote:
> @mirandac: here's a fix, as we discussed. Please review profiles/ in
particular,
> thanks!
> 
> @shess: please review the changes to SafeBrowsingService. Currently it is
always
> running, even if it is disabled for all profiles; this patch makes it run only
> if a profile wants to use it.
> 
> @mattm: as far as I understand your comments linked from
http://crbug.com/88661,
> it should only be an issue if g_browser_process->safe_browsing_service()
> returned NULL when the service is not used; however the service is always
there,
> it's just not running. Am I missing something?

No, that's correct for the SafeBrowsingService.  But what about the
ClientSideDetectionService?  Were you going to handle it in a separate CL?

[Joao da Silva, 2011-07-18 11:38:32]

@shess: please review again the changes to profile created/destroyed
notifications.

@mattm: this CL was only meant to fix the SafeBrowsingService; I'll have a look
at the ClientSideDetectionService, but that'll be another CL.

Thanks all for reviewing!

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

http://codereview.chromium.org/7383012/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_service.cc:1223: std::string* pref;
On 2011/07/15 20:58:27, shess wrote:
> Can these not be declared inline at point of use?

Done.

http://codereview.chromium.org/7383012/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_service.cc:1234:
RemovePrefService(profile->GetPrefs());
On 2011/07/15 20:58:27, shess wrote:
> I see that NOTIFICATION_PROFILE_CREATED is not symmetrical with
> NOTIFICATION_PROFILE_DESTROYED.  It makes this code seem broken at first
glance.
>  A comment is probably worthwhile, or a DCHECK(!otr) in the created case, or
> both.
Good points. I've reduced the notifications to just PROFILE_CREATED and
PROFILE_DESTROYED, which are sent for both normal and OTR profiles. Most users
of _DESTROYED specify a profile as the source; other places have been refactored
to handle the change. WDYT?

[Scott Hess - ex-Googler, 2011-07-18 20:25:50]

LGTM.  Sending created/destroyed the same in all cases seems pretty reasonable.

[commit-bot: I haz the power, 2011-07-19 09:40:22]

Change committed as 92993