Initialize and reset V4LocalDBManager. Instantiate V4Stores.

Initialize and reset V4LocalDBManager. Instantiate V4Stores.

A rough sketch of how this works is here: https://goto.google.com/pver4-LoadUpdateDatabase-doc

BUG=543161
Committed: https://crrev.com/32b9468ab5177534cb155b2d222611a6bca16c98
Cr-Commit-Position: refs/heads/master@{#393719}

[vakh (use Gerrit instead), 2016-05-07 02:13:57]

vakh@chromium.org changed reviewers:
+ nparker@chromium.org, shess@chromium.org

[vakh (use Gerrit instead), 2016-05-07 02:13:57]

shess@chromium.org: Please review changes in 
*

nparker@chromium.org: Please review changes in 
*

[vakh (use Gerrit instead), 2016-05-09 18:12:59]

Add v4_store.cc to gypi and update a comment

[Scott Hess - ex-Googler, 2016-05-09 19:40:39]

https://codereview.chromium.org/1954393002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/1954393002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:21: }
DCHECK() and closing brace should be undented 4 spaces.

https://codereview.chromium.org/1954393002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:237: closing_database_
= true;
Before I dig in and ponder things like "OMG, can't we just kill this code,
rather than propagating it?", I want to ask whether we're in a situation where
this code is the only plausible way to make progress, or if it's just happening
because it's being copied and pasted from elsewhere?

I think this basic approach has lived in this code (obviously not this
_specific_ code) for many many years.  Periodically there will be a change
resulting a a contentious review where we worked our race conditions and papered
over them, but I've often not found myself entirely convinced that the approach
was a good one.  What's putting me back in that mind is the amount of comments
explaining things, plus pieces which seem racy (here closing_database_ is being
set without a lock, SetupDatabase() has two different locking scopes, I can
probably find more).  So if this is only of this form "just because that's how
it was before", then maybe now would be a good time to think on approaches which
would be more obviously correct without a bunch of explanations and assertions.

But if there's a higher-level reason why other approaches would be unacceptable,
then stop me before I spend time thinking about it :-).

[Scott Hess - ex-Googler, 2016-05-09 20:28:09]

https://codereview.chromium.org/1954393002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/1954393002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:237: closing_database_
= true;
On 2016/05/09 19:40:39, Scott Hess wrote:
> Before I dig in and ponder things like "OMG, can't we just kill this code,
> rather than propagating it?", I want to ask whether we're in a situation where
> this code is the only plausible way to make progress, or if it's just
happening
> because it's being copied and pasted from elsewhere?

Too late!  Already pondered!

The original source for this code references database_ for two reasons.  There
is a set of thread-safe methods which can be called where database_ uses
internal locking.  Then there are the other methods which need to be async on
another thread.  The entire handle-setting part does _not_ really need all of
this cross-thread stuff.

Due to the consistent task_runner_, this class can assume that once it posts
"Close the database" to the background thread, it could simply stop using the
thread-safe handle.  Then this code could have setup like:

 - Post a setup task to the background thread.
 - Background thread does setup and posts a completion task to originating
thread.
 - Originating thread's task gets thread-safe handle as a parameter and stores
it with no locking.

teardown could be like:

 - Post a teardown task to the background thread.
 - Immediately forget thread-safe handle, no locking needed.
 - If setup is called here, a new background setup task will post after the
posted teardown task.

The foreground code can still track "Was the setup task posted already?" to
prevent creation of multiple database_ handles.

[vakh (use Gerrit instead), 2016-05-09 20:32:25]

https://codereview.chromium.org/1954393002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/1954393002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:237: closing_database_
= true;
Scott, you're right that some of this code, such as locking and
closing_database_, are pieces from existing code because it seems
to make sense why it would be done.

That being said, the re-write does aim to remove things that are unneeded today
or are inapplicable with the new protocol so I am going to keep this simple and
remove the locks for now. I can always add them later if needed.

On 2016/05/09 19:40:39, Scott Hess wrote:
> Before I dig in and ponder things like "OMG, can't we just kill this code,
> rather than propagating it?", I want to ask whether we're in a situation where
> this code is the only plausible way to make progress, or if it's just
happening
> because it's being copied and pasted from elsewhere?
> 
> I think this basic approach has lived in this code (obviously not this
> _specific_ code) for many many years.  Periodically there will be a change
> resulting a a contentious review where we worked our race conditions and
papered
> over them, but I've often not found myself entirely convinced that the
approach
> was a good one.  What's putting me back in that mind is the amount of comments
> explaining things, plus pieces which seem racy (here closing_database_ is
being
> set without a lock, SetupDatabase() has two different locking scopes, I can
> probably find more).  So if this is only of this form "just because that's how
> it was before", then maybe now would be a good time to think on approaches
which
> would be more obviously correct without a bunch of explanations and
assertions.
> 
> But if there's a higher-level reason why other approaches would be
unacceptable,
> then stop me before I spend time thinking about

[vakh (use Gerrit instead), 2016-05-09 20:34:30]

https://codereview.chromium.org/1954393002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/1954393002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:237: closing_database_
= true;
On 2016/05/09 20:28:08, Scott Hess wrote:
> On 2016/05/09 19:40:39, Scott Hess wrote:
> > Before I dig in and ponder things like "OMG, can't we just kill this code,
> > rather than propagating it?", I want to ask whether we're in a situation
where
> > this code is the only plausible way to make progress, or if it's just
> happening
> > because it's being copied and pasted from elsewhere?
> 
> Too late!  Already pondered!
> 
> The original source for this code references database_ for two reasons.  There
> is a set of thread-safe methods which can be called where database_ uses
> internal locking.  Then there are the other methods which need to be async on
> another thread.  The entire handle-setting part does _not_ really need all of
> this cross-thread stuff.
> 
> Due to the consistent task_runner_, this class can assume that once it posts
> "Close the database" to the background thread, it could simply stop using the
> thread-safe handle.  Then this code could have setup like:
> 
>  - Post a setup task to the background thread.
>  - Background thread does setup and posts a completion task to originating
> thread.
>  - Originating thread's task gets thread-safe handle as a parameter and stores
> it with no locking.
> 
> teardown could be like:
> 
>  - Post a teardown task to the background thread.
>  - Immediately forget thread-safe handle, no locking needed.
>  - If setup is called here, a new background setup task will post after the
> posted teardown task.
> 
> The foreground code can still track "Was the setup task posted already?" to
> prevent creation of multiple database_ handles.

Sounds good. I'll look into this.
My understanding of threads in Chromium isn't fantastic so this is super useful.

[vakh (use Gerrit instead), 2016-05-11 01:45:06]

Remove locking. Use ownership and thread hopping to guarantee that no locks are
needed to access resources.

[vakh (use Gerrit instead), 2016-05-11 01:49:09]

Description was changed from

==========
Initialize and reset V4LocalDBManager. Instantiate V4Stores.

BUG=543161
==========

to

==========
Initialize and reset V4LocalDBManager. Instantiate V4Stores.

A rough sketch of how this works is here:
https://goto.google.com/pver4-LoadUpdateDatabase-doc

BUG=543161
==========

[vakh (use Gerrit instead), 2016-05-11 18:00:32]

Use #if so that code is not unreachable

[vakh (use Gerrit instead), 2016-05-11 18:03:19]

content/public/browser is a dependency of v4_database

[vakh (use Gerrit instead), 2016-05-11 19:57:02]

base::Time->Time and use kSafeBrowsingV4LocalDatabaseManagerEnabled always

[vakh (use Gerrit instead), 2016-05-11 20:01:27]

Thanks for your previous review. Please take another look.

Here's an early draft of how we use threads to update and query the database:
https://goto.google.com/pver4-loadupdatedatabase-doc

https://codereview.chromium.org/1954393002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/1954393002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:21: }
On 2016/05/09 19:40:39, Scott Hess wrote:
> DCHECK() and closing brace should be undented 4 spaces.

Done.

[vakh (use Gerrit instead), 2016-05-11 20:26:26]

No need to reset last_response_time_ and add DCHECK

[vakh (use Gerrit instead), 2016-05-11 20:43:49]

Minor: Remove unused variable

[Scott Hess - ex-Googler, 2016-05-11 22:23:57]

Hmm.  About 50/50 actual concerns versus style nits.  It's like a casino!

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_database.cc:39: V4Database* v4_database =
nullptr;
This doesn't need to be scoped above the if() unless you're going to use it
after the if()/else.  Just declare it in both places it is initialized.

Also, WDYT of making it a std::unique_ptr?  Then you can pass the ownership when
posting the task (base::Passed(&u_ptr)), and then base::Bind() will own the
reference.  That way if the posted task gets lost (for instance if the thread
goes away), the database won't leak.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_database.cc:63: callback.Run(v4_database);
Are you sure?  This is going to run the callback in the db_task_runner, as I
understand it.  Would it be possible to post the callback to the same place in
both cases?

I can see why the IO thread may not be right in tests.  But posting a task is
probably right in tests, to make sure your code is really testing that the
callbacks are async rather than synchronous.  You could post to the current
thread, but per my earlier comment, if you refactor the runs-in-background bit
of Create() into an implementation detail, another option would be to have the
CreateImpl() task post back to the thread Create() was called on.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_database.cc:82: reset_success = reset_success &&
store_reset_success;
A phrasing with fewer moving parts to read might be:
  if (!store_id_and_store.second->Reset())
    reset_success = false;

I'm sad that this isn't possible, though:
  reset_success &&= store_id_and_store.second->Reset();

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_database.h:49: // Runs on the task runner.
This seems to imply that Create() should be called on the task runner, but then
create also takes the task runner as a parameter.  I think what's happening is
that Create() is being called on the task runner and it's passing the task
runner down to owned objects.

Maybe this relationship could be more explicit and not under the influence of
the caller?  Right now the caller can call it on whatever thread it wants. 
Probably the easiest option would be to have the exposed API implemented on the
_current_ thread (not posted to task_runner_), and have the implementation post
the helper task to the background thread.

Also, in that case, the callback should be posted to the thread Create() was
originally called from.  That is probably the IO thread, of course.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_database.h:72: const
scoped_refptr<base::SequencedTaskRunner>& db_task_runner_;
I can see the const, if you can manage to construct it in the constructor, but
what does a const ref mean in this context?

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:21: << "base_path_: "
<< base_path_.value();
I'd suggest AsUTF8Unsafe() instead of value() for this.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:156: }
Is there any reason to setup this up out here, rather than inside
SetupDatabase()?  AFAICT that's the only user of task_runner_.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:209: void
V4LocalDatabaseManager::SwapDatabase(V4Database* v4_database) {
This isn't really a swap, more of a set.  Also, it schedules an update, so maybe
not even a set.  Maybe DatabaseReady() or similar?

This would be std::unique_ptr<> if you took my advice in V4Database::Create(),
and the reset() would become a swap() or =.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:211:
DCHECK(v4_update_protocol_manager_.get());
This DCHECK() seems redundant, given the explicit deref below.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:216: // The database is
in place. Start fetching updates now.
Is the DVLOG going to be here for the long term?  If so the comment is
redundant.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:228:
v4_database_.reset();
Either optional braces everywhere, or nowhere.  Chromium tends to lean nowhere. 
I tend to lean towards being consistent within a file.

[Though I'll admit that optional braces nowhere is easier than arguing with some
new reviewer who's being pedantic.]

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:234:
v4_update_protocol_manager_.reset();
Another optional braces to make consistent.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:243:
DCHECK(v4_update_protocol_manager_.get());
The deref will enforce this.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
File components/safe_browsing_db/v4_store.h (right):

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_store.h:17: const base::FilePath& store_path);
|store_path| is obvious, but what's |task_runner| going to be used for, here? 
Not so much for me, but for readers of the file :-).

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
File components/safe_browsing_db/v4_update_protocol_manager.cc (right):

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_update_protocol_manager.cc:132: // updates.
It's possible that at some point you'll get feedback on this style of comments. 
I generally write paragraph-style and use Emacs fill mode to reformat to 80
columns.  If it's two paragraphs, a blank line between, but if it's just one
thought, formatted as a whole rather than chopped into pieces.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_update_protocol_manager.cc:179: if (next <
base::TimeDelta()) {
This file also has a mix of some always-optional versus no-optional bracing. 
Per earlier comment, I'd lean towards no-optional braces.

Also, I'd expect std::max(next, base::TimeDelta()) to work for TimeDelta.

Hmm, and in the interest of options, this test is only here because the -= may
have sent next negative.  You might consider:
  if (callback_time < next) {
    next -= callback_time;
  } else {
    next = base::TimeDelta();
  }
Yeah, slightly longer, so probably not.

[vakh (use Gerrit instead), 2016-05-12 22:29:51]

CR feedback

[vakh (use Gerrit instead), 2016-05-12 22:30:20]

Thanks for the review. PTAL.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_database.cc:39: V4Database* v4_database =
nullptr;
On 2016/05/11 22:23:56, Scott Hess wrote:
> This doesn't need to be scoped above the if() unless you're going to use it
> after the if()/else.  Just declare it in both places it is initialized.
> 
> Also, WDYT of making it a std::unique_ptr?  Then you can pass the ownership
when
> posting the task (base::Passed(&u_ptr)), and then base::Bind() will own the
> reference.  That way if the posted task gets lost (for instance if the thread
> goes away), the database won't leak.

Done.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_database.cc:63: callback.Run(v4_database);
On 2016/05/11 22:23:56, Scott Hess wrote:
> Are you sure?  This is going to run the callback in the db_task_runner, as I
> understand it.  Would it be possible to post the callback to the same place in
> both cases?
> 
> I can see why the IO thread may not be right in tests.  But posting a task is
> probably right in tests, to make sure your code is really testing that the
> callbacks are async rather than synchronous.  You could post to the current
> thread, but per my earlier comment, if you refactor the runs-in-background bit
> of Create() into an implementation detail, another option would be to have the
> CreateImpl() task post back to the thread Create() was called on.

Done.
TBH, I hadn't thought too much about how to test it. I was going to focus on
that when I implement the skeleton methods. But I completely agree that we
should post a task to test async behavior.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_database.cc:82: reset_success = reset_success &&
store_reset_success;
On 2016/05/11 22:23:56, Scott Hess wrote:
> A phrasing with fewer moving parts to read might be:
>   if (!store_id_and_store.second->Reset())
>     reset_success = false;
> 
> I'm sad that this isn't possible, though:
>   reset_success &&= store_id_and_store.second->Reset();

Done.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_database.h:49: // Runs on the task runner.
On 2016/05/11 22:23:56, Scott Hess wrote:
> This seems to imply that Create() should be called on the task runner, but
then
> create also takes the task runner as a parameter.  I think what's happening is
> that Create() is being called on the task runner and it's passing the task
> runner down to owned objects.
> 
> Maybe this relationship could be more explicit and not under the influence of
> the caller?  Right now the caller can call it on whatever thread it wants. 
> Probably the easiest option would be to have the exposed API implemented on
the
> _current_ thread (not posted to task_runner_), and have the implementation
post
> the helper task to the background thread.
> 
Done.

> Also, in that case, the callback should be posted to the thread Create() was
> originally called from.  That is probably the IO thread, of course.

Is that necessary? For avoiding some sort of race condition?
As long as the caller and the callee understand that the callback will always be
on IO thread, it is probably OK. WDYT?

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_database.h:72: const
scoped_refptr<base::SequencedTaskRunner>& db_task_runner_;
On 2016/05/11 22:23:56, Scott Hess wrote:
> I can see the const, if you can manage to construct it in the constructor, but
> what does a const ref mean in this context?

Removed the ref.
As for the "const" -- it helps to make sure that it doesn't get assigned by
mistake. Is there a downside to adding const-ness?

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:21: << "base_path_: "
<< base_path_.value();
On 2016/05/11 22:23:56, Scott Hess wrote:
> I'd suggest AsUTF8Unsafe() instead of value() for this.

Done.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:156: }
On 2016/05/11 22:23:56, Scott Hess wrote:
> Is there any reason to setup this up out here, rather than inside
> SetupDatabase()?  AFAICT that's the only user of task_runner_.

Done.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:209: void
V4LocalDatabaseManager::SwapDatabase(V4Database* v4_database) {
On 2016/05/11 22:23:56, Scott Hess wrote:
> This isn't really a swap, more of a set.  Also, it schedules an update, so
maybe
> not even a set.  Maybe DatabaseReady() or similar?
> 
Done

> This would be std::unique_ptr<> if you took my advice in V4Database::Create(),
> and the reset() would become a swap() or =.
It could be a swap after-all :)

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:211:
DCHECK(v4_update_protocol_manager_.get());
On 2016/05/11 22:23:56, Scott Hess wrote:
> This DCHECK() seems redundant, given the explicit deref below.

Done.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:216: // The database is
in place. Start fetching updates now.
On 2016/05/11 22:23:56, Scott Hess wrote:
> Is the DVLOG going to be here for the long term?  If so the comment is
> redundant.

Done.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:228:
v4_database_.reset();
On 2016/05/11 22:23:56, Scott Hess wrote:
> Either optional braces everywhere, or nowhere.  Chromium tends to lean
nowhere. 
> I tend to lean towards being consistent within a file.
> 
> [Though I'll admit that optional braces nowhere is easier than arguing with
some
> new reviewer who's being pedantic.]

I always add optional braces (which is what I have done on this file now) but I
would be willing to remove them all if you feel strongly about no optional
braces.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:234:
v4_update_protocol_manager_.reset();
On 2016/05/11 22:23:56, Scott Hess wrote:
> Another optional braces to make consistent.

Done.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:243:
DCHECK(v4_update_protocol_manager_.get());
On 2016/05/11 22:23:56, Scott Hess wrote:
> The deref will enforce this.

Done.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
File components/safe_browsing_db/v4_store.h (right):

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_store.h:17: const base::FilePath& store_path);
On 2016/05/11 22:23:57, Scott Hess wrote:
> |store_path| is obvious, but what's |task_runner| going to be used for, here? 
> Not so much for me, but for readers of the file :-).

Done.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
File components/safe_browsing_db/v4_update_protocol_manager.cc (right):

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_update_protocol_manager.cc:132: // updates.
On 2016/05/11 22:23:57, Scott Hess wrote:
> It's possible that at some point you'll get feedback on this style of
comments. 
> I generally write paragraph-style and use Emacs fill mode to reformat to 80
> columns.  If it's two paragraphs, a blank line between, but if it's just one
> thought, formatted as a whole rather than chopped into pieces.

Done.

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_update_protocol_manager.cc:179: if (next <
base::TimeDelta()) {
On 2016/05/11 22:23:57, Scott Hess wrote:
> This file also has a mix of some always-optional versus no-optional bracing. 
> Per earlier comment, I'd lean towards no-optional braces.
> 
Done, but with all optional braces.

> Also, I'd expect std::max(next, base::TimeDelta()) to work for TimeDelta.
> 
> Hmm, and in the interest of options, this test is only here because the -= may
> have sent next negative.  You might consider:
>   if (callback_time < next) {
>     next -= callback_time;
>   } else {
>     next = base::TimeDelta();
>   }
> Yeah, slightly longer, so probably not.
I like this more because it makes the reason for this logic very explicit.

[Scott Hess - ex-Googler, 2016-05-12 23:18:00]

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_database.cc:63: callback.Run(v4_database);
On 2016/05/12 22:30:20, vakh wrote:
> On 2016/05/11 22:23:56, Scott Hess wrote:
> > Are you sure?  This is going to run the callback in the db_task_runner, as I
> > understand it.  Would it be possible to post the callback to the same place
in
> > both cases?
> > 
> > I can see why the IO thread may not be right in tests.  But posting a task
is
> > probably right in tests, to make sure your code is really testing that the
> > callbacks are async rather than synchronous.  You could post to the current
> > thread, but per my earlier comment, if you refactor the runs-in-background
bit
> > of Create() into an implementation detail, another option would be to have
the
> > CreateImpl() task post back to the thread Create() was called on.
> 
> Done.
> TBH, I hadn't thought too much about how to test it. I was going to focus on
> that when I implement the skeleton methods. But I completely agree that we
> should post a task to test async behavior.

Might be worthwhile thinking about how to test it, just because otherwise you'll
find yourself having to decide between refactoring to make it testable, or
inadequately testing things.  [Been there, done that!]

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_database.h:49: // Runs on the task runner.
On 2016/05/12 22:30:20, vakh wrote:
> On 2016/05/11 22:23:56, Scott Hess wrote:
> > Also, in that case, the callback should be posted to the thread Create() was
> > originally called from.  That is probably the IO thread, of course.
> 
> Is that necessary? For avoiding some sort of race condition?
> As long as the caller and the callee understand that the callback will always
be
> on IO thread, it is probably OK. WDYT?

It's probably OK, but I think it implies a global understanding of things which
isn't needed.  To my mind, if a client makes a call like this:

  Create(task_runner, base_path, list_info_map, callback);

then the most sensible/obvious place for |callback| to run is in the thread that
Create() is being called in.  If |callback| should run elsewhere, then maybe it
should specify it by:

  Create(task_runner, base_path, list_info_map, callback_task_runner, callback);

Having it default to the IO thread means that you're designing particular design
assumptions into the API.  Sometimes, that's really the only choice, of course.

In the original version of this code, you required Create() itself to run on the
db_task_runner, which meant that you really were forced to make an assumption
about which thread the |callback| came back on.  But in the new version, having
it come back on the task runner which called Create() is reasonable.  And then
you don't have to document the assumption.

[The easiest way to do something like this is PostTaskAndReply(), otherwise you
can pass something like base::MessageLoop::current()->task_runner() down to the
inner CreateOnTaskRunner().]

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_database.h:72: const
scoped_refptr<base::SequencedTaskRunner>& db_task_runner_;
On 2016/05/12 22:30:20, vakh wrote:
> On 2016/05/11 22:23:56, Scott Hess wrote:
> > I can see the const, if you can manage to construct it in the constructor,
but
> > what does a const ref mean in this context?
> 
> Removed the ref.
> As for the "const" -- it helps to make sure that it doesn't get assigned by
> mistake. Is there a downside to adding const-ness?

Yep!  That's why I said I can see where you were going with the const (often you
simply can't get there due to how the containing object is constructed).

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:228:
v4_database_.reset();
On 2016/05/12 22:30:20, vakh wrote:
> On 2016/05/11 22:23:56, Scott Hess wrote:
> > Either optional braces everywhere, or nowhere.  Chromium tends to lean
> nowhere. 
> > I tend to lean towards being consistent within a file.
> > 
> > [Though I'll admit that optional braces nowhere is easier than arguing with
> some
> > new reviewer who's being pedantic.]
> 
> I always add optional braces (which is what I have done on this file now) but
I
> would be willing to remove them all if you feel strongly about no optional
> braces.

I prefer always having the optional braces myself, BUT I think Chromium
generally prefers no optional braces.  So mostly I stick with "be consistent
above all else", so if the file already has optional braces, have them, if it
omits the optional braces, omit them.

https://codereview.chromium.org/1954393002/diff/160001/components/safe_browsi...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/1954393002/diff/160001/components/safe_browsi...
components/safe_browsing_db/v4_database.cc:77: base::Bind(callback,
base::Passed(&v4_database)));
This seems odd to me.  In some cases, tests setup a fake task runner where
everything actually runs on the same thread, in which case this works.  But if
the test is actually setting up a real thread for db_task_runner, then this is
posting the callback to that thread, which might differ from the IO thread in
that case.  If the test is directly testing this code, that is probably alright,
but if the test is sitting above a client of this code, then it could easily be
breaking threading assumptions made by that client.

WRT my suggestion about posting back to the task-runner that the original
Create() came from, I think this is all fine (the test case and the production
case will get the callback in the right logical place).  If you really don't
want to go that way, though, I think you need the Create() to accept a
callback_task_runner, so that where the callback runs is under control of the
client.

[Of course, if this ends up with a callback_task_runner parameter, then it's
going to seem like the PostTask() and v4_database declaration should be outside
the if() block.]

https://codereview.chromium.org/1954393002/diff/160001/components/safe_browsi...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/1954393002/diff/160001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:227: }
With this and the one below, is there any value to checking before reset()?  If
it's already empty, the reset() is going to be awful cheap.

https://codereview.chromium.org/1954393002/diff/160001/components/safe_browsi...
File components/safe_browsing_db/v4_update_protocol_manager.cc (right):

https://codereview.chromium.org/1954393002/diff/160001/components/safe_browsi...
components/safe_browsing_db/v4_update_protocol_manager.cc:280:
GetBase64SerializedUpdateRequestProto(current_list_states_);
I generally appreciate this kind of formatting.  As long as you're going to be
multiple lines, might as well scope concepts together cleanly rather than
line-breaking in the middle.

[vakh (use Gerrit instead), 2016-05-13 00:02:38]

CR feedback: call NewDBReadyCallback on the same thread

[vakh (use Gerrit instead), 2016-05-13 00:07:32]

v4_db declared outside if()

[vakh (use Gerrit instead), 2016-05-13 00:07:32]

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/1954393002/diff/140001/components/safe_browsi...
components/safe_browsing_db/v4_database.cc:63: callback.Run(v4_database);
On 2016/05/12 23:18:00, Scott Hess wrote:
> On 2016/05/12 22:30:20, vakh wrote:
> > On 2016/05/11 22:23:56, Scott Hess wrote:
> > > Are you sure?  This is going to run the callback in the db_task_runner, as
I
> > > understand it.  Would it be possible to post the callback to the same
place
> in
> > > both cases?
> > > 
> > > I can see why the IO thread may not be right in tests.  But posting a task
> is
> > > probably right in tests, to make sure your code is really testing that the
> > > callbacks are async rather than synchronous.  You could post to the
current
> > > thread, but per my earlier comment, if you refactor the runs-in-background
> bit
> > > of Create() into an implementation detail, another option would be to have
> the
> > > CreateImpl() task post back to the thread Create() was called on.
> > 
> > Done.
> > TBH, I hadn't thought too much about how to test it. I was going to focus on
> > that when I implement the skeleton methods. But I completely agree that we
> > should post a task to test async behavior.
> 
> Might be worthwhile thinking about how to test it, just because otherwise
you'll
> find yourself having to decide between refactoring to make it testable, or
> inadequately testing things.  [Been there, done that!]

Acknowledged. Absolutely agree.

https://codereview.chromium.org/1954393002/diff/160001/components/safe_browsi...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/1954393002/diff/160001/components/safe_browsi...
components/safe_browsing_db/v4_database.cc:77: base::Bind(callback,
base::Passed(&v4_database)));
On 2016/05/12 23:18:00, Scott Hess wrote:
> This seems odd to me.  In some cases, tests setup a fake task runner where
> everything actually runs on the same thread, in which case this works.  But if
> the test is actually setting up a real thread for db_task_runner, then this is
> posting the callback to that thread, which might differ from the IO thread in
> that case.  If the test is directly testing this code, that is probably
alright,
> but if the test is sitting above a client of this code, then it could easily
be
> breaking threading assumptions made by that client.
> 
> WRT my suggestion about posting back to the task-runner that the original
> Create() came from, I think this is all fine (the test case and the production
> case will get the callback in the right logical place).  If you really don't
> want to go that way, though, I think you need the Create() to accept a
> callback_task_runner, so that where the callback runs is under control of the
> client.
> 
> [Of course, if this ends up with a callback_task_runner parameter, then it's
> going to seem like the PostTask() and v4_database declaration should be
outside
> the if() block.]

Done.

https://codereview.chromium.org/1954393002/diff/160001/components/safe_browsi...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/1954393002/diff/160001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:227: }
On 2016/05/12 23:18:00, Scott Hess wrote:
> With this and the one below, is there any value to checking before reset()? 
If
> it's already empty, the reset() is going to be awful cheap.

I think I saw other places in codesearch that were doing this and assumed that
that's the "recommended" way. Changed.

https://codereview.chromium.org/1954393002/diff/160001/components/safe_browsi...
File components/safe_browsing_db/v4_update_protocol_manager.cc (right):

https://codereview.chromium.org/1954393002/diff/160001/components/safe_browsi...
components/safe_browsing_db/v4_update_protocol_manager.cc:280:
GetBase64SerializedUpdateRequestProto(current_list_states_);
On 2016/05/12 23:18:00, Scott Hess wrote:
> I generally appreciate this kind of formatting.  As long as you're going to be
> multiple lines, might as well scope concepts together cleanly rather than
> line-breaking in the middle.

Acknowledged. vim auto format :)

[Scott Hess - ex-Googler, 2016-05-13 17:58:56]

The test-writing comment is just informational, though a few tests should be on
your near-term radar (like next CL).

https://codereview.chromium.org/1954393002/diff/200001/components/safe_browsi...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/1954393002/diff/200001/components/safe_browsi...
components/safe_browsing_db/v4_database.cc:32: // the NewDatabaseReadyCallback
on the same thread as it was called.
I'd stick with just the header-file comment.  Having the comment in two places
means that it's likely to bitrot.  Similar for CreateOnTaskRunner().  The
"static" comment should stay, because that's actually noting an attribute of the
method which is non-obvious (though I've never found it helpful, myself).

https://codereview.chromium.org/1954393002/diff/200001/components/safe_browsi...
components/safe_browsing_db/v4_database.cc:38: const
scoped_refptr<base::SingleThreadTaskRunner> callback_task_runner =
Can this be a const ref?  If I understand things correctly, this will AddRef(),
then Bind() will AddRef(), then end of scope will Release().  Then making this a
const ref will drop the outer AddRef()/Release().

https://codereview.chromium.org/1954393002/diff/200001/components/safe_browsi...
components/safe_browsing_db/v4_database.cc:64: const base::FilePath::CharType
suffix = list_info.second;
Another nice thing about having minimal tests is that they'd probably directly
answer questions like "Was this intended to be a single char?"

https://codereview.chromium.org/1954393002/diff/200001/components/safe_browsi...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/1954393002/diff/200001/components/safe_browsi...
components/safe_browsing_db/v4_database.h:60: StoreMap store_map);
OK, sort of weird question which didn't occur to me earlier - you have Create()
for purposes of creating instances, so should the constructor be private?  That
pattern is frequently used in Chromium.

[Or perhaps it needs to be protected: so that you can subclass for the factory.]

https://codereview.chromium.org/1954393002/diff/200001/components/safe_browsi...
components/safe_browsing_db/v4_database.h:70: }
Thinking about the constructor being exposed made me step back and think about
the virtual methods and this and V4DatabaseFactory.  Many people would argue
that you should have V4DatabaseFactory and the supporting code in place until
you have tests which use them, because they'll just be dead code you have to
deal with and which will probably not work right when you do first implement it.
 The obvious resolutions of that are "Remove the dead code for now" or "Write a
test which at least exercises the factory-ness".  In the interests of not
dragging this specific code review on forever, I am fine with not doing anything
now, but IMHO you really should land some minimal tests as the next step.

[vakh (use Gerrit instead), 2016-05-13 23:11:02]

https://codereview.chromium.org/1954393002/diff/200001/components/safe_browsi...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/1954393002/diff/200001/components/safe_browsi...
components/safe_browsing_db/v4_database.cc:32: // the NewDatabaseReadyCallback
on the same thread as it was called.
On 2016/05/13 17:58:56, Scott Hess wrote:
> I'd stick with just the header-file comment.  Having the comment in two places
> means that it's likely to bitrot.  Similar for CreateOnTaskRunner().  The
> "static" comment should stay, because that's actually noting an attribute of
the
> method which is non-obvious (though I've never found it helpful, myself).

Done.

https://codereview.chromium.org/1954393002/diff/200001/components/safe_browsi...
components/safe_browsing_db/v4_database.cc:38: const
scoped_refptr<base::SingleThreadTaskRunner> callback_task_runner =
On 2016/05/13 17:58:55, Scott Hess wrote:
> Can this be a const ref?  If I understand things correctly, this will
AddRef(),
> then Bind() will AddRef(), then end of scope will Release().  Then making this
a
> const ref will drop the outer AddRef()/Release().

Done.

https://codereview.chromium.org/1954393002/diff/200001/components/safe_browsi...
components/safe_browsing_db/v4_database.cc:64: const base::FilePath::CharType
suffix = list_info.second;
On 2016/05/13 17:58:56, Scott Hess wrote:
> Another nice thing about having minimal tests is that they'd probably directly
> answer questions like "Was this intended to be a single char?"

Acknowledged.

https://codereview.chromium.org/1954393002/diff/200001/components/safe_browsi...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/1954393002/diff/200001/components/safe_browsi...
components/safe_browsing_db/v4_database.h:60: StoreMap store_map);
On 2016/05/13 17:58:56, Scott Hess wrote:
> OK, sort of weird question which didn't occur to me earlier - you have
Create()
> for purposes of creating instances, so should the constructor be private? 
That
> pattern is frequently used in Chromium.
> 
> [Or perhaps it needs to be protected: so that you can subclass for the
factory.]

Done.

https://codereview.chromium.org/1954393002/diff/200001/components/safe_browsi...
components/safe_browsing_db/v4_database.h:70: }
On 2016/05/13 17:58:56, Scott Hess wrote:
> Thinking about the constructor being exposed made me step back and think about
> the virtual methods and this and V4DatabaseFactory.  Many people would argue
> that you should have V4DatabaseFactory and the supporting code in place until
> you have tests which use them, because they'll just be dead code you have to
> deal with and which will probably not work right when you do first implement
it.
>  The obvious resolutions of that are "Remove the dead code for now" or "Write
a
> test which at least exercises the factory-ness".  In the interests of not
> dragging this specific code review on forever, I am fine with not doing
anything
> now, but IMHO you really should land some minimal tests as the next step.

Acknowledged.

[vakh (use Gerrit instead), 2016-05-13 23:11:06]

CR feedback

[Scott Hess - ex-Googler, 2016-05-13 23:14:37]

LGTM!

[Nathan Parker, 2016-05-14 00:43:41]

lgtm

https://codereview.chromium.org/1954393002/diff/220001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/services_delegate_impl.cc (right):

https://codereview.chromium.org/1954393002/diff/220001/chrome/browser/safe_br...
chrome/browser/safe_browsing/services_delegate_impl.cc:189: #ifndef NDEBUG
Do you want to keep this?

https://codereview.chromium.org/1954393002/diff/220001/components/safe_browsi...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/1954393002/diff/220001/components/safe_browsi...
components/safe_browsing_db/v4_database.cc:84: V4Database::~V4Database() {}
Check that this is on the correct thread.

https://codereview.chromium.org/1954393002/diff/220001/components/safe_browsi...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/1954393002/diff/220001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:132: if (!enabled_) {
I assume that if v4_database_ == null, we'll queue requests.

[vakh (use Gerrit instead), 2016-05-14 01:05:38]

The CQ bit was checked by vakh@chromium.org

[commit-bot: I haz the power, 2016-05-14 01:06:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1954393002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1954393002/220001

[commit-bot: I haz the power, 2016-05-14 01:10:07]

Description was changed from

==========
Initialize and reset V4LocalDBManager. Instantiate V4Stores.

A rough sketch of how this works is here:
https://goto.google.com/pver4-LoadUpdateDatabase-doc

BUG=543161
==========

to

==========
Initialize and reset V4LocalDBManager. Instantiate V4Stores.

A rough sketch of how this works is here:
https://goto.google.com/pver4-LoadUpdateDatabase-doc

BUG=543161
==========

[commit-bot: I haz the power, 2016-05-14 01:10:08]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-05-14 01:11:04]

Description was changed from

==========
Initialize and reset V4LocalDBManager. Instantiate V4Stores.

A rough sketch of how this works is here:
https://goto.google.com/pver4-LoadUpdateDatabase-doc

BUG=543161
==========

to

==========
Initialize and reset V4LocalDBManager. Instantiate V4Stores.

A rough sketch of how this works is here:
https://goto.google.com/pver4-LoadUpdateDatabase-doc

BUG=543161

Committed: https://crrev.com/32b9468ab5177534cb155b2d222611a6bca16c98
Cr-Commit-Position: refs/heads/master@{#393719}
==========

[commit-bot: I haz the power, 2016-05-14 01:11:06]

Patchset 12 (id:??) landed as
https://crrev.com/32b9468ab5177534cb155b2d222611a6bca16c98
Cr-Commit-Position: refs/heads/master@{#393719}

[Ken Rockot(use gerrit already), 2016-05-15 00:30:58]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/1983603002/ by rockot@chromium.org.

The reason for reverting is: Seems very likely to be the cause of Mac build
failures:

https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20(dbg)
.

[vakh (use Gerrit instead), 2016-05-16 02:27:12]

On 2016/05/15 00:30:58, Ken Rockot wrote:
> A revert of this CL (patchset #12 id:220001) has been created in
> https://codereview.chromium.org/1983603002/ by mailto:rockot@chromium.org.
> 
> The reason for reverting is: Seems very likely to be the cause of Mac build
> failures:
> 
> https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20(dbg)
> .

Probably this:
https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%2...

[vakh (use Gerrit instead), 2016-05-16 22:04:12]

nparker@ -- I'm going to address your comments in
https://codereview.chromium.org/1980173003

https://codereview.chromium.org/1954393002/diff/220001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/services_delegate_impl.cc (right):

https://codereview.chromium.org/1954393002/diff/220001/chrome/browser/safe_br...
chrome/browser/safe_browsing/services_delegate_impl.cc:189: #ifndef NDEBUG
On 2016/05/14 00:43:41, Nathan Parker wrote:
> Do you want to keep this?

Yes, added a TODO until I figure out why it isn't showing in UMA.

https://codereview.chromium.org/1954393002/diff/220001/components/safe_browsi...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/1954393002/diff/220001/components/safe_browsi...
components/safe_browsing_db/v4_database.cc:84: V4Database::~V4Database() {}
On 2016/05/14 00:43:41, Nathan Parker wrote:
> Check that this is on the correct thread.

Done.

https://codereview.chromium.org/1954393002/diff/220001/components/safe_browsi...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/1954393002/diff/220001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:132: if (!enabled_) {
On 2016/05/14 00:43:41, Nathan Parker wrote:
> I assume that if v4_database_ == null, we'll queue requests.

Yes, will add that logic later.