v4_update_protocol_manager: Basic implementation with TODOs

v4_update_protocol_manager: Basic implementation with TODOs

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

[vakh (use Gerrit instead), 2016-02-24 11:03:19]

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

[vakh (use Gerrit instead), 2016-02-24 11:03:20]

I'm having a number of patch issues with the original CL so please review this
one instead.
Original CL: https://codereview.chromium.org/1719883003

[vakh (use Gerrit instead), 2016-02-24 17:46:46]

Fix some GN dependencies

[vakh (use Gerrit instead), 2016-02-24 19:21:36]

Delete some objects allocated for tests

[kcarattini, 2016-03-17 03:05:03]

kcarattini@chromium.org changed reviewers:
+ kcarattini@chromium.org

[kcarattini, 2016-03-17 03:05:04]

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

https://codereview.chromium.org/1727033003/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:199: // proceed with
the request. If not, we are required to return empty results
Does that last sentence make sense for updates? I think it was specific to
gethash requests.

[vakh (use Gerrit instead), 2016-03-21 20:00:28]

Rebase

[vakh (use Gerrit instead), 2016-03-21 20:35:04]

Fix comment.

[vakh (use Gerrit instead), 2016-03-21 21:35:33]

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

https://codereview.chromium.org/1727033003/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:199: // proceed with
the request. If not, we are required to return empty results
On 2016/03/17 03:05:04, kcarattini wrote:
> Does that last sentence make sense for updates? I think it was specific to
> gethash requests.

Removed. Thanks for pointing that out.

[vakh (use Gerrit instead), 2016-03-22 16:43:43]

ping.

[vakh (use Gerrit instead), 2016-03-22 16:46:00]

vakh@chromium.org changed reviewers:
+ blundell@chromium.org, holte@chromium.org

[vakh (use Gerrit instead), 2016-03-22 16:46:00]

blundell@chromium.org: Please review changes in 
components/safe_browsing_db.gypi

holte@chromium.org: Please review changes in
tools/metrics/histograms/histograms.xml

[blundell, 2016-03-22 16:49:51]

Nathan's LG should be enough here.

[Nathan Parker, 2016-03-22 23:57:55]

Overall looks great to me.  Mostly little comments.

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_protocol_manager_util.h:48: enum
OperationResultType {
nit: How about just "OperationResult"?

Another idea: You could call it "class enum V4ProtocolResult" and put it outside
this class, so it's not a nested type.  Then the values would be ref'd by
V4ProtocolResult::PARSE_ERROR, etc.  Your choice...

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

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:58: const char
kUmaV4UpdateResponseMetricName[] =
It's pretty standard to just put the metric name's string literal directly in
the call, if it's used in only one place.

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:87: factory_ = new
V4UpdateProtocolManagerFactoryImpl();
Do you need this Impl class at all?  Another way would be to return a new V4UPM
right here if !factory_

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:160: next_update_time_
=
If it doesn't have a min wait duration, does next_update_time_ remain as ~now?

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:216: net::URLFetcher*
fetcher =
nit: You could put this directly into fetcher_

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
File components/safe_browsing_db/v4_update_protocol_manager.h (right):

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:74: //   - The set of
list to fetch the updates for.
nit: s/list/lists/

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:76: // It is valud to
have one or more lists in lists_to_update set that have no
nit: valid

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:80: const
base::hash_set<const UpdateListIdentifier*>& lists_to_update,
Could you do without lists_to_update, and just assume all that are present in
the _states should be updated? 

Also: Could store the UpdateListIdentifiers directly rather than ptrs, so
there's no concern of ownership.  They're pretty small.

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:85: // Record an update
operation result.
This could be in an anonymous namespace in the .cc, ya?

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:103: GURL
GetUpdateUrl(const std::string& request_base64) const;
add a comment

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:139: // error, or the
minimum wait time if the last response was successful.
This comment is hard to grok. How about describing how it's used rather than how
it's set?

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:152: bool
update_request_pending_;
Is this identical to request_ != null?

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
File components/safe_browsing_db/v4_update_protocol_manager_unittest.cc (right):

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager_unittest.cc:150:
fetcher->set_response_code(200);
Does it need a response code and string if it has a network error?

[Steven Holte, 2016-03-23 18:33:11]

histograms lgtm

[vakh (use Gerrit instead), 2016-03-24 22:08:46]

Incorporate review feedback.

[vakh (use Gerrit instead), 2016-03-24 22:34:25]

PTAL.

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_protocol_manager_util.h:48: enum
OperationResultType {
On 2016/03/22 23:57:54, Nathan Parker wrote:
> nit: How about just "OperationResult"?
> 
> Another idea: You could call it "class enum V4ProtocolResult" and put it
outside
> this class, so it's not a nested type.  Then the values would be ref'd by
> V4ProtocolResult::PARSE_ERROR, etc.  Your choice...

Done. Changed to enum V4OperationResult outside the class.
UMA_HISTOGRAM_ENUMERATION doesn't allow "enum class"

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

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:58: const char
kUmaV4UpdateResponseMetricName[] =
On 2016/03/22 23:57:55, Nathan Parker wrote:
> It's pretty standard to just put the metric name's string literal directly in
> the call, if it's used in only one place.

Done.

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:87: factory_ = new
V4UpdateProtocolManagerFactoryImpl();
On 2016/03/22 23:57:55, Nathan Parker wrote:
> Do you need this Impl class at all?  Another way would be to return a new
V4UPM
> right here if !factory_

This would be useful for testing using DI. The old implementation uses it that
way.
Leaving it as-is for now but I'd not mind removing the factory if you feel
strongly.

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:160: next_update_time_
=
On 2016/03/22 23:57:55, Nathan Parker wrote:
> If it doesn't have a min wait duration, does next_update_time_ remain as ~now?

Yes, we don't update it if the response doesn't contain min_wait_duration.

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:216: net::URLFetcher*
fetcher =
On 2016/03/22 23:57:55, Nathan Parker wrote:
> nit: You could put this directly into fetcher_

Done.

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
File components/safe_browsing_db/v4_update_protocol_manager.h (right):

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:74: //   - The set of
list to fetch the updates for.
On 2016/03/22 23:57:55, Nathan Parker wrote:
> nit: s/list/lists/

Done.

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:76: // It is valud to
have one or more lists in lists_to_update set that have no
On 2016/03/22 23:57:55, Nathan Parker wrote:
> nit: valid

Done.

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:80: const
base::hash_set<const UpdateListIdentifier*>& lists_to_update,
On 2016/03/22 23:57:55, Nathan Parker wrote:
> Could you do without lists_to_update, and just assume all that are present in
> the _states should be updated?
There won't be anything in the _states on first launch.
The set is for the updates we want to fetch and the _states map is for updates
we've already fetched.
 
> 
> Also: Could store the UpdateListIdentifiers directly rather than ptrs, so
> there's no concern of ownership.  They're pretty small.

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:85: // Record an update
operation result.
On 2016/03/22 23:57:55, Nathan Parker wrote:
> This could be in an anonymous namespace in the .cc, ya?

Done.

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:103: GURL
GetUpdateUrl(const std::string& request_base64) const;
On 2016/03/22 23:57:55, Nathan Parker wrote:
> add a comment

Done.

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:139: // error, or the
minimum wait time if the last response was successful.
On 2016/03/22 23:57:55, Nathan Parker wrote:
> This comment is hard to grok. How about describing how it's used rather than
how
> it's set?

Done.

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:152: bool
update_request_pending_;
On 2016/03/22 23:57:55, Nathan Parker wrote:
> Is this identical to request_ != null?

Done. I had added this with some timeout handling in mind but since I am not
handling timeouts anyway, I have removed that variable and added a TODO.

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
File components/safe_browsing_db/v4_update_protocol_manager_unittest.cc (right):

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager_unittest.cc:150:
fetcher->set_response_code(200);
On 2016/03/22 23:57:55, Nathan Parker wrote:
> Does it need a response code and string if it has a network error?

Done.

[vakh (use Gerrit instead), 2016-03-24 22:34:31]

Fix comment && git fetch && git rebase

[Nathan Parker, 2016-03-24 22:48:05]

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

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:160: next_update_time_
=
On 2016/03/24 22:34:24, vakh wrote:
> On 2016/03/22 23:57:55, Nathan Parker wrote:
> > If it doesn't have a min wait duration, does next_update_time_ remain as
~now?
> 
> Yes, we don't update it if the response doesn't contain min_wait_duration.

ok, so then assumption is that the caller to GetUpdate() would only invoke it at
a reasonable rate.

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
File components/safe_browsing_db/v4_update_protocol_manager.h (right):

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:80: const
base::hash_set<const UpdateListIdentifier*>& lists_to_update,

This one too:
> Also: Could store the UpdateListIdentifiers directly rather than ptrs, so
> there's no concern of ownership.  They're pretty small.

[vakh (use Gerrit instead), 2016-03-25 02:20:18]

Use scoped_ptrs instead of naked pointers for better memory management.

[vakh (use Gerrit instead), 2016-03-25 02:42:37]

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

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:160: next_update_time_
=
On 2016/03/24 22:48:05, Nathan Parker wrote:
> On 2016/03/24 22:34:24, vakh wrote:
> > On 2016/03/22 23:57:55, Nathan Parker wrote:
> > > If it doesn't have a min wait duration, does next_update_time_ remain as
> ~now?
> > 
> > Yes, we don't update it if the response doesn't contain min_wait_duration.
> 
> ok, so then assumption is that the caller to GetUpdate() would only invoke it
at
> a reasonable rate.

Not really. The class stores next_update_time_ which tells it until when not to
send another update request.

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
File components/safe_browsing_db/v4_update_protocol_manager.h (right):

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:80: const
base::hash_set<const UpdateListIdentifier*>& lists_to_update,
On 2016/03/24 22:48:05, Nathan Parker wrote:
> 
> This one too:
> > Also: Could store the UpdateListIdentifiers directly rather than ptrs, so
> > there's no concern of ownership.  They're pretty small.
> 

Can't store instances of UpdateListIdentifiers directly into an unordered_set
since it can't be compared to another instance directly. That would require
adding a comparison method.
Instead, to manage the memory ownership better, I have changed it to use
scoped_ptrs.

[Nathan Parker, 2016-03-25 15:53:18]

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
File components/safe_browsing_db/v4_update_protocol_manager.h (right):

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:80: const
base::hash_set<const UpdateListIdentifier*>& lists_to_update,
On 2016/03/25 02:42:37, vakh wrote:
> On 2016/03/24 22:48:05, Nathan Parker wrote:
> > 
> > This one too:
> > > Also: Could store the UpdateListIdentifiers directly rather than ptrs, so
> > > there's no concern of ownership.  They're pretty small.
> > 
> 
> Can't store instances of UpdateListIdentifiers directly into an unordered_set
> since it can't be compared to another instance directly. That would require
> adding a comparison method.
> Instead, to manage the memory ownership better, I have changed it to use
> scoped_ptrs.

The "set" guarantees there's only one of each.  With scoped_ptrs, I suspect it's
comparing the inner pointers.  So it won't guarantee you have a unique set of
_values_ unless you have a comparison operator. I ran into a similar case with
ThreatMetadata structs and added a few methods to make it more easily comparable
(in my case just for tests)  Code:

https://code.google.com/p/chromium/codesearch#chromium/src/components/safe_br...

[vakh (use Gerrit instead), 2016-03-25 22:25:58]

Use struct objects in unordered_map and unordered_set

[vakh (use Gerrit instead), 2016-03-25 22:51:47]

Thanks for the review. PTAL.

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
File components/safe_browsing_db/v4_update_protocol_manager.h (right):

https://codereview.chromium.org/1727033003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:80: const
base::hash_set<const UpdateListIdentifier*>& lists_to_update,
On 2016/03/25 15:53:18, Nathan Parker wrote:
> On 2016/03/25 02:42:37, vakh wrote:
> > On 2016/03/24 22:48:05, Nathan Parker wrote:
> > > 
> > > This one too:
> > > > Also: Could store the UpdateListIdentifiers directly rather than ptrs,
so
> > > > there's no concern of ownership.  They're pretty small.
> > > 
> > 
> > Can't store instances of UpdateListIdentifiers directly into an
unordered_set
> > since it can't be compared to another instance directly. That would require
> > adding a comparison method.
> > Instead, to manage the memory ownership better, I have changed it to use
> > scoped_ptrs.
> 
> The "set" guarantees there's only one of each.  With scoped_ptrs, I suspect
it's
> comparing the inner pointers.  So it won't guarantee you have a unique set of
> _values_ unless you have a comparison operator. I ran into a similar case with
> ThreatMetadata structs and added a few methods to make it more easily
comparable
> (in my case just for tests)  Code:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/safe_br...

Done.

[Nathan Parker, 2016-03-25 23:51:01]

lgtm

Looks great!  A few small fixes and otherwise LGTM.

https://codereview.chromium.org/1727033003/diff/150001/components/safe_browsi...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/1727033003/diff/150001/components/safe_browsi...
components/safe_browsing_db/v4_protocol_manager_util.h:44: return platform_type
== other.platform_type &&
The body of these should probably go in a .cc file so it doesn't get recompiled
for every file that includes this .h

https://codereview.chromium.org/1727033003/diff/150001/components/safe_browsi...
components/safe_browsing_db/v4_protocol_manager_util.h:144: std::size_t
operator()(const safe_browsing::UpdateListIdentifier& s) const {
Cool! I'm glad you figured out how to do this.  I'm going to use your code as an
example when I try to do the same.

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

https://codereview.chromium.org/1727033003/diff/150001/components/safe_browsi...
components/safe_browsing_db/v4_update_protocol_manager.cc:114: const
base::hash_map<UpdateListIdentifier, const std::string&>&
Using a reference for the string does require someone to own the ptr (since it's
effectively a ptr).  Adding it by value is gonna be safer (and then no need for
the const).

https://codereview.chromium.org/1727033003/diff/150001/components/safe_browsi...
File components/safe_browsing_db/v4_update_protocol_manager.h (right):

https://codereview.chromium.org/1727033003/diff/150001/components/safe_browsi...
components/safe_browsing_db/v4_update_protocol_manager.h:90:
FRIEND_TEST_ALL_PREFIXES(SafeBrowsingV4UpdateProtocolManagerTest,
readability nit: Do these tests need to start with "SafeBrowsing*"?

https://codereview.chromium.org/1727033003/diff/150001/components/safe_browsi...
File components/safe_browsing_db/v4_update_protocol_manager_unittest.cc (right):

https://codereview.chromium.org/1727033003/diff/150001/components/safe_browsi...
components/safe_browsing_db/v4_update_protocol_manager_unittest.cc:49:
UpdateListIdentifier* list_identifier = new UpdateListIdentifier();
This should be on the stack.

[vakh (use Gerrit instead), 2016-03-26 00:03:35]

git fetch && git rebase

[vakh (use Gerrit instead), 2016-03-26 00:05:38]

Incorporate review feedback.

[vakh (use Gerrit instead), 2016-03-26 00:06:57]

https://codereview.chromium.org/1727033003/diff/150001/components/safe_browsi...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/1727033003/diff/150001/components/safe_browsi...
components/safe_browsing_db/v4_protocol_manager_util.h:44: return platform_type
== other.platform_type &&
On 2016/03/25 23:51:01, Nathan Parker wrote:
> The body of these should probably go in a .cc file so it doesn't get
recompiled
> for every file that includes this .h

Done.

https://codereview.chromium.org/1727033003/diff/150001/components/safe_browsi...
components/safe_browsing_db/v4_protocol_manager_util.h:144: std::size_t
operator()(const safe_browsing::UpdateListIdentifier& s) const {
On 2016/03/25 23:51:01, Nathan Parker wrote:
> Cool! I'm glad you figured out how to do this.  I'm going to use your code as
an
> example when I try to do the same.

Acknowledged.

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

https://codereview.chromium.org/1727033003/diff/150001/components/safe_browsi...
components/safe_browsing_db/v4_update_protocol_manager.cc:114: const
base::hash_map<UpdateListIdentifier, const std::string&>&
On 2016/03/25 23:51:01, Nathan Parker wrote:
> Using a reference for the string does require someone to own the ptr (since
it's
> effectively a ptr).  Adding it by value is gonna be safer (and then no need
for
> the const).

Done.

https://codereview.chromium.org/1727033003/diff/150001/components/safe_browsi...
File components/safe_browsing_db/v4_update_protocol_manager.h (right):

https://codereview.chromium.org/1727033003/diff/150001/components/safe_browsi...
components/safe_browsing_db/v4_update_protocol_manager.h:90:
FRIEND_TEST_ALL_PREFIXES(SafeBrowsingV4UpdateProtocolManagerTest,
On 2016/03/25 23:51:01, Nathan Parker wrote:
> readability nit: Do these tests need to start with "SafeBrowsing*"?

Done.

https://codereview.chromium.org/1727033003/diff/150001/components/safe_browsi...
File components/safe_browsing_db/v4_update_protocol_manager_unittest.cc (right):

https://codereview.chromium.org/1727033003/diff/150001/components/safe_browsi...
components/safe_browsing_db/v4_update_protocol_manager_unittest.cc:49:
UpdateListIdentifier* list_identifier = new UpdateListIdentifier();
On 2016/03/25 23:51:01, Nathan Parker wrote:
> This should be on the stack.

Done.

[vakh (use Gerrit instead), 2016-03-26 00:08:48]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2016-03-26 00:08:48]

The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org,
nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/1727033003/#ps190001
(title: "Incorporate review feedback.")

[commit-bot: I haz the power, 2016-03-26 00:08:59]

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

[commit-bot: I haz the power, 2016-03-26 01:16:04]

Committed patchset #11 (id:190001)

[commit-bot: I haz the power, 2016-03-26 01:17:03]

Description was changed from

==========
v4_update_protocol_manager: Basic implementation with TODOs

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

to

==========
v4_update_protocol_manager: Basic implementation with TODOs

BUG=543161

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

[commit-bot: I haz the power, 2016-03-26 01:17:04]

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