Split to SafeBrowsingDatabaseManager into Local* and Remote*.

Split to SafeBrowsingDatabaseManager into Local* and Remote*.

SafeBrowsingDatabaseManager: Pure interface
  TestSafeBrowsingDatabaseManager: Dummy impl for tests
  LocalSafeBrowsingDatabaseManager: Existing full impl.
  RemoteSafeBrowsingDatabaseManager: New for Android

Wire up the safe_browsing=3 build-mode to switch Android to
use Remote*.  Move SafeBrowsingCheck into Local* and create
the Test* impl that can't accidentally call a real
implementation's methods.  Adjust tests to use either Test*
or Local*.

The initial implementation of Remote* does the bookeeping
of requests, but doesn't yet make calls to Java and isn't
tested.

BUG=474608
Committed: https://crrev.com/b2cd5eef530be7100274af882af3f5aff4d1c4be
Cr-Commit-Position: refs/heads/master@{#329551}

[Nathan Parker, 2015-04-29 00:34:06]

nparker@chromium.org changed reviewers:
+ mattm@chromium.org

[Nathan Parker, 2015-04-29 00:34:07]

Hi Matt --

Please start with the *DatabaseManager classes since I'm still picking off some
test failures.  This CL shouldn't change any actual functionality on Desktop,
and only changes Android when compiled with safe_browsing=3.  Thanks.

 -- Nathan

[mattm, 2015-04-29 22:59:59]

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/android_safe_browsing_api_handler.cc (right):

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/android_safe_browsing_api_handler.cc:27:
RemoteSafeBrowsingDatabaseManager::ClientRequest* client_request) {
I find the split of ClientRequests handling being implemented in both
AndroidSafeBrowsingAPIHandler and RemoteSafeBrowsingDatabaseManager confusing. I
think it would be cleaner and more understandable if ClientRequest was only a
RemoteSafeBrowsingDatabaseManager thing, and AndroidSafeBrowsingAPIHandler just
takes a base::Callback or something like that.

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/android_safe_browsing_api_handler.cc:59: // 1)
Convert metadata to be ascii proto of MalwarePatternType
metadata should be encoded protobuf, not ascii. (using protobuf ParseFromString
just means it's taking in a std::string).

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/database_manager.h (right):

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/database_manager.h:121: virtual
SafeBrowsingProtocolManagerDelegate* GetProtocolManagerDelegate() = 0;
Seems like this shouldn't be part of the base SafeBrowsingDatabaseManager
interface, though I don't have an idea off hand to handling it cleanly...

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/local_database_manager.cc (right):

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/local_database_manager.cc:659:
content::Source<LocalSafeBrowsingDatabaseManager>(this),
I think everything that observes NOTIFICATION_SAFE_BROWSING_UPDATE_COMPLETE also
needs to have the Source type updated.

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/remote_database_manager.cc (right):

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.cc:38: DCHECK(false) <<
"Not available in this implementation.";
how about just make these NOTREACHED?

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.cc:103:
DCHECK(api_handler_->StartRequest(req)) << "Request failed to start";
don't put real code in a DCHECK

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.cc:113:
std::vector<ClientRequest*> to_delete;
Doesn't actually delete here. Maybe to_remove?

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_app_manager.cc (right):

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_app_manager.cc:8: #include "base/location.h"
?

[Nathan Parker, 2015-05-04 21:17:19]

Thanks for the review, Matt!  PTAL.

I've added a delayed callback to simulate the Java side, and confirmed it works
as intended.

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/android_safe_browsing_api_handler.cc (right):

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/android_safe_browsing_api_handler.cc:27:
RemoteSafeBrowsingDatabaseManager::ClientRequest* client_request) {
On 2015/04/29 22:59:59, mattm wrote:
> I find the split of ClientRequests handling being implemented in both
> AndroidSafeBrowsingAPIHandler and RemoteSafeBrowsingDatabaseManager confusing.
I
> think it would be cleaner and more understandable if ClientRequest was only a
> RemoteSafeBrowsingDatabaseManager thing, and AndroidSafeBrowsingAPIHandler
just
> takes a base::Callback or something like that.

ok, I've make ClientRequest private and moved all of its logic there and in
RSBM.  I'm getting used to refcounted ptrs (they're frown'd upon in google3).

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/android_safe_browsing_api_handler.cc:59: // 1)
Convert metadata to be ascii proto of MalwarePatternType
On 2015/04/29 22:59:59, mattm wrote:
> metadata should be encoded protobuf, not ascii. (using protobuf
ParseFromString
> just means it's taking in a std::string).
Fixed.

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/database_manager.h (right):

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/database_manager.h:121: virtual
SafeBrowsingProtocolManagerDelegate* GetProtocolManagerDelegate() = 0;
On 2015/04/29 22:59:59, mattm wrote:
> Seems like this shouldn't be part of the base SafeBrowsingDatabaseManager
> interface, though I don't have an idea off hand to handling it cleanly...

I agree, I don't like this, but I haven't found a better solution.  This can be
solved wholly within the #ifdefs of SaveBrowsingService if I could merge
CreateDatabaseManager(), Initialize() and StartOnIOThread(), but I decided that
was worse.  I could static_cast<>.  Do you know if we can do run-time down-cast
type checking?

Added a TODO to fix this.

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/local_database_manager.cc (right):

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/local_database_manager.cc:659:
content::Source<LocalSafeBrowsingDatabaseManager>(this),
On 2015/04/29 22:59:59, mattm wrote:
> I think everything that observes NOTIFICATION_SAFE_BROWSING_UPDATE_COMPLETE
also
> needs to have the Source type updated.

I've switched them all to SafeBrowsingDatabaseManager since otherwise some calls
will need a downcast.

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/remote_database_manager.cc (right):

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.cc:38: DCHECK(false) <<
"Not available in this implementation.";
On 2015/04/29 22:59:59, mattm wrote:
> how about just make these NOTREACHED?

Done.

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.cc:103:
DCHECK(api_handler_->StartRequest(req)) << "Request failed to start";
On 2015/04/29 22:59:59, mattm wrote:
> don't put real code in a DCHECK

Indeed.  Fixed.

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.cc:113:
std::vector<ClientRequest*> to_delete;
On 2015/04/29 22:59:59, mattm wrote:
> Doesn't actually delete here. Maybe to_remove? 

I've refactored this since it's now a vector of scoped_refptrs, and since I
really just expect one, I now only remove one.

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_app_manager.cc (right):

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_app_manager.cc:8: #include "base/location.h"
On 2015/04/29 22:59:59, mattm wrote:
> ?

Yea... This is required for FROM_HERE to be defined (used below), and should be
included.  Used to get it from

chrome/browser/extensions/extension_service.h ->
chrome/browser/extensions/blacklist.h ->
chrome/browser/safe_browsing/database_manager.h ->
  (I severed this tie)
chrome/browser/safe_browsing/protocol_manager.h ->
base/timer/timer.h ->
base/location.h

I'll put this in a separate CL.

[mattm, 2015-05-06 02:42:46]

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/database_manager.h (right):

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/database_manager.h:121: virtual
SafeBrowsingProtocolManagerDelegate* GetProtocolManagerDelegate() = 0;
On 2015/05/04 21:17:18, nparker wrote:
> On 2015/04/29 22:59:59, mattm wrote:
> > Seems like this shouldn't be part of the base SafeBrowsingDatabaseManager
> > interface, though I don't have an idea off hand to handling it cleanly...
> 
> I agree, I don't like this, but I haven't found a better solution.  This can
be
> solved wholly within the #ifdefs of SaveBrowsingService if I could merge
> CreateDatabaseManager(), Initialize() and StartOnIOThread(), but I decided
that
> was worse.  I could static_cast<>.  Do you know if we can do run-time
down-cast
> type checking?
> 
> Added a TODO to fix this.

No, pretty sure we don't have RTTI.

But if the type can easily be verified to be correct by code inspection just
doing a downcast should be okay.  Downcasting is done in a number of places in
chrome, although those ones aren't generally where the type could vary based on
ifdef, I think it's fine.

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_app_manager.cc (right):

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_app_manager.cc:8: #include "base/location.h"
On 2015/05/04 21:17:19, nparker wrote:
> On 2015/04/29 22:59:59, mattm wrote:
> > ?
> 
> Yea... This is required for FROM_HERE to be defined (used below), and should
be
> included.  Used to get it from
> 
> chrome/browser/extensions/extension_service.h ->
> chrome/browser/extensions/blacklist.h ->
> chrome/browser/safe_browsing/database_manager.h ->
>   (I severed this tie)
> chrome/browser/safe_browsing/protocol_manager.h ->
> base/timer/timer.h ->
> base/location.h
> 
> I'll put this in a separate CL.

ah, makes sense. Thanks.

https://codereview.chromium.org/1110723002/diff/140001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1110723002/diff/140001/build/common.gypi#newc...
build/common.gypi:2176: 'clang_plugin_args%': '',
For future reference, it's best when updating a CL to upload separate patchsets,
one containing just the rebase, and then one (or more) containing the updates to
the CL. Makes it easier to diff from previous versions.

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/remote_database_manager.cc (right):

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.cc:104:
scoped_refptr<ClientRequest> req = new ClientRequest();
Refcounting is definitely more common in chrome, but still generally best
avoided where possible. Seems like this would work with having the callback take
a weakptr instead of refcounting.

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.cc:131: if (!client)
if you do it with weakptrs I think you can also get rid of the setting
client=NULL stuff, and just delete the ClientRequest as soon as it finishes or
is canceled.

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/remote_database_manager.h (right):

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.h:59: protected:
Why are these things protected instead of private?

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.h:61: bool enabled_;
members should go after all types and methods in the section

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.h:64: class ClientRequest :
public base::RefCounted<ClientRequest> {
I think generally inner classes will go near the top of the section, before any
methods.

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.h:70: Client* client;
members of classes should have trailing _

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/test_database_manager.h (right):

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/test_database_manager.h:20: public
SafeBrowsingProtocolManagerDelegate {
Doesn't that make this sort of a TestLocalSafeBrowsingDatabaseManager? Seems a
little weird..

[Nathan Parker, 2015-05-06 22:27:36]

PTAL.  Thanks for the suggestions, this is much improved.

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/database_manager.h (right):

https://codereview.chromium.org/1110723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/database_manager.h:121: virtual
SafeBrowsingProtocolManagerDelegate* GetProtocolManagerDelegate() = 0;
On 2015/05/06 02:42:45, mattm wrote:
> On 2015/05/04 21:17:18, nparker wrote:
> > On 2015/04/29 22:59:59, mattm wrote:
> > > Seems like this shouldn't be part of the base SafeBrowsingDatabaseManager
> > > interface, though I don't have an idea off hand to handling it cleanly...
> > 
> > I agree, I don't like this, but I haven't found a better solution.  This can
> be
> > solved wholly within the #ifdefs of SaveBrowsingService if I could merge
> > CreateDatabaseManager(), Initialize() and StartOnIOThread(), but I decided
> that
> > was worse.  I could static_cast<>.  Do you know if we can do run-time
> down-cast
> > type checking?
> > 
> > Added a TODO to fix this.
> 
> No, pretty sure we don't have RTTI.
> 
> But if the type can easily be verified to be correct by code inspection just
> doing a downcast should be okay.  Downcasting is done in a number of places in
> chrome, although those ones aren't generally where the type could vary based
on
> ifdef, I think it's fine.


I've moved all this logic out of *DatabaseManager and into SafeBrowsingService
where most of the #ifdef logic lives, and did a downcast.  The only caveat here
is that tests can override SafeBrowsingService::CreateDatabaseManager() to
return a non-delegate-derived class (such as TestSBDB), but I made them also
override the func that determines the delegate, so it works.  The prod code is
trivially code-inspectable.

https://codereview.chromium.org/1110723002/diff/140001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1110723002/diff/140001/build/common.gypi#newc...
build/common.gypi:2176: 'clang_plugin_args%': '',
On 2015/05/06 02:42:45, mattm wrote:
> For future reference, it's best when updating a CL to upload separate
patchsets,
> one containing just the rebase, and then one (or more) containing the updates
to
> the CL. Makes it easier to diff from previous versions.

Yup, sorry, I aim to do that.

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/remote_database_manager.cc (right):

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.cc:104:
scoped_refptr<ClientRequest> req = new ClientRequest();
On 2015/05/06 02:42:45, mattm wrote:
> Refcounting is definitely more common in chrome, but still generally best
> avoided where possible. Seems like this would work with having the callback
take
> a weakptr instead of refcounting.

Good idea.  I think that's what you were suggesting on the doc, now that you
describe it this way. 
Done.

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.cc:131: if (!client)
On 2015/05/06 02:42:45, mattm wrote:
> if you do it with weakptrs I think you can also get rid of the setting
> client=NULL stuff, and just delete the ClientRequest as soon as it finishes or
> is canceled.

Done.

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/remote_database_manager.h (right):

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.h:59: protected:
On 2015/05/06 02:42:46, mattm wrote:
> Why are these things protected instead of private?

Fixed.

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.h:61: bool enabled_;
On 2015/05/06 02:42:46, mattm wrote:
> members should go after all types and methods in the section

Done.

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.h:64: class ClientRequest :
public base::RefCounted<ClientRequest> {
On 2015/05/06 02:42:45, mattm wrote:
> I think generally inner classes will go near the top of the section, before
any
> methods.

Done.

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.h:70: Client* client;
On 2015/05/06 02:42:45, mattm wrote:
> members of classes should have trailing _

Done.

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/test_database_manager.h (right):

https://codereview.chromium.org/1110723002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/test_database_manager.h:20: public
SafeBrowsingProtocolManagerDelegate {
On 2015/05/06 02:42:46, mattm wrote:
> Doesn't that make this sort of a TestLocalSafeBrowsingDatabaseManager? Seems a
> little weird..

I've removed this since SafeBrowsingService now handles it all.

[Nathan Parker, 2015-05-06 22:47:08]

nparker@chromium.org changed reviewers:
+ scottmg@chromium.org, thakis@chromium.org

[Nathan Parker, 2015-05-06 22:47:09]

thakis@chromium.org: Please review changes in chrome/browser/ excluding
safe_browsing (mattm has those)

scottmg@chromium.org: Please review changes in build/

Thanks!

[scottmg, 2015-05-06 23:00:24]

build/ lgtm

[mattm, 2015-05-08 02:17:26]

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/android_safe_browsing_api_handler.cc (right):

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/android_safe_browsing_api_handler.cc:33: // will
delete this ptr, thus releasing the internal ref.
Stale comment? Also not sure talking about refs here makes sense in any case.
The consumer of a callback shouldn't really care how the callback is
implemented.

Could just say we need to make a copy of the callback on the heap so it can be
passed through JNI.

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/android_safe_browsing_api_handler.cc:53: // Static.
nit: In chrome I've only ever seen these comments spelled as "// static"

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/android_safe_browsing_api_handler.cc:60: return;
I don't think you should try to handle this failure, since it should never
occur, right?  Just DCHECK and continue. If somehow there is a time it's null,
we'd rather it crashes so we can get a crash report and know we have a bug.

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/remote_database_manager.cc (right):

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.cc:102: req->url_ = url;
could just have ClientRequest constructor take these as params

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/remote_database_manager.h (right):

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.h:26: // Need to initialize
before using.
nit: clarify comment & match style guide. maybe something like "Construct <foo>.
Must be initialized by calling <bar> before using."

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.h:72:
base::WeakPtrFactory<ClientRequest> weak_factory_;
I guess the members should also be private and have accessors.

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.cc:478: //
SafeBrowsingProtocolManagerDelegate should override this to return NULL.
Should be "anything that isn't derived from LocalSafeBrowsingDatabaseManager",
for the cast to work, right? not just any SafeBrowsingProtocolManagerDelegate.

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:485: static
LocalSafeBrowsingDatabaseManager* local_database_manager(
I don't think lower_underscore names apply to static functions. Maybe
"LocalDatabaseManagerForService" ?

https://codereview.chromium.org/1110723002/diff/200001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1110723002/diff/200001/chrome/chrome_browser....
chrome/chrome_browser.gypi:2360:
'browser/renderer_host/safe_browsing_resource_throttle.cc',
I thought this still shouldn't be used for mobile(=2)?

https://codereview.chromium.org/1110723002/diff/200001/chrome/chrome_browser....
chrome/chrome_browser.gypi:2366:
'browser/safe_browsing/malware_details_cache.h',
sorting got switched around here?

https://codereview.chromium.org/1110723002/diff/200001/chrome/chrome_browser....
chrome/chrome_browser.gypi:2378:
'browser/safe_browsing/safe_browsing_database.h',
not needed for 2 or 3, right?

https://codereview.chromium.org/1110723002/diff/200001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (left):

https://codereview.chromium.org/1110723002/diff/200001/chrome/chrome_tests.gy...
chrome/chrome_tests.gypi:2951:
'../testing/android/on_device_instrumentation.gyp:require_driver_apk',
was this intentional?

[Nathan Parker, 2015-05-12 01:04:02]

Thanks for the review.  PTAL.

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/android_safe_browsing_api_handler.cc (right):

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/android_safe_browsing_api_handler.cc:33: // will
delete this ptr, thus releasing the internal ref.
On 2015/05/08 02:17:26, mattm wrote:
> Stale comment? Also not sure talking about refs here makes sense in any case.
> The consumer of a callback shouldn't really care how the callback is
> implemented.
> 
> Could just say we need to make a copy of the callback on the heap so it can be
> passed through JNI.

Done.

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/android_safe_browsing_api_handler.cc:53: // Static.
On 2015/05/08 02:17:26, mattm wrote:
> nit: In chrome I've only ever seen these comments spelled as "// static"

Done.

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/android_safe_browsing_api_handler.cc:60: return;
On 2015/05/08 02:17:26, mattm wrote:
> I don't think you should try to handle this failure, since it should never
> occur, right?  Just DCHECK and continue. If somehow there is a time it's null,
> we'd rather it crashes so we can get a crash report and know we have a bug.

Interesting.  I may do that in a few other places too then, since I agree
crashing could be more useful than randomly malfunctioning (leaking, hanging)
for very unlikely errors.

Done.

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/remote_database_manager.cc (right):

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.cc:102: req->url_ = url;
On 2015/05/08 02:17:26, mattm wrote:
> could just have ClientRequest constructor take these as params

Done, while making them private.

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/remote_database_manager.h (right):

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.h:26: // Need to initialize
before using.
On 2015/05/08 02:17:26, mattm wrote:
> nit: clarify comment & match style guide. maybe something like "Construct
<foo>.
> Must be initialized by calling <bar> before using."

Done.  Is there some other style violation here?

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.h:72:
base::WeakPtrFactory<ClientRequest> weak_factory_;
On 2015/05/08 02:17:26, mattm wrote:
> I guess the members should also be private and have accessors.

Sure, done.  My theory was that since it's a private inner-class, the scope of
code that can access them is small and contained to the inner+outer class.  I
also moved the whole class to the .cc file.

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.cc:478: //
SafeBrowsingProtocolManagerDelegate should override this to return NULL.
On 2015/05/08 02:17:26, mattm wrote:
> Should be "anything that isn't derived from LocalSafeBrowsingDatabaseManager",
> for the cast to work, right? not just any SafeBrowsingProtocolManagerDelegate.

Done.

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:485: static
LocalSafeBrowsingDatabaseManager* local_database_manager(
On 2015/05/08 02:17:26, mattm wrote:
> I don't think lower_underscore names apply to static functions. Maybe
> "LocalDatabaseManagerForService" ?

Done.

https://codereview.chromium.org/1110723002/diff/200001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1110723002/diff/200001/chrome/chrome_browser....
chrome/chrome_browser.gypi:2360:
'browser/renderer_host/safe_browsing_resource_throttle.cc',
On 2015/05/08 02:17:26, mattm wrote:
> I thought this still shouldn't be used for mobile(=2)?

Yes there are a few extra sources here that aren't needed for mode=2 (I expect
the compiler will remove the .objs that have no use).  To get rid them here,
I'll add them them in both mobile_extended_sources and full_sources.  I was
trying to avoid that duplication since it can lead to mistakes.

(The alternative is a fourth list,
'chrome_browser_safe_browsing_database_sources' or some such. )

https://codereview.chromium.org/1110723002/diff/200001/chrome/chrome_browser....
chrome/chrome_browser.gypi:2366:
'browser/safe_browsing/malware_details_cache.h',
On 2015/05/08 02:17:26, mattm wrote:
> sorting got switched around here?

Was missing "LC_COLLATE=C" on the sort. Fixed.

https://codereview.chromium.org/1110723002/diff/200001/chrome/chrome_browser....
chrome/chrome_browser.gypi:2378:
'browser/safe_browsing/safe_browsing_database.h',
On 2015/05/08 02:17:26, mattm wrote:
> not needed for 2 or 3, right?

Done.

https://codereview.chromium.org/1110723002/diff/200001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (left):

https://codereview.chromium.org/1110723002/diff/200001/chrome/chrome_tests.gy...
chrome/chrome_tests.gypi:2951:
'../testing/android/on_device_instrumentation.gyp:require_driver_apk',
On 2015/05/08 02:17:26, mattm wrote:
> was this intentional?

Nope. Looks like a rebase issue.  I must have picked it up when moving patches
back and forth between two git trees I have for testing (one is for mobile, one
for desktop). I haven't found a reliable way to use just one directory tree for
both since clank generally lags behind TOT and has a difference in directory
structure.  But it is a pain.

Fixed, and I looked through all the other files for similar changes (were none).

[mattm, 2015-05-12 07:00:41]

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/remote_database_manager.h (right):

https://codereview.chromium.org/1110723002/diff/200001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.h:26: // Need to initialize
before using.
On 2015/05/12 01:04:01, nparker wrote:
> On 2015/05/08 02:17:26, mattm wrote:
> > nit: clarify comment & match style guide. maybe something like "Construct
> <foo>.
> > Must be initialized by calling <bar> before using."
> 
> Done.  Is there some other style violation here?

Ah, no. I just meant the comment style.

https://codereview.chromium.org/1110723002/diff/260001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/android_safe_browsing_api_handler.h (right):

https://codereview.chromium.org/1110723002/diff/260001/chrome/browser/safe_br...
chrome/browser/safe_browsing/android_safe_browsing_api_handler.h:1: // Copyright
(c) 2015 The Chromium Authors. All rights reserved.
copyright in new files shouldn't have (c)
(https://www.chromium.org/developers/coding-style#TOC-File-headers)

https://codereview.chromium.org/1110723002/diff/260001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/remote_database_manager.cc (right):

https://codereview.chromium.org/1110723002/diff/260001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.cc:159:
std::vector<SBThreatType> threat_types{SB_THREAT_TYPE_URL_MALWARE};
C++11 uniform initialization syntax isn't allowed yet (see
https://chromium-cpp.appspot.com/)

[Nathan Parker, 2015-05-12 18:23:20]

Done.

https://codereview.chromium.org/1110723002/diff/260001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/android_safe_browsing_api_handler.h (right):

https://codereview.chromium.org/1110723002/diff/260001/chrome/browser/safe_br...
chrome/browser/safe_browsing/android_safe_browsing_api_handler.h:1: // Copyright
(c) 2015 The Chromium Authors. All rights reserved.
On 2015/05/12 07:00:41, mattm wrote:
> copyright in new files shouldn't have (c)
> (https://www.chromium.org/developers/coding-style#TOC-File-headers)

Done, here and elsewhere.

https://codereview.chromium.org/1110723002/diff/260001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/remote_database_manager.cc (right):

https://codereview.chromium.org/1110723002/diff/260001/chrome/browser/safe_br...
chrome/browser/safe_browsing/remote_database_manager.cc:159:
std::vector<SBThreatType> threat_types{SB_THREAT_TYPE_URL_MALWARE};
On 2015/05/12 07:00:41, mattm wrote:
> C++11 uniform initialization syntax isn't allowed yet (see
> https://chromium-cpp.appspot.com/)

Done.

[mattm, 2015-05-12 20:34:15]

lgtm

[Nathan Parker, 2015-05-12 22:14:41]

thakis@ -- Can you review the following files?  Thanks.

 chrome/browser/extensions/fake_safe_browsing_database_manager.h
 chrome/browser/extensions/fake_safe_browsing_database_manager.cc
 chrome/browser/prerender/prerender_browsertest.cc
 chrome/browser/profiles/profile_impl.cc
 chrome/browser/renderer_host/safe_browsing_resource_throttle.cc
 chrome/browser/renderer_host/safe_browsing_resource_throttle_factory.h
 chrome/browser/renderer_host/safe_browsing_resource_throttle_factory.cc

 -- Nathan

[Nico, 2015-05-13 00:07:23]

lgtm except that I don't understand the finch changes. (feng, who added them, is
no longer with the project I think, else I would've added him)

https://codereview.chromium.org/1110723002/diff/280001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl.cc (left):

https://codereview.chromium.org/1110723002/diff/280001/chrome/browser/profile...
chrome/browser/profiles/profile_impl.cc:839: // for users enrolled in Finch
trial before.
Why do you no longer need the field trial?

https://codereview.chromium.org/1110723002/diff/280001/chrome/browser/rendere...
File chrome/browser/renderer_host/safe_browsing_resource_throttle.cc (left):

https://codereview.chromium.org/1110723002/diff/280001/chrome/browser/rendere...
chrome/browser/renderer_host/safe_browsing_resource_throttle.cc:98: #endif
Same question

[Nathan Parker, 2015-05-13 00:11:23]

Thanks.

https://codereview.chromium.org/1110723002/diff/280001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl.cc (left):

https://codereview.chromium.org/1110723002/diff/280001/chrome/browser/profile...
chrome/browser/profiles/profile_impl.cc:839: // for users enrolled in Finch
trial before.
On 2015/05/13 00:07:23, Nico wrote:
> Why do you no longer need the field trial?

The field trial was used last Fall to test full Safe Browsing on mobile, to
collect data for the code that's going in now.  We no longer need that trial,
and in fact the IsEnabledByFieldTrial() was removed a while ago.  Dead code.

Feng and I discussed the state of it before he left Google.

https://codereview.chromium.org/1110723002/diff/280001/chrome/browser/rendere...
File chrome/browser/renderer_host/safe_browsing_resource_throttle.cc (left):

https://codereview.chromium.org/1110723002/diff/280001/chrome/browser/rendere...
chrome/browser/renderer_host/safe_browsing_resource_throttle.cc:98: #endif
On 2015/05/13 00:07:23, Nico wrote:
> Same question

Acknowledged.

[Nathan Parker, 2015-05-13 00:11:50]

The CQ bit was checked by nparker@chromium.org

[Nathan Parker, 2015-05-13 00:11:51]

The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/1110723002/#ps280001
(title: "Respond to review.  Tweak comments and list initializer.")

[commit-bot: I haz the power, 2015-05-13 00:13:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1110723002/280001

[commit-bot: I haz the power, 2015-05-13 01:12:19]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-05-13 01:13:20]

Patchset 15 (id:??) landed as
https://crrev.com/b2cd5eef530be7100274af882af3f5aff4d1c4be
Cr-Commit-Position: refs/heads/master@{#329551}