Destroy store on task runner. That's where all store operations happen.

Destroy store on task runner. That's where all store operations happen.

BUG=656215, 656005
Committed: https://crrev.com/b8eecb024bdb6569e20e5fd93af9027a3a6a75ee
Cr-Commit-Position: refs/heads/master@{#426077}

[vakh (use Gerrit instead), 2016-10-18 01:51:08]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 01:51:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2016-10-18 01:51:50]

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

[Scott Hess - ex-Googler, 2016-10-18 02:57:01]

LGTM, minor suggestion you can ignore if you don't agree.

https://codereview.chromium.org/2425703004/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/2425703004/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.cc:146: (*store_map_)[identifier] =
std::move(new_store);
Does this work?

I mean, I think it probably does, but I'm having troubles describing how.  I'll
just throw this out there, but leave it in your hands whether you want to go
there:

std::swap((*store_map_)[identifier], new_store);
if (new_store) {
  V4Store::Destroy(std::move(new_store));
}

Of course, abusing new_store as temp storage like this is optional, and I
haven't read Destroy() yet to see if that's where the null handling should be.

https://codereview.chromium.org/2425703004/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_store.cc (right):

https://codereview.chromium.org/2425703004/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_store.cc:214: }
OK, yeah, same pattern as V4Database, makes sense.

[Scott Hess - ex-Googler, 2016-10-18 02:57:55]

https://codereview.chromium.org/2425703004/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/2425703004/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.cc:146: (*store_map_)[identifier] =
std::move(new_store);
On 2016/10/18 02:57:01, Scott Hess wrote:
> std::swap((*store_map_)[identifier], new_store);

Or, of course:

(*store_map_)[identifier].swap(new_store);

[commit-bot: I haz the power, 2016-10-18 03:16:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 03:16:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vakh (use Gerrit instead), 2016-10-18 20:10:27]

On 2016/10/18 02:57:55, Scott Hess wrote:
>
https://codereview.chromium.org/2425703004/diff/1/components/safe_browsing_db...
> File components/safe_browsing_db/v4_database.cc (right):
> 
>
https://codereview.chromium.org/2425703004/diff/1/components/safe_browsing_db...
> components/safe_browsing_db/v4_database.cc:146: (*store_map_)[identifier] =
> std::move(new_store);
> On 2016/10/18 02:57:01, Scott Hess wrote:
> > std::swap((*store_map_)[identifier], new_store);
> 
> Or, of course:
> 
> (*store_map_)[identifier].swap(new_store);

The next line, as you mentioned, would be:
V4Store::Destroy(std::move(new_store));

I agree that this approach looks attractive (fewer lines of code), but I think
calling Destroy on the new_store (which by then is the old store) seems
counter-intuitive. I feel that the current patch is more readable because the
reader doesn't see anything they don't expect.

[vakh (use Gerrit instead), 2016-10-18 20:11:21]

Description was changed from

==========
Destroy store on task runner. That's where all store operations happen.

BUG=656215, 656005, 543161
==========

to

==========
Destroy store on task runner. That's where all store operations happen.

BUG=656215, 656005
==========

[Scott Hess - ex-Googler, 2016-10-18 20:19:19]

On 2016/10/18 20:10:27, vakh (Varun Khaneja) wrote:
> On 2016/10/18 02:57:55, Scott Hess wrote:
> >
>
https://codereview.chromium.org/2425703004/diff/1/components/safe_browsing_db...
> > File components/safe_browsing_db/v4_database.cc (right):
> > 
> >
>
https://codereview.chromium.org/2425703004/diff/1/components/safe_browsing_db...
> > components/safe_browsing_db/v4_database.cc:146: (*store_map_)[identifier] =
> > std::move(new_store);
> > On 2016/10/18 02:57:01, Scott Hess wrote:
> > > std::swap((*store_map_)[identifier], new_store);
> > 
> > Or, of course:
> > 
> > (*store_map_)[identifier].swap(new_store);
> 
> The next line, as you mentioned, would be:
> V4Store::Destroy(std::move(new_store));
> 
> I agree that this approach looks attractive (fewer lines of code), but I think
> calling Destroy on the new_store (which by then is the old store) seems
> counter-intuitive. I feel that the current patch is more readable because the
> reader doesn't see anything they don't expect.

Yeah, maybe I'm just having cognitive dissonance WRT move semantics.  Having the
indeterminate state living in the store_map_ bothers me more than having it live
in a scoped variable, though they are the same thing in the end when exceptions
are disabled.

[vakh (use Gerrit instead), 2016-10-18 21:21:25]

shess@feedback. WaitForTasksOnTaskRunner at the end of the test that calls
ApplyUpdate and expects new stores since the old stores would get deleted later.

[vakh (use Gerrit instead), 2016-10-18 21:21:29]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 21:22:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2016-10-18 21:24:09]

On 2016/10/18 20:19:19, Scott Hess wrote:
> On 2016/10/18 20:10:27, vakh (Varun Khaneja) wrote:
> > On 2016/10/18 02:57:55, Scott Hess wrote:
> > >
> >
>
https://codereview.chromium.org/2425703004/diff/1/components/safe_browsing_db...
> > > File components/safe_browsing_db/v4_database.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2425703004/diff/1/components/safe_browsing_db...
> > > components/safe_browsing_db/v4_database.cc:146: (*store_map_)[identifier]
=
> > > std::move(new_store);
> > > On 2016/10/18 02:57:01, Scott Hess wrote:
> > > > std::swap((*store_map_)[identifier], new_store);
> > > 
> > > Or, of course:
> > > 
> > > (*store_map_)[identifier].swap(new_store);
> > 
> > The next line, as you mentioned, would be:
> > V4Store::Destroy(std::move(new_store));
> > 
> > I agree that this approach looks attractive (fewer lines of code), but I
think
> > calling Destroy on the new_store (which by then is the old store) seems
> > counter-intuitive. I feel that the current patch is more readable because
the
> > reader doesn't see anything they don't expect.
> 
> Yeah, maybe I'm just having cognitive dissonance WRT move semantics.  Having
the
> indeterminate state living in the store_map_ bothers me more than having it
live
> in a scoped variable, though they are the same thing in the end when
exceptions
> are disabled.

Fair enough. I'm convinced.
I've uploaded a new patch. It also fixes the asan failures.
Thanks.

[vakh (use Gerrit instead), 2016-10-18 21:24:20]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2016-10-18 21:24:21]

The patchset sent to the CQ was uploaded after l-g-t-m from shess@chromium.org
Link to the patchset: https://codereview.chromium.org/2425703004/#ps20001
(title: "shess@feedback. WaitForTasksOnTaskRunner at the end of the test that
calls ApplyUpdate and expects …")

[commit-bot: I haz the power, 2016-10-18 21:24:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 21:24:33]

This CL has an open dependency (Issue 2427863002 Patch 20001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[vakh (use Gerrit instead), 2016-10-18 21:32:24]

The CQ bit was checked by vakh@chromium.org

[commit-bot: I haz the power, 2016-10-18 21:32:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 23:06:54]

Description was changed from

==========
Destroy store on task runner. That's where all store operations happen.

BUG=656215, 656005
==========

to

==========
Destroy store on task runner. That's where all store operations happen.

BUG=656215, 656005
==========

[commit-bot: I haz the power, 2016-10-18 23:06:56]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-21 13:04:18]

Description was changed from

==========
Destroy store on task runner. That's where all store operations happen.

BUG=656215, 656005
==========

to

==========
Destroy store on task runner. That's where all store operations happen.

BUG=656215, 656005

Committed: https://crrev.com/b8eecb024bdb6569e20e5fd93af9027a3a6a75ee
Cr-Commit-Position: refs/heads/master@{#426077}
==========

[commit-bot: I haz the power, 2016-10-21 13:04:19]

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