SafeBrowsing PVer4: Send mutable response to the database and the stores

SafeBrowsing PVer4: Send mutable response to the database and the stores

V4Store now gets a unique pointer to a ListUpdateResponse, which it can modify. It swaps this ListUpdateResponse proto into the V4FileFormat proto before the V4FileFormat proto is written to disk.

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

[vakh (use Gerrit instead), 2016-06-28 01:33:10]

Description was changed from

==========
PVer4: Send mutable response to the database and the stores

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

to

==========
PVer4: Send mutable response to the database and the stores

V4Store now gets a unique pointer to a ListUpdateResponse, which it can modify.
It swaps this ListUpdateResponse proto into the V4FileFormat proto before the
V4FileFormat proto is written to disk.

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

[vakh (use Gerrit instead), 2016-06-28 01:33:11]

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

[Scott Hess - ex-Googler, 2016-06-28 03:58:44]

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

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.cc:108: for
(std::unique_ptr<ListUpdateResponse>& response :
I'm not entirely comfortable with the non-const &, here.  I'm not entirely sure
where the style guide covers this case, either to allow or disallow.  Does not
having it be a reference still steal the values as it iterates?  If so, I think
that would be preferable (as-is, I think the std::move(response) line actually
is where the value moves out of the collection).

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

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database_unittest.cc:134: new
ParsedServerResponse);
Is this your formatting or auto-formatting?  Once the line is broken, I
generally prefer to format like
  std::unique_ptr<ParsedServerResponse>
    parsed_server_response(new ParsedServerResponse);

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database_unittest.cc:245: callback_db_updated_);
Would the following work as:
  v4_database_->ApplyUpdate(
      CreateFakeServerResponse(expected_store_state_map_),
      callback_db_updated_);
???  Right now it feels like it's creating a long-winded temporary for no
particular purpose.

Otherwise, I see no reason not to use |response|.  There aren't other responses
here, so recapitulating the entire type doesn't make it clearer.

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_local_database_manager.h (right):

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_local_database_manager.h:66:
std::unique_ptr<ParsedServerResponse> parsed_server_response);
I'd say "responses" is still a reasonable argument name.

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_protocol_manager_util.h:61:
UpdateListIdentifier(const ListUpdateResponse*);
Should this be explicit?  The change to pointer makes me wonder if it could
match in new contexts.

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

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_store_unittest.cc:123: ListUpdateResponse*
list_update_response = new ListUpdateResponse;
This leaves the pointer unowned for a bit.  Instead this should be the
unique_ptr, with std::move() at the write.

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_store_unittest.cc:131: ListUpdateResponse*
list_update_response = new ListUpdateResponse;
Likewise here.

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

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_update_protocol_manager.cc:259:
ListUpdateResponse* add = new ListUpdateResponse(list_update_response);
Don't leave a gap where the pointer is raw, unique it as soon as created.

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_update_protocol_manager.h (right):

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_update_protocol_manager.h:45:
std::unique_ptr<ParsedServerResponse> parsed_server_response)>
Don't need the parameter's name in this context, just the type.

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

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_update_protocol_manager_unittest.cc:56:
std::unique_ptr<ListUpdateResponse>& actual =
const should still be appropriate for a reference.  The change _allows_
destructively rearranging the overall response, it doesn't require it.

[vakh (use Gerrit instead), 2016-06-28 07:16:10]

git pull

[Nathan Parker, 2016-06-28 19:00:38]

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

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.cc:108: for
(std::unique_ptr<ListUpdateResponse>& response :
On 2016/06/28 03:58:43, Scott Hess wrote:
> I'm not entirely comfortable with the non-const &, here.  I'm not entirely
sure
> where the style guide covers this case, either to allow or disallow.  Does not
> having it be a reference still steal the values as it iterates?  If so, I
think
> that would be preferable (as-is, I think the std::move(response) line actually
> is where the value moves out of the collection).

I think for(..:..) loops by default copy the elements, so if copying the
unique_ptr actually does a move(), then just removing the '&' would do the
trick.  If that doesn't work, then the non-const-ref seems like the next
cleanest.

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

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database_unittest.cc:134: new
ParsedServerResponse);
On 2016/06/28 03:58:43, Scott Hess (OOO Jul 1-Aug 7) wrote:
> Is this your formatting or auto-formatting?  Once the line is broken, I
> generally prefer to format like
>   std::unique_ptr<ParsedServerResponse>
>     parsed_server_response(new ParsedServerResponse);

I'd say if the auto-formatting is non-optimal, we should still use it but then
file a bug to fix it.  Otherwise, we'll run into this everytime someone hits
auto-format.

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

https://codereview.chromium.org/2103693002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:248: for
(ListUpdateResponse list_update_response :
I think this should be a reference, so you can swap them out below.

https://codereview.chromium.org/2103693002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:259:
ListUpdateResponse* add = new ListUpdateResponse(list_update_response);
Rather than copying, you could swap:

ListUpdateResponse* add = new ListUpdateResponse;
add->Swap(&list_update_response);
..push_back(..)

[vakh (use Gerrit instead), 2016-06-28 21:33:38]

shess@ and nparer@ feedback

[vakh (use Gerrit instead), 2016-06-28 21:34:14]

Responded to all the comments (mostly with "Done"). PTAL.

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

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.cc:108: for
(std::unique_ptr<ListUpdateResponse>& response :
On 2016/06/28 03:58:43, Scott Hess (OOO Jul 1-Aug 7) wrote:
> I'm not entirely comfortable with the non-const &, here.  I'm not entirely
sure
> where the style guide covers this case, either to allow or disallow.  Does not
> having it be a reference still steal the values as it iterates?  If so, I
think
> that would be preferable (as-is, I think the std::move(response) line actually
> is where the value moves out of the collection).

1. Adding constness leads to a compile-time error:
../../base/bind_helpers.h:419:13: error: 'mutable' and 'const' cannot be mixed

2. There's lots of precedent:
https://cs.chromium.org/search/?q=%22for+(std::unique_ptr%22&sq=package:chrom...

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.cc:108: for
(std::unique_ptr<ListUpdateResponse>& response :
On 2016/06/28 19:00:37, Nathan Parker wrote:
> On 2016/06/28 03:58:43, Scott Hess wrote:
> > I'm not entirely comfortable with the non-const &, here.  I'm not entirely
> sure
> > where the style guide covers this case, either to allow or disallow.  Does
not
> > having it be a reference still steal the values as it iterates?  If so, I
> think
> > that would be preferable (as-is, I think the std::move(response) line
actually
> > is where the value moves out of the collection).
> 
> I think for(..:..) loops by default copy the elements, so if copying the
> unique_ptr actually does a move(), then just removing the '&' would do the
> trick.  If that doesn't work, then the non-const-ref seems like the next
> cleanest.

Removing the "&" also leads to a compile-time error:
../../components/safe_browsing_db/v4_database.cc:108:50: error: call to deleted
constructor of 'const std::unique_ptr<ListUpdateResponse>' (aka 'const
unique_ptr<safe_browsing::FetchThreatListUpdatesResponse_ListUpdateResponse>')

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

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database_unittest.cc:134: new
ParsedServerResponse);
On 2016/06/28 03:58:43, Scott Hess (OOO Jul 1-Aug 7) wrote:
> Is this your formatting or auto-formatting?  Once the line is broken, I
> generally prefer to format like
>   std::unique_ptr<ParsedServerResponse>
>     parsed_server_response(new ParsedServerResponse);

$ git cl format

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database_unittest.cc:134: new
ParsedServerResponse);
On 2016/06/28 19:00:38, Nathan Parker wrote:
> On 2016/06/28 03:58:43, Scott Hess (OOO Jul 1-Aug 7) wrote:
> > Is this your formatting or auto-formatting?  Once the line is broken, I
> > generally prefer to format like
> >   std::unique_ptr<ParsedServerResponse>
> >     parsed_server_response(new ParsedServerResponse);
> 
> I'd say if the auto-formatting is non-optimal, we should still use it but then
> file a bug to fix it.  Otherwise, we'll run into this everytime someone hits
> auto-format.

I wouldn't call it a bug. It's just something that some of us dislike slightly
more than the alternative. At best, we could consider starting a discussion
about it.

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database_unittest.cc:245: callback_db_updated_);
On 2016/06/28 03:58:43, Scott Hess (OOO Jul 1-Aug 7) wrote:
> Would the following work as:
>   v4_database_->ApplyUpdate(
>       CreateFakeServerResponse(expected_store_state_map_),
>       callback_db_updated_);
> ???  Right now it feels like it's creating a long-winded temporary for no
> particular purpose.
> 
> Otherwise, I see no reason not to use |response|.  There aren't other
responses
> here, so recapitulating the entire type doesn't make it clearer.

Done.

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_protocol_manager_util.h:61:
UpdateListIdentifier(const ListUpdateResponse*);
On 2016/06/28 03:58:43, Scott Hess (OOO Jul 1-Aug 7) wrote:
> Should this be explicit?  The change to pointer makes me wonder if it could
> match in new contexts.

Not sure what you mean by that. What could be the new context(s)?
But, changed back to reference.

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

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_store_unittest.cc:123: ListUpdateResponse*
list_update_response = new ListUpdateResponse;
On 2016/06/28 03:58:43, Scott Hess (OOO Jul 1-Aug 7) wrote:
> This leaves the pointer unowned for a bit.  Instead this should be the
> unique_ptr, with std::move() at the write.

Done.

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_store_unittest.cc:131: ListUpdateResponse*
list_update_response = new ListUpdateResponse;
On 2016/06/28 03:58:43, Scott Hess (OOO Jul 1-Aug 7) wrote:
> Likewise here.

Done.

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

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_update_protocol_manager.cc:259:
ListUpdateResponse* add = new ListUpdateResponse(list_update_response);
On 2016/06/28 03:58:43, Scott Hess (OOO Jul 1-Aug 7) wrote:
> Don't leave a gap where the pointer is raw, unique it as soon as created.

Done.

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_update_protocol_manager.h (right):

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_update_protocol_manager.h:45:
std::unique_ptr<ParsedServerResponse> parsed_server_response)>
On 2016/06/28 03:58:43, Scott Hess (OOO Jul 1-Aug 7) wrote:
> Don't need the parameter's name in this context, just the type.

Done.

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

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_update_protocol_manager_unittest.cc:56:
std::unique_ptr<ListUpdateResponse>& actual =
On 2016/06/28 03:58:43, Scott Hess (OOO Jul 1-Aug 7) wrote:
> const should still be appropriate for a reference.  The change _allows_
> destructively rearranging the overall response, it doesn't require it.

Done.

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

https://codereview.chromium.org/2103693002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:248: for
(ListUpdateResponse list_update_response :
On 2016/06/28 19:00:38, Nathan Parker wrote:
> I think this should be a reference, so you can swap them out below.

Done.

https://codereview.chromium.org/2103693002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:259:
ListUpdateResponse* add = new ListUpdateResponse(list_update_response);
On 2016/06/28 19:00:38, Nathan Parker wrote:
> Rather than copying, you could swap:
> 
> ListUpdateResponse* add = new ListUpdateResponse;
> add->Swap(&list_update_response);
> ..push_back(..)

Done.

[vakh (use Gerrit instead), 2016-06-28 21:59:48]

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_protocol_manager_util.h:61:
UpdateListIdentifier(const ListUpdateResponse*);
On 2016/06/28 21:34:14, vakh wrote:
> On 2016/06/28 03:58:43, Scott Hess (OOO Jul 1-Aug 7) wrote:
> > Should this be explicit?  The change to pointer makes me wonder if it could
> > match in new contexts.
> 
> Not sure what you mean by that. What could be the new context(s)?
> But, changed back to reference.

nparker@ told me that you probable meant the use of "explicit" keyword.
I don't feel there's a reason to use explicit here -- as long as the caller
passes something that can become ListUpdateResponse (whether implicitly or
explicitly), we shouldn't care.
To ensure that the values inside the ListUpdateResponse are valid, I already
have the DCHECKs in place inside the constructor.

[Scott Hess - ex-Googler, 2016-06-28 22:22:25]

LGTM.  Your call as to whether any suggestions below should be followed.  The
explicit one maybe is a style-guide thing, though...

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

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.cc:108: for
(std::unique_ptr<ListUpdateResponse>& response :
On 2016/06/28 21:34:14, vakh wrote:
> On 2016/06/28 03:58:43, Scott Hess (OOO Jul 1-Aug 7) wrote:
> > I'm not entirely comfortable with the non-const &, here.  I'm not entirely
> sure
> > where the style guide covers this case, either to allow or disallow.  Does
not
> > having it be a reference still steal the values as it iterates?  If so, I
> think
> > that would be preferable (as-is, I think the std::move(response) line
actually
> > is where the value moves out of the collection).
> 
> 1. Adding constness leads to a compile-time error:
> ../../base/bind_helpers.h:419:13: error: 'mutable' and 'const' cannot be mixed

Apologies - that's expected, because you need to modify it, I think the const
will break both the ways you mentioned.  I was meaning that if it's a reference,
it should be const, but if it can't be const, it shouldn't be a reference
(that's my interpretation of the style discussions I recall).

> 2. There's lots of precedent:
>
https://cs.chromium.org/search/?q=%22for+(std::unique_ptr%22&sq=package:chrom...

To me that looks like a mixture of unique_ptr and unique_ptr&, which implies
that unique_ptr would work fine on its own (non-const, non-&).

My target, here, is that unique_ptr& says that there's a unique_ptr in the
response, and this is a reference to it, whereas unique_ptr says that _this_ is
the owning pointer.  The difference (I think) is when the referenced data will
be unwound.  As a reference, it will be released either at the std::move(), or
when the function exits (if the various if statements were false, leaving the
element in the container).  As a plain non-reference unique_ptr, it will always
be either forwarded or released on each iteration, so the container will always
be gradually emptying.

I feel like pulling the elements out of the container one-by-one is more
intuitive, but if you think using the ref is clearer, I'll defer to that.

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

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database_unittest.cc:134: new
ParsedServerResponse);
On 2016/06/28 21:34:14, vakh wrote:
> On 2016/06/28 03:58:43, Scott Hess (OOO Jul 1-Aug 7) wrote:
> > Is this your formatting or auto-formatting?  Once the line is broken, I
> > generally prefer to format like
> >   std::unique_ptr<ParsedServerResponse>
> >     parsed_server_response(new ParsedServerResponse);
> 
> $ git cl format

Acknowledged.  All hail our formatting overlords.

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_protocol_manager_util.h:61:
UpdateListIdentifier(const ListUpdateResponse*);
On 2016/06/28 21:59:48, vakh wrote:
> On 2016/06/28 21:34:14, vakh wrote:
> > On 2016/06/28 03:58:43, Scott Hess (OOO Jul 1-Aug 7) wrote:
> > > Should this be explicit?  The change to pointer makes me wonder if it
could
> > > match in new contexts.
> > 
> > Not sure what you mean by that. What could be the new context(s)?
> > But, changed back to reference.
> 
> nparker@ told me that you probable meant the use of "explicit" keyword.
> I don't feel there's a reason to use explicit here -- as long as the caller
> passes something that can become ListUpdateResponse (whether implicitly or
> explicitly), we shouldn't care.
> To ensure that the values inside the ListUpdateResponse are valid, I already
> have the DCHECKs in place inside the constructor.

What I meant by explicit was the keyword, and why I mentioned it is that you
changed from reference to pointer.  As a reference, it can't be matched
inappropriately, I think.  But as a pointer, I think it could also match
nullptr, and perhaps NULL, which means it might be able to match 0, and that
maybe leads to other weird cases (though I _think_ it can't match arbitrary
integers, or integer-convertable things, I think just the special case of 0. 
Unless c++11 changes things, or I'm wrong).  I think with explicit you'd have to
cast NULL to a ListUpdateResponse* explicitly to make that case happen.

In any case, the pointer version needs a NULL check.  The reference version does
not, because it can't be NULL.

It may be that it should be explicit either way:
   https://google.github.io/styleguide/cppguide.html#Implicit_Conversions

[Nathan Parker, 2016-06-28 22:37:33]

lgtm

owner's stamp

[vakh (use Gerrit instead), 2016-06-28 22:51:48]

git pull

[vakh (use Gerrit instead), 2016-06-29 19:48:00]

Minor: Add the explicit keyword for UpdateListIdentifier constructor

[vakh (use Gerrit instead), 2016-06-29 19:58:52]

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

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.cc:108: for
(std::unique_ptr<ListUpdateResponse>& response :
On 2016/06/28 at 22:22:25, Scott Hess (OOO Jul 1-Aug 7) wrote:
> On 2016/06/28 21:34:14, vakh wrote:
> > On 2016/06/28 03:58:43, Scott Hess (OOO Jul 1-Aug 7) wrote:
> > > I'm not entirely comfortable with the non-const &, here.  I'm not entirely
> > sure
> > > where the style guide covers this case, either to allow or disallow.  Does
not
> > > having it be a reference still steal the values as it iterates?  If so, I
> > think
> > > that would be preferable (as-is, I think the std::move(response) line
actually
> > > is where the value moves out of the collection).
> > 
> > 1. Adding constness leads to a compile-time error:
> > ../../base/bind_helpers.h:419:13: error: 'mutable' and 'const' cannot be
mixed
> 
> Apologies - that's expected, because you need to modify it, I think the const
will break both the ways you mentioned.  I was meaning that if it's a reference,
it should be const, but if it can't be const, it shouldn't be a reference
(that's my interpretation of the style discussions I recall).
> 
> > 2. There's lots of precedent:
> >
https://cs.chromium.org/search/?q=%22for+(std::unique_ptr%22&sq=package:chrom...
> 
> To me that looks like a mixture of unique_ptr and unique_ptr&, which implies
that unique_ptr would work fine on its own (non-const, non-&).
> 
> My target, here, is that unique_ptr& says that there's a unique_ptr in the
response, and this is a reference to it, whereas unique_ptr says that _this_ is
the owning pointer.  The difference (I think) is when the referenced data will
be unwound.  As a reference, it will be released either at the std::move(), or
when the function exits (if the various if statements were false, leaving the
element in the container).  As a plain non-reference unique_ptr, it will always
be either forwarded or released on each iteration, so the container will always
be gradually emptying.
> 
> I feel like pulling the elements out of the container one-by-one is more
intuitive, but if you think using the ref is clearer, I'll defer to that.

I am going ahead with the current implementation for the following reasons:
1. Can't use 'const' since I need to modify this proto later.
2. Can't drop '&' since it leads to a compile-time error. See above.
3. The object does get moved and ownership passed even with non-const reference.
I added the following logs:
[ RUN      ] V4DatabaseTest.TestApplyUpdateWithNewStates
[120220:120220:0629/123925:618849993560:VERBOSE1:v4_database.cc(119)] response
(before): 0x1e0c71c2ed30
[120220:120220:0629/123925:618849993621:VERBOSE1:v4_database.cc(125)] response
(after): 0
[120220:120220:0629/123925:618849993656:VERBOSE1:v4_database.cc(119)] response
(before): 0x1e0c71c2e180
[120220:120220:0629/123925:618849993687:VERBOSE1:v4_database.cc(125)] response
(after): 0

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_protocol_manager_util.h:61:
UpdateListIdentifier(const ListUpdateResponse*);
On 2016/06/28 at 22:22:25, Scott Hess (OOO Jul 1-Aug 7) wrote:
> On 2016/06/28 21:59:48, vakh wrote:
> > On 2016/06/28 21:34:14, vakh wrote:
> > > On 2016/06/28 03:58:43, Scott Hess (OOO Jul 1-Aug 7) wrote:
> > > > Should this be explicit?  The change to pointer makes me wonder if it
could
> > > > match in new contexts.
> > > 
> > > Not sure what you mean by that. What could be the new context(s)?
> > > But, changed back to reference.
> > 
> > nparker@ told me that you probable meant the use of "explicit" keyword.
> > I don't feel there's a reason to use explicit here -- as long as the caller
> > passes something that can become ListUpdateResponse (whether implicitly or
> > explicitly), we shouldn't care.
> > To ensure that the values inside the ListUpdateResponse are valid, I already
> > have the DCHECKs in place inside the constructor.
> 
> What I meant by explicit was the keyword, and why I mentioned it is that you
changed from reference to pointer.  As a reference, it can't be matched
inappropriately, I think.  But as a pointer, I think it could also match
nullptr, and perhaps NULL, which means it might be able to match 0, and that
maybe leads to other weird cases (though I _think_ it can't match arbitrary
integers, or integer-convertable things, I think just the special case of 0. 
Unless c++11 changes things, or I'm wrong).  I think with explicit you'd have to
cast NULL to a ListUpdateResponse* explicitly to make that case happen.
> 
> In any case, the pointer version needs a NULL check.  The reference version
does not, because it can't be NULL.
> 
> It may be that it should be explicit either way:
>    https://google.github.io/styleguide/cppguide.html#Implicit_Conversions

Done. Thanks for pointing it out.

[vakh (use Gerrit instead), 2016-06-29 20:00:17]

Description was changed from

==========
PVer4: Send mutable response to the database and the stores

V4Store now gets a unique pointer to a ListUpdateResponse, which it can modify.
It swaps this ListUpdateResponse proto into the V4FileFormat proto before the
V4FileFormat proto is written to disk.

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

to

==========
SafeBrowsing PVer4: Send mutable response to the database and the stores

V4Store now gets a unique pointer to a ListUpdateResponse, which it can modify.
It swaps this ListUpdateResponse proto into the V4FileFormat proto before the
V4FileFormat proto is written to disk.

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

[vakh (use Gerrit instead), 2016-06-29 20:00:24]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2016-06-29 20:00:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, shess@chromium.org
Link to the patchset: https://codereview.chromium.org/2103693002/#ps80001
(title: "Minor: Add the explicit keyword for UpdateListIdentifier constructor")

[commit-bot: I haz the power, 2016-06-29 20:02:49]

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

[Scott Hess - ex-Googler, 2016-06-29 20:15:58]

still lgtm

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

https://codereview.chromium.org/2103693002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.cc:108: for
(std::unique_ptr<ListUpdateResponse>& response :
On 2016/06/29 19:58:52, vakh wrote:
> On 2016/06/28 at 22:22:25, Scott Hess (OOO Jul 1-Aug 7) wrote:
> > On 2016/06/28 21:34:14, vakh wrote:
> > > On 2016/06/28 03:58:43, Scott Hess (OOO Jul 1-Aug 7) wrote:
> > > > I'm not entirely comfortable with the non-const &, here.  I'm not
entirely
> > > sure
> > > > where the style guide covers this case, either to allow or disallow. 
Does
> not
> > > > having it be a reference still steal the values as it iterates?  If so,
I
> > > think
> > > > that would be preferable (as-is, I think the std::move(response) line
> actually
> > > > is where the value moves out of the collection).
> > > 
> > > 1. Adding constness leads to a compile-time error:
> > > ../../base/bind_helpers.h:419:13: error: 'mutable' and 'const' cannot be
> mixed
> > 
> > Apologies - that's expected, because you need to modify it, I think the
const
> will break both the ways you mentioned.  I was meaning that if it's a
reference,
> it should be const, but if it can't be const, it shouldn't be a reference
> (that's my interpretation of the style discussions I recall).
> > 
> > > 2. There's lots of precedent:
> > >
>
https://cs.chromium.org/search/?q=%22for+(std::unique_ptr%22&sq=package:chrom...
> > 
> > To me that looks like a mixture of unique_ptr and unique_ptr&, which implies
> that unique_ptr would work fine on its own (non-const, non-&).
> > 
> > My target, here, is that unique_ptr& says that there's a unique_ptr in the
> response, and this is a reference to it, whereas unique_ptr says that _this_
is
> the owning pointer.  The difference (I think) is when the referenced data will
> be unwound.  As a reference, it will be released either at the std::move(), or
> when the function exits (if the various if statements were false, leaving the
> element in the container).  As a plain non-reference unique_ptr, it will
always
> be either forwarded or released on each iteration, so the container will
always
> be gradually emptying.
> > 
> > I feel like pulling the elements out of the container one-by-one is more
> intuitive, but if you think using the ref is clearer, I'll defer to that.
> 
> I am going ahead with the current implementation for the following reasons:
> 1. Can't use 'const' since I need to modify this proto later.
> 2. Can't drop '&' since it leads to a compile-time error. See above.
> 3. The object does get moved and ownership passed even with non-const
reference.
> I added the following logs:
> [ RUN      ] V4DatabaseTest.TestApplyUpdateWithNewStates
> [120220:120220:0629/123925:618849993560:VERBOSE1:v4_database.cc(119)] response
> (before): 0x1e0c71c2ed30
> [120220:120220:0629/123925:618849993621:VERBOSE1:v4_database.cc(125)] response
> (after): 0
> [120220:120220:0629/123925:618849993656:VERBOSE1:v4_database.cc(119)] response
> (before): 0x1e0c71c2e180
> [120220:120220:0629/123925:618849993687:VERBOSE1:v4_database.cc(125)] response
> (after): 0

Acknowledged.  I wonder where we'd be if move semantics were _not_ intuitive?

[commit-bot: I haz the power, 2016-06-29 21:37:34]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-29 21:40:28]

Description was changed from

==========
SafeBrowsing PVer4: Send mutable response to the database and the stores

V4Store now gets a unique pointer to a ListUpdateResponse, which it can modify.
It swaps this ListUpdateResponse proto into the V4FileFormat proto before the
V4FileFormat proto is written to disk.

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

to

==========
SafeBrowsing PVer4: Send mutable response to the database and the stores

V4Store now gets a unique pointer to a ListUpdateResponse, which it can modify.
It swaps this ListUpdateResponse proto into the V4FileFormat proto before the
V4FileFormat proto is written to disk.

BUG=543161

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

[commit-bot: I haz the power, 2016-06-29 21:40:29]

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