Add switches and apis in safebrowsing code to allow tests.

Add switches and apis in safebrowsing protocol_manager that will allow end-to-end test later.

BUG=6787, 47318
TEST=protocol_manager_unittest.cc


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

[lzheng, 2010-06-23 19:37:02]

First CL to add test for safebrowsing. This cl is to add necessary hooks so we
could call in test.

[eroman, 2010-06-23 21:44:25]

Overall design comment:

I like to keep command-line processing out of core logic classes. And instead
make the optional parameters arguments to the constructor (helps with
unit-testing, and understanding what the actual inputs are).

Can you extract the command-line processing logic up a layer, and pass in the
resulting options as constructor arguments (or setter calls) to ProtocolManager?

[lzheng, 2010-06-23 22:00:48]

Good points. I will make the change and ping you back.
Lei

On 2010/06/23 21:44:25, eroman wrote:
> Overall design comment:
> 
> I like to keep command-line processing out of core logic classes. And instead
> make the optional parameters arguments to the constructor (helps with
> unit-testing, and understanding what the actual inputs are).
> 
> Can you extract the command-line processing logic up a layer, and pass in the
> resulting options as constructor arguments (or setter calls) to
ProtocolManager?

[lzheng, 2010-06-24 00:46:11]

I moved the switches up to safebrowsing service. Can you take another look?
Thanks!

Lei

On 2010/06/23 22:00:48, lzheng wrote:
> Good points. I will make the change and ping you back.
> Lei
> 
> On 2010/06/23 21:44:25, eroman wrote:
> > Overall design comment:
> > 
> > I like to keep command-line processing out of core logic classes. And
instead
> > make the optional parameters arguments to the constructor (helps with
> > unit-testing, and understanding what the actual inputs are).
> > 
> > Can you extract the command-line processing logic up a layer, and pass in
the
> > resulting options as constructor arguments (or setter calls) to
> ProtocolManager?

[eroman, 2010-06-24 01:29:43]

http://codereview.chromium.org/2868030/diff/36002/32003
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/2868030/diff/36002/32003#newcode47
chrome/browser/safe_browsing/protocol_manager.cc:47:
"http://%s/safebrowsing/newkey?client=%s&appver=%s&pver=2.2";
Is it intentional that you have changed the protocol from https to http?

http://codereview.chromium.org/2868030/diff/36002/32003#newcode66
chrome/browser/safe_browsing/protocol_manager.cc:66: const std::string&
hostname,
nit: I would call this "host" rather than "hostname" (since it can contain an
optional port too).

http://codereview.chromium.org/2868030/diff/36002/32003#newcode464
chrome/browser/safe_browsing/protocol_manager.cc:464: DCHECK(next_update_msec >=
0);
DCHECK_GE()

http://codereview.chromium.org/2868030/diff/36002/32003#newcode685
chrome/browser/safe_browsing/protocol_manager.cc:685: const char* base_url,
const std::string& host,
indent these continued parameters by 4 spaces.

http://codereview.chromium.org/2868030/diff/36002/32004
File chrome/browser/safe_browsing/protocol_manager.h (right):

http://codereview.chromium.org/2868030/diff/36002/32004#newcode57
chrome/browser/safe_browsing/protocol_manager.h:57: // is true, protocol manager
won't schedule next update untill
untill -> until

http://codereview.chromium.org/2868030/diff/36002/32004#newcode65
chrome/browser/safe_browsing/protocol_manager.h:65: bool should_force_update);
wording: maybe this should be called "disable_updates" or
"disable_schedule_next_update" instead.

http://codereview.chromium.org/2868030/diff/36002/32004#newcode85
chrome/browser/safe_browsing/protocol_manager.h:85: // Force the start of next
update based on |next_update_msec| in msec.
nit: Use passive tense: "Force" --> "Forces".

http://codereview.chromium.org/2868030/diff/36002/32004#newcode86
chrome/browser/safe_browsing/protocol_manager.h:86: void
ForceScheduleNextUpdate(const int next_update_msec);
Can you have this take a base::TimeDelta instead?

http://codereview.chromium.org/2868030/diff/36002/32004#newcode88
chrome/browser/safe_browsing/protocol_manager.h:88: bool is_initial_request() {
return initial_request_; }
can you make this "const" ?

http://codereview.chromium.org/2868030/diff/36002/32004#newcode112
chrome/browser/safe_browsing/protocol_manager.h:112: // at the end of urls for
downloads/gethash/newkey requests.
nit: "at the end of urls" --> "to the end of URLs".

http://codereview.chromium.org/2868030/diff/36002/32004#newcode131
chrome/browser/safe_browsing/protocol_manager.h:131: // Compose a url to fetch
safe browsing key, chunks or hash.
nit:

"Compose" --> "Composes"
"url" --> "URL".

http://codereview.chromium.org/2868030/diff/36002/32004#newcode133
chrome/browser/safe_browsing/protocol_manager.h:133: //  
http://%s/safebrowsing/xxx?client=%s&appver=%s&pver=2.2 where xxx is
nit: I suggest you also reference the constant variable names (since this
comment may become out of date with the value in the variables)

http://codereview.chromium.org/2868030/diff/36002/32004#newcode136
chrome/browser/safe_browsing/protocol_manager.h:136: // are browser's name and
version. |additional_query| is an additional query
nit: "are browser's" --> "are the browser's"

http://codereview.chromium.org/2868030/diff/36002/32004#newcode139
chrome/browser/safe_browsing/protocol_manager.h:139: const std::string& host,
const std::string& client_name,
nit: continued lines indent by 4 spaces.

http://codereview.chromium.org/2868030/diff/36002/32004#newcode277
chrome/browser/safe_browsing/protocol_manager.h:277: // Additional query string
to the urls for safebrowsing.
"url" --> URLs

http://codereview.chromium.org/2868030/diff/36002/32005
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

http://codereview.chromium.org/2868030/diff/36002/32005#newcode6
chrome/browser/safe_browsing/protocol_manager_unittest.cc:6: #include
"base/command_line.h"
no longer needed

http://codereview.chromium.org/2868030/diff/36002/32005#newcode11
chrome/browser/safe_browsing/protocol_manager_unittest.cc:11: #include
"chrome/common/chrome_switches.h"
no longer needed

http://codereview.chromium.org/2868030/diff/36002/32005#newcode145
chrome/browser/safe_browsing/protocol_manager_unittest.cc:145:
"http://%s/safebrowsing/downloads?client=%s&appver=%s&pver=2.2";
4 spaces

http://codereview.chromium.org/2868030/diff/36002/32005#newcode151
chrome/browser/safe_browsing/protocol_manager_unittest.cc:151:
"http://a.b.com/safebrowsing/downloads?client=unittest&appver=1.0&pver=2.2";
4 spaces

http://codereview.chromium.org/2868030/diff/36002/32006
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/2868030/diff/36002/32006#newcode358
chrome/browser/safe_browsing/safe_browsing_service.cc:358: std::string
safebrowsing_host = cmdline->HasSwitch(switches::kSbHost)?
nit: space before the '?'

http://codereview.chromium.org/2868030/diff/36002/32007
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

http://codereview.chromium.org/2868030/diff/36002/32007#newcode16
chrome/browser/safe_browsing/safe_browsing_service.h:16: #include
"base/gtest_prod_util.h"
is this needed?

http://codereview.chromium.org/2868030/diff/36002/32008
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/2868030/diff/36002/32008#newcode725
chrome/common/chrome_switches.cc:725: const char kSbHost[] = "sbhost";
can you give this a more descriptive name, like:
"safebrowsing-host"

http://codereview.chromium.org/2868030/diff/36002/32008#newcode729
chrome/common/chrome_switches.cc:729: const char kSbForceUpdateOnly[] =
"sb-forceupdate-only";
I am not sure I understand how this flag will be used. If some other piece of
code needs to be called (ForceScheduleNextUpdate()), then there must be another
codepath to exercise this stuff (in which case is it actually advantageous to
have this in a flag?)

[lzheng, 2010-06-25 00:40:16]

Thanks for the careful review.

I replace the hostname flag with two url_prefixes: one for the regular
information download, one for key download.

I also added several functions to generate different urls in protocol_manager
with unittests.

Please take another look when you have time.

Thanks!

Lei

http://codereview.chromium.org/2868030/diff/36002/32003
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/2868030/diff/36002/32003#newcode47
chrome/browser/safe_browsing/protocol_manager.cc:47:
"http://%s/safebrowsing/newkey?client=%s&appver=%s&pver=2.2";
On 2010/06/24 01:29:43, eroman wrote:
> Is it intentional that you have changed the protocol from https to http?
I modified the code so we have to pass in two url prefixes: one for key
fetching, another for other info.

http://codereview.chromium.org/2868030/diff/36002/32003#newcode66
chrome/browser/safe_browsing/protocol_manager.cc:66: const std::string&
hostname,
On 2010/06/24 01:29:43, eroman wrote:
> nit: I would call this "host" rather than "hostname" (since it can contain an
> optional port too).
This is changed to xxx_prefix.

http://codereview.chromium.org/2868030/diff/36002/32003#newcode464
chrome/browser/safe_browsing/protocol_manager.cc:464: DCHECK(next_update_msec >=
0);
On 2010/06/24 01:29:43, eroman wrote:
> DCHECK_GE()

Done.

http://codereview.chromium.org/2868030/diff/36002/32003#newcode685
chrome/browser/safe_browsing/protocol_manager.cc:685: const char* base_url,
const std::string& host,
On 2010/06/24 01:29:43, eroman wrote:
> indent these continued parameters by 4 spaces.

Done.

http://codereview.chromium.org/2868030/diff/36002/32004
File chrome/browser/safe_browsing/protocol_manager.h (right):

http://codereview.chromium.org/2868030/diff/36002/32004#newcode57
chrome/browser/safe_browsing/protocol_manager.h:57: // is true, protocol manager
won't schedule next update untill
On 2010/06/24 01:29:43, eroman wrote:
> untill -> until

Done.

http://codereview.chromium.org/2868030/diff/36002/32004#newcode65
chrome/browser/safe_browsing/protocol_manager.h:65: bool should_force_update);
On 2010/06/24 01:29:43, eroman wrote:
> wording: maybe this should be called "disable_updates" or
> "disable_schedule_next_update" instead.

I switched to disable_update.

http://codereview.chromium.org/2868030/diff/36002/32004#newcode85
chrome/browser/safe_browsing/protocol_manager.h:85: // Force the start of next
update based on |next_update_msec| in msec.
On 2010/06/24 01:29:43, eroman wrote:
> nit: Use passive tense: "Force" --> "Forces".
Done. Change several other verbs too.

http://codereview.chromium.org/2868030/diff/36002/32004#newcode86
chrome/browser/safe_browsing/protocol_manager.h:86: void
ForceScheduleNextUpdate(const int next_update_msec);
On 2010/06/24 01:29:43, eroman wrote:
> Can you have this take a base::TimeDelta instead?
I found it is much easier to manipulate to use an int here. There are several
places uses int to decide when to do the next update. Using TimeDelta, I will
need to convert the values around. Let me know if you feel strong about this.

http://codereview.chromium.org/2868030/diff/36002/32004#newcode88
chrome/browser/safe_browsing/protocol_manager.h:88: bool is_initial_request() {
return initial_request_; }
On 2010/06/24 01:29:43, eroman wrote:
> can you make this "const" ?

Done.

http://codereview.chromium.org/2868030/diff/36002/32004#newcode112
chrome/browser/safe_browsing/protocol_manager.h:112: // at the end of urls for
downloads/gethash/newkey requests.
On 2010/06/24 01:29:43, eroman wrote:
> nit: "at the end of urls" --> "to the end of URLs".

Done.

http://codereview.chromium.org/2868030/diff/36002/32004#newcode131
chrome/browser/safe_browsing/protocol_manager.h:131: // Compose a url to fetch
safe browsing key, chunks or hash.
On 2010/06/24 01:29:43, eroman wrote:
> nit:
> 
> "Compose" --> "Composes"
> "url" --> "URL".

Done.

http://codereview.chromium.org/2868030/diff/36002/32004#newcode133
chrome/browser/safe_browsing/protocol_manager.h:133: //  
http://%s/safebrowsing/xxx?client=%s&appver=%s&pver=2.2 where xxx is
On 2010/06/24 01:29:43, eroman wrote:
> nit: I suggest you also reference the constant variable names (since this
> comment may become out of date with the value in the variables)
The comment here are modified. Please take another look.

http://codereview.chromium.org/2868030/diff/36002/32004#newcode139
chrome/browser/safe_browsing/protocol_manager.h:139: const std::string& host,
const std::string& client_name,
On 2010/06/24 01:29:43, eroman wrote:
> nit: continued lines indent by 4 spaces.
I wish editor could do this automatically. How to you make sure the 4 spaces
here but 2 spaces at other situations?

http://codereview.chromium.org/2868030/diff/36002/32004#newcode277
chrome/browser/safe_browsing/protocol_manager.h:277: // Additional query string
to the urls for safebrowsing.
On 2010/06/24 01:29:43, eroman wrote:
> "url" --> URLs

Done.

http://codereview.chromium.org/2868030/diff/36002/32005
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

http://codereview.chromium.org/2868030/diff/36002/32005#newcode6
chrome/browser/safe_browsing/protocol_manager_unittest.cc:6: #include
"base/command_line.h"
On 2010/06/24 01:29:43, eroman wrote:
> no longer needed

Done.

http://codereview.chromium.org/2868030/diff/36002/32005#newcode11
chrome/browser/safe_browsing/protocol_manager_unittest.cc:11: #include
"chrome/common/chrome_switches.h"
On 2010/06/24 01:29:43, eroman wrote:
> no longer needed

Done.

http://codereview.chromium.org/2868030/diff/36002/32005#newcode145
chrome/browser/safe_browsing/protocol_manager_unittest.cc:145:
"http://%s/safebrowsing/downloads?client=%s&appver=%s&pver=2.2";
On 2010/06/24 01:29:43, eroman wrote:
> 4 spaces
This test is replaced by several more tests.

http://codereview.chromium.org/2868030/diff/36002/32006
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/2868030/diff/36002/32006#newcode358
chrome/browser/safe_browsing/safe_browsing_service.cc:358: std::string
safebrowsing_host = cmdline->HasSwitch(switches::kSbHost)?
On 2010/06/24 01:29:43, eroman wrote:
> nit: space before the '?'

Done.

http://codereview.chromium.org/2868030/diff/36002/32007
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

http://codereview.chromium.org/2868030/diff/36002/32007#newcode16
chrome/browser/safe_browsing/safe_browsing_service.h:16: #include
"base/gtest_prod_util.h"
On 2010/06/24 01:29:43, eroman wrote:
> is this needed?
removed.

http://codereview.chromium.org/2868030/diff/36002/32008
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/2868030/diff/36002/32008#newcode725
chrome/common/chrome_switches.cc:725: const char kSbHost[] = "sbhost";
On 2010/06/24 01:29:43, eroman wrote:
> can you give this a more descriptive name, like:
> "safebrowsing-host"
Replaced this with safebrowsing_info_url_prefix and mackey_url_prefix.

http://codereview.chromium.org/2868030/diff/36002/32008#newcode729
chrome/common/chrome_switches.cc:729: const char kSbForceUpdateOnly[] =
"sb-forceupdate-only";
On 2010/06/24 01:29:43, eroman wrote:
> I am not sure I understand how this flag will be used. If some other piece of
> code needs to be called (ForceScheduleNextUpdate()), then there must be
another
> codepath to exercise this stuff (in which case is it actually advantageous to
> have this in a flag?)
I will need this flag for the safebrowsing service test. I need to set this flag
to be true so I could control when the update happens and after it finishes, I
need to verify the result and force another update. The test will be coming.

[eroman, 2010-06-25 01:33:09]

http://codereview.chromium.org/2868030/diff/36002/32004
File chrome/browser/safe_browsing/protocol_manager.h (right):

http://codereview.chromium.org/2868030/diff/36002/32004#newcode86
chrome/browser/safe_browsing/protocol_manager.h:86: void
ForceScheduleNextUpdate(const int next_update_msec);
On 2010/06/25 00:40:16, lzheng wrote:
> On 2010/06/24 01:29:43, eroman wrote:
> > Can you have this take a base::TimeDelta instead?
> I found it is much easier to manipulate to use an int here. There are several
> places uses int to decide when to do the next update. Using TimeDelta, I will
> need to convert the values around. Let me know if you feel strong about this.
> 

ok, no need.

http://codereview.chromium.org/2868030/diff/44001/34004
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/2868030/diff/44001/34004#newcode557
chrome/browser/safe_browsing/protocol_manager.cc:557: GURL
update_url(UpdateUrl(use_mac));
Can UpdateUrl return a GURL ?

http://codereview.chromium.org/2868030/diff/44001/34004#newcode592
chrome/browser/safe_browsing/protocol_manager.cc:592: GURL
report_url(MalwareReportUrl(malware_url, page_url, referrer_url));
Can MalwareReportUrl return a GURL ?

http://codereview.chromium.org/2868030/diff/44001/34004#newcode666
chrome/browser/safe_browsing/protocol_manager.cc:666: return
ComposeUrl(info_url_prefix_, "gethash", client_name_, version_,
Shouldn't this be including the wrapped key as well? (the existing code did).

http://codereview.chromium.org/2868030/diff/44001/34004#newcode675
chrome/browser/safe_browsing/protocol_manager.cc:675: std::string
SafeBrowsingProtocolManager::MalwareReportUrl(
ReportMalwareUrl()

http://codereview.chromium.org/2868030/diff/44001/34004#newcode696
chrome/browser/safe_browsing/protocol_manager.cc:696: next_url +=
additional_query_;
indent by 2.

http://codereview.chromium.org/2868030/diff/44001/34005
File chrome/browser/safe_browsing/protocol_manager.h (right):

http://codereview.chromium.org/2868030/diff/44001/34005#newcode116
chrome/browser/safe_browsing/protocol_manager.h:116: void
set_additional_query(const std::string& query) {
do we need the setters?

my preference is for them to be only controlled via the constructor (avoids
questions like "what happens if you change the query while an update is already
in progress").

for the test paths currently using the setters, I think they could just create
separate ProtocolManager() objects.

http://codereview.chromium.org/2868030/diff/44001/34005#newcode141
chrome/browser/safe_browsing/protocol_manager.h:141: // Composes a url using URL
|prefix|, |method| (e.g.: gethash, download,
nit: I think this should read:
"Composes a URL using |prefix|, |method|" ...

http://codereview.chromium.org/2868030/diff/44001/34005#newcode151
chrome/browser/safe_browsing/protocol_manager.h:151: // Add "wrkey=xxx" to the
URL when |use_mac| is true.
How about: "Appends "wrkey=xxx" to the URL" ...

http://codereview.chromium.org/2868030/diff/44001/34006
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

http://codereview.chromium.org/2868030/diff/44001/34006#newcode232
chrome/browser/safe_browsing/protocol_manager_unittest.cc:232:
EXPECT_EQ("http://localhost:1234/foo/bar?foo", pm.NextChunkUrl(url_http_full));
Please break the line to be under 80 characters.

http://codereview.chromium.org/2868030/diff/44001/34009
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/2868030/diff/44001/34009#newcode730
chrome/common/chrome_switches.cc:730: //
SafeBrowsingProtocolManager::ForceScheduleNextUpdate() is explicitly called.
Per our discussion in person, you may not need this flag. For the tests it may
be possible to set prefs::kSafeBrowsingEnabled to false, and then enable it
later once the test is ready.

[lzheng, 2010-06-25 18:16:19]

http://codereview.chromium.org/2868030/diff/44001/34004
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/2868030/diff/44001/34004#newcode557
chrome/browser/safe_browsing/protocol_manager.cc:557: GURL
update_url(UpdateUrl(use_mac));
On 2010/06/25 01:33:09, eroman wrote:
> Can UpdateUrl return a GURL ?

Good suggestion. Done.

http://codereview.chromium.org/2868030/diff/44001/34004#newcode666
chrome/browser/safe_browsing/protocol_manager.cc:666: return
ComposeUrl(info_url_prefix_, "gethash", client_name_, version_,
On 2010/06/25 01:33:09, eroman wrote:
> Shouldn't this be including the wrapped key as well? (the existing code did).
It is bad I missed that.

http://codereview.chromium.org/2868030/diff/44001/34004#newcode675
chrome/browser/safe_browsing/protocol_manager.cc:675: std::string
SafeBrowsingProtocolManager::MalwareReportUrl(
On 2010/06/25 01:33:09, eroman wrote:
> ReportMalwareUrl()

MalwareReportUrl sounds better to me: a url for malware report. Let me know if
you change your mind.

http://codereview.chromium.org/2868030/diff/44001/34004#newcode696
chrome/browser/safe_browsing/protocol_manager.cc:696: next_url +=
additional_query_;
On 2010/06/25 01:33:09, eroman wrote:
> indent by 2.

Done.

http://codereview.chromium.org/2868030/diff/44001/34005
File chrome/browser/safe_browsing/protocol_manager.h (right):

http://codereview.chromium.org/2868030/diff/44001/34005#newcode116
chrome/browser/safe_browsing/protocol_manager.h:116: void
set_additional_query(const std::string& query) {
This is needed so each update, we could change the query string to info the test
server which step we are at. Also, it is better to re-use the same protocol
manager instance the test which is what the browser actually does in prod (there
maybe no easy way to create a new protocol manager for each update in browser
test too).

Caller (sbservice) could decide when to call this setter by observing callbacks
in UpdateFinished and IssueUpdateRequest.

I added comment to explain this.

On 2010/06/25 01:33:09, eroman wrote:
> do we need the setters?
> 
> my preference is for them to be only controlled via the constructor (avoids
> questions like "what happens if you change the query while an update is
already
> in progress").
> 
> for the test paths currently using the setters, I think they could just create
> separate ProtocolManager() objects.

http://codereview.chromium.org/2868030/diff/44001/34005#newcode141
chrome/browser/safe_browsing/protocol_manager.h:141: // Composes a url using URL
|prefix|, |method| (e.g.: gethash, download,
On 2010/06/25 01:33:09, eroman wrote:
> nit: I think this should read:
> "Composes a URL using |prefix|, |method|" ...

Done.

http://codereview.chromium.org/2868030/diff/44001/34005#newcode151
chrome/browser/safe_browsing/protocol_manager.h:151: // Add "wrkey=xxx" to the
URL when |use_mac| is true.
On 2010/06/25 01:33:09, eroman wrote:
> How about: "Appends "wrkey=xxx" to the URL" ...

Done.

http://codereview.chromium.org/2868030/diff/44001/34006
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

http://codereview.chromium.org/2868030/diff/44001/34006#newcode232
chrome/browser/safe_browsing/protocol_manager_unittest.cc:232:
EXPECT_EQ("http://localhost:1234/foo/bar?foo", pm.NextChunkUrl(url_http_full));
On 2010/06/25 01:33:09, eroman wrote:
> Please break the line to be under 80 characters.

Done.

http://codereview.chromium.org/2868030/diff/44001/34009
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/2868030/diff/44001/34009#newcode730
chrome/common/chrome_switches.cc:730: //
SafeBrowsingProtocolManager::ForceScheduleNextUpdate() is explicitly called.
Now I think about this, I may have to add a new prefs e.g.:
kSafeBrowsingDisableAutoUpdate flag instead of reusing the kSafeBrowsingEnabled.
So, when kSafeBrowsingEnabled is true and kSafeBrowsingDisableAutoUpdate is
true, we have to force each update. I will add a todo here and once we agree.
On 2010/06/25 01:33:09, eroman wrote:
> Per our discussion in person, you may not need this flag. For the tests it may
> be possible to set prefs::kSafeBrowsingEnabled to false, and then enable it
> later once the test is ready.

[eroman, 2010-06-28 18:34:10]

lgtm after considering comment below.

http://codereview.chromium.org/2868030/diff/47004/49002
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/2868030/diff/47004/49002#newcode433
chrome/browser/safe_browsing/protocol_manager.cc:433: update_timer_.Stop();
Do we not want to call update_timer_.Stop() each time?

(before it would always call this, now it only calls it for the
diable_auto_udpate_ path).

[lzheng, 2010-06-28 20:45:01]

Eric:
Thanks for the careful review. Comments below:

http://codereview.chromium.org/2868030/diff/47004/49002
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/2868030/diff/47004/49002#newcode433
chrome/browser/safe_browsing/protocol_manager.cc:433: update_timer_.Stop();
On 2010/06/28 18:34:10, eroman wrote:
> Do we not want to call update_timer_.Stop() each time?
> 
> (before it would always call this, now it only calls it for the
> diable_auto_udpate_ path).

The ForceScheduleNextUpdate() will always call update_timer_.Stop(). So, there
should be no changes on when the timer stop is called.

[eroman, 2010-06-28 20:47:11]

lgtm