Add a browser test for safebrowsing service....

Add a browser test for safebrowsing service.
BUG=none
TEST=safe_browsing_service_browsertest

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

[lzheng, 2010-12-06 23:57:49]

I added a browsertest so we could do more controlled test in
SafeBrowsingService.

[Paweł Hajdan Jr., 2010-12-07 08:17:22]

Drive-by with minor test comments. No need to wait for another review by me.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:56:
NOTREACHED();  // TODO(lzheng): Implement this for future tests.
I suggest removing those NOTREACHED()s unless you like crashy tests. ADD_FAILURE
sounds like good replacement.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:258: namespace
{
nit: Is it possible to put everything here in the anonymous namespace?

[Scott Hess - ex-Googler, 2010-12-07 21:22:56]

Seems a little funky, be thinking about useful ways to make this more testable
without having to inject stuff.  [I mean long-term, not in the context of this
CL.]

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.cc:79: if (!factory_)
If this is thread-safe, tell me why, and maybe CHECK that it's happening on the
exact thread expected.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/protocol_manager.h (right):

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.h:47: class ProtocolManagerFactory
{
This seems like a bit generic of a name to be in the global namespace.  I'd be
happy with SBProtocolManagerFactory, or tucking it into the
SafeBrowsingProtocolManager itself.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.h:80: // Makes the passed
|factory| the factory used to instanciate
instantiate.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.h:263: static
ProtocolManagerFactory* factory_;
"controls", and a newline between this variable and the next comment.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_database.cc:162: factory_ = new
SafeBrowsingDatabaseFactoryImpl();
Again, if thread-safe make assertions that it is happening exactly as you expect
it to.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_database.h:51: // Makes the passed
|factory| the factory used to instanciate
"instantiate".

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_database.h:134: private:
Newline between RecordFailure() and private:.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_database.h:135: // The factory used
to instanciate a SafeBrowsingDatabase object.
"instantiate".

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:48: if
(badurls_.find(url.spec()) == badurls_.end())
I'd be happy enough with badurls_.count(), here, but given that these all have
to be consistent to make sense, I think it would be reasonable to capture the
iterator in a variable and use that for the rest of the function.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:86:
base::hash_map<std::string, Hits> badurls_;
Why not a hash_multimap<>?  [Answer: Because that would be a bad idea :-).]

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:90: class
TestSafeBrowsingDatabaseFactory : public SafeBrowsingDatabaseFactory {
Would rather you used consistent naming.  Fake* or Test* or Mock* for both the
object and the factory.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:141: check,
full_hashes_[check->url.spec()], cancache));
This scares me a little.  I can live with it.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:155: //
Factory that creates FakeSafeBrowsingDatabase instances.
Wrong class name in comment.

Again consistent naming between test object and text-object factory.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:212:
SafeBrowsingDatabase::RegisterFactory(NULL);
Un-register both factories.

I suspect you have the registration intentionally outside the nested
SetUp()/TearDown(), so that things aren't already setup.  If so, I think it's
subtle enough to comment on.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:237:
static_cast<FakeSafeBrowsingDatabase*>(db_factory_.GetDb());
Just have db_factory_.GetDb() return FakeSafeBrowsingDatabase directly.  Though
the parent class has SafeBrowsingDatabase, I think you can return a proper
subclass.

Otherwise, just add another accessor, or a friend.  static_cast<>() shouldn't be
necessary when you have the fully-specified factory.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:241:
static_cast<FakeProtocolManager*>(pm_factory_.GetProtocolManager());
Here too.

[lzheng, 2010-12-08 01:57:32]

Thanks for the comment. New change uploaded. Another look?

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.cc:79: if (!factory_)
There should only be one protocol manager globally which is created by
safebrowsing service. I added the CHECK for thread and add
DCHECK(!protocol_manager_) in safebrowsing service before the create().
On 2010/12/07 21:22:57, shess wrote:
> If this is thread-safe, tell me why, and maybe CHECK that it's happening on
the
> exact thread expected.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/protocol_manager.h (right):

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.h:47: class ProtocolManagerFactory
{
On 2010/12/07 21:22:57, shess wrote:
> This seems like a bit generic of a name to be in the global namespace.  I'd be
> happy with SBProtocolManagerFactory, or tucking it into the
> SafeBrowsingProtocolManager itself.

Done.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.h:80: // Makes the passed
|factory| the factory used to instanciate
On 2010/12/07 21:22:57, shess wrote:
> instantiate.

Done.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.h:263: static
ProtocolManagerFactory* factory_;
On 2010/12/07 21:22:57, shess wrote:
> "controls", and a newline between this variable and the next comment.

Done.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_database.cc:162: factory_ = new
SafeBrowsingDatabaseFactoryImpl();
This is not thread safe. Caller has to call this from safebrowsing thread. I
found it is hard to add a DCHECK here since we are using safe browsing thread,
but in the safe browsing service, we do have a DCHECK. I added comment on this. 
On 2010/12/07 21:22:57, shess wrote:
> Again, if thread-safe make assertions that it is happening exactly as you
expect
> it to.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:48: if
(badurls_.find(url.spec()) == badurls_.end())
On 2010/12/07 21:22:57, shess wrote:
> I'd be happy enough with badurls_.count(), here, but given that these all have
> to be consistent to make sense, I think it would be reasonable to capture the
> iterator in a variable and use that for the rest of the function.

Done.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:56:
NOTREACHED();  // TODO(lzheng): Implement this for future tests.
On 2010/12/07 08:17:22, Paweł Hajdan Jr. wrote:
> I suggest removing those NOTREACHED()s unless you like crashy tests.
ADD_FAILURE
> sounds like good replacement.

Done.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:90: class
TestSafeBrowsingDatabaseFactory : public SafeBrowsingDatabaseFactory {
"Test " then.

On 2010/12/07 21:22:57, shess wrote:
> Would rather you used consistent naming.  Fake* or Test* or Mock* for both the
> object and the factory.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:90: class
TestSafeBrowsingDatabaseFactory : public SafeBrowsingDatabaseFactory {
"Test " then.

On 2010/12/07 21:22:57, shess wrote:
> Would rather you used consistent naming.  Fake* or Test* or Mock* for both the
> object and the factory.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:155: //
Factory that creates FakeSafeBrowsingDatabase instances.
On 2010/12/07 21:22:57, shess wrote:
> Wrong class name in comment.
> 
> Again consistent naming between test object and text-object factory.

Done.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:212:
SafeBrowsingDatabase::RegisterFactory(NULL);
On 2010/12/07 21:22:57, shess wrote:
> Un-register both factories.
> 
> I suspect you have the registration intentionally outside the nested
> SetUp()/TearDown(), so that things aren't already setup.  If so, I think it's
> subtle enough to comment on.

Done.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:237:
static_cast<FakeSafeBrowsingDatabase*>(db_factory_.GetDb());
On 2010/12/07 21:22:57, shess wrote:
> Just have db_factory_.GetDb() return FakeSafeBrowsingDatabase directly. 
Though
> the parent class has SafeBrowsingDatabase, I think you can return a proper
> subclass.
> 
> Otherwise, just add another accessor, or a friend.  static_cast<>() shouldn't
be
> necessary when you have the fully-specified factory.

Done.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:241:
static_cast<FakeProtocolManager*>(pm_factory_.GetProtocolManager());
On 2010/12/07 21:22:57, shess wrote:
> Here too.

Done.

http://codereview.chromium.org/5544008/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:258: namespace
{
On 2010/12/07 08:17:22, Paweł Hajdan Jr. wrote:
> nit: Is it possible to put everything here in the anonymous namespace?

Done.

[Scott Hess - ex-Googler, 2010-12-10 22:20:59]

lgtm, fix the leaking factories.

http://codereview.chromium.org/5544008/diff/38001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/protocol_manager.h (right):

http://codereview.chromium.org/5544008/diff/38001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.h:141: base::Time last_update()
const { return last_update_; }
Whitespace between comment+fn groups.

http://codereview.chromium.org/5544008/diff/38001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

http://codereview.chromium.org/5544008/diff/38001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:51: badurls_it
= badurls_.find(url.spec());
Sigh.  Catching the iterator always seems like such a good idea in your brain,
but half the time when you do it things get so verbose that you kinda regret the
suggestion.

Still the right thing to do, I think.

http://codereview.chromium.org/5544008/diff/38001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:109:
TestSafeBrowsingDatabase* db_;
Should this be scoped_ptr<>?

http://codereview.chromium.org/5544008/diff/38001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:186:
TestProtocolManager* pm_;
Should this be scoped_ptr<>?

http://codereview.chromium.org/5544008/diff/38001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:222: // (which
distructs SafeBrowsingService).
s/distructs/destructs/

http://codereview.chromium.org/5544008/diff/38001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:241: const
SBFullHashResult& full_hash) {
indent.

[lzheng, 2010-12-11 01:15:20]

Thanks. Addressed the comments. FYI: The db_ and pm_ pointers are owned by
SafeBrowsingService, it is commented in the code.

http://codereview.chromium.org/5544008/diff/38001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/protocol_manager.h (right):

http://codereview.chromium.org/5544008/diff/38001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/protocol_manager.h:141: base::Time last_update()
const { return last_update_; }
On 2010/12/10 22:20:59, shess wrote:
> Whitespace between comment+fn groups.

Done.

http://codereview.chromium.org/5544008/diff/38001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

http://codereview.chromium.org/5544008/diff/38001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:51: badurls_it
= badurls_.find(url.spec());
LoL.
On 2010/12/10 22:20:59, shess wrote:
> Sigh.  Catching the iterator always seems like such a good idea in your brain,
> but half the time when you do it things get so verbose that you kinda regret
the
> suggestion.
> 
> Still the right thing to do, I think.

http://codereview.chromium.org/5544008/diff/38001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:109:
TestSafeBrowsingDatabase* db_;
On 2010/12/10 22:20:59, shess wrote:
> Should this be scoped_ptr<>?
No, see comments above. It is owned by the safebrowsing service.

http://codereview.chromium.org/5544008/diff/38001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:186:
TestProtocolManager* pm_;
On 2010/12/10 22:20:59, shess wrote:
> Should this be scoped_ptr<>?
Same as db_. Owned by safebrowsing service.

http://codereview.chromium.org/5544008/diff/38001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:222: // (which
distructs SafeBrowsingService).
On 2010/12/10 22:20:59, shess wrote:
> s/distructs/destructs/

Done.

http://codereview.chromium.org/5544008/diff/38001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:241: const
SBFullHashResult& full_hash) {
On 2010/12/10 22:20:59, shess wrote:
> indent.

Done.

[Scott Hess - ex-Googler, 2010-12-13 20:33:52]

lgtm.

I'm nervous that it won't be passed by valgrind, though.

http://codereview.chromium.org/5544008/diff/38001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

http://codereview.chromium.org/5544008/diff/38001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:109:
TestSafeBrowsingDatabase* db_;
On 2010/12/11 01:15:20, lzheng wrote:
> On 2010/12/10 22:20:59, shess wrote:
> > Should this be scoped_ptr<>?
> No, see comments above. It is owned by the safebrowsing service.

Sorry, I think I was conflating the factory instances with the instances
generated by the factories.