Implement HostListManager by Refactoring HostListLoader

Implement HostListManager by Refactoring HostListLoader

This CL renamed and refactored HostListLoader into HostListManager and
added functionality of updating and deleting a host entry in directory
service.

BUG=607216
Committed: https://crrev.com/8ee766c67317b0d1c97686c175fa313821cccf61
Cr-Commit-Position: refs/heads/master@{#392083}

[Yuwei, 2016-05-03 21:37:01]

yuweih@chromium.org changed reviewers:
+ lambroslambrou@chromium.org

[Yuwei, 2016-05-03 21:37:02]

There will be upcoming CL's for generalizing the OAuthTokenFetcher callback
(maybe do it for the telemetry logger issue) and implementing the front end.
PTAL :)

[Yuwei, 2016-05-03 21:40:56]

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

[commit-bot: I haz the power, 2016-05-03 21:41:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1948793002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1948793002/1

[Lambros, 2016-05-03 22:11:03]

https://codereview.chromium.org/1948793002/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/HostListManager.java
(right):

https://codereview.chromium.org/1948793002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/HostListManager.java:100:
postError(callback, response.error);
If response.error is null, it feels wrong to pass that to the callback. Maybe
pass Error.UNKNOWN instead if that happens?
Or disallow the body being null, make it an empty string, then let the JSON
parsing code handle the failure naturally.
In your Response class, it's probably a good idea to document which members are
allowed to be null. Maybe say either |error| or |body| can be null but not both?

[Yuwei, 2016-05-03 22:39:02]

https://codereview.chromium.org/1948793002/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/HostListManager.java
(right):

https://codereview.chromium.org/1948793002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/HostListManager.java:100:
postError(callback, response.error);
On 2016/05/03 22:11:03, Lambros wrote:
> If response.error is null, it feels wrong to pass that to the callback. Maybe
> pass Error.UNKNOWN instead if that happens?
> Or disallow the body being null, make it an empty string, then let the JSON
> parsing code handle the failure naturally.
> In your Response class, it's probably a good idea to document which members
are
> allowed to be null. Maybe say either |error| or |body| can be null but not
both?

Good catch:) I will let the JSON parser fail in that case and define either
|error| or |body| can be null but not both.

[Yuwei, 2016-05-03 22:47:57]

https://codereview.chromium.org/1948793002/diff/1/remoting/android/java/src/o...
File remoting/android/java/src/org/chromium/chromoting/HostListManager.java
(right):

https://codereview.chromium.org/1948793002/diff/1/remoting/android/java/src/o...
remoting/android/java/src/org/chromium/chromoting/HostListManager.java:100:
postError(callback, response.error);
On 2016/05/03 22:39:02, yuweih wrote:
> On 2016/05/03 22:11:03, Lambros wrote:
> > If response.error is null, it feels wrong to pass that to the callback.
Maybe
> > pass Error.UNKNOWN instead if that happens?
> > Or disallow the body being null, make it an empty string, then let the JSON
> > parsing code handle the failure naturally.
> > In your Response class, it's probably a good idea to document which members
> are
> > allowed to be null. Maybe say either |error| or |body| can be null but not
> both?
> 
> Good catch:) I will let the JSON parser fail in that case and define either
> |error| or |body| can be null but not both.

Wait... Is it possible that a request succeeds with no response body?

[Yuwei, 2016-05-04 22:19:47]

Patch 3 is uploaded. PTAL

[Lambros, 2016-05-05 00:00:14]

lgtm

https://codereview.chromium.org/1948793002/diff/40001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/HostListManager.java
(right):

https://codereview.chromium.org/1948793002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/HostListManager.java:47: *
|error| and |body| can be null and they can't be both non-null.
The way this is phrased, it sounds like |error| and |body| might sometimes both
be null. But sendRequest() never returns an object where error == body == null.
Maybe clearer to phrase this as "exactly one of |error| and |body| is non-null"
so clients don't have to worry about handling the both-null case.

https://codereview.chromium.org/1948793002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/HostListManager.java:298:
String response = String.valueOf(responseBuilder);
Just my own preference, but I would write this as
responseBuilder.toString() (you know that responseBuilder is non-null, so you
don't need the extra null-test that String.valueOf(Object) does). And you could
inline this into the return statement below, if you like.

[Yuwei, 2016-05-05 00:40:59]

https://codereview.chromium.org/1948793002/diff/40001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/HostListManager.java
(right):

https://codereview.chromium.org/1948793002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/HostListManager.java:47: *
|error| and |body| can be null and they can't be both non-null.
On 2016/05/05 00:00:14, Lambros wrote:
> The way this is phrased, it sounds like |error| and |body| might sometimes
both
> be null. But sendRequest() never returns an object where error == body ==
null.
> Maybe clearer to phrase this as "exactly one of |error| and |body| is
non-null"
> so clients don't have to worry about handling the both-null case.

Basically error==/!=null tells you whether the request succeeds or not, and
|body| gives you the actual response if it succeeds. I'm just thinking of the
possibility of a successful request with no response...

Given the way sendRequest() works I think it is safe to disallow the both-null
state and maybe later we can set |body| to an empty string if the no-response
case really happens. (Not sure how HTTP/HTTPRequest works... Maybe you will
always get an empty string if the response doesn't have a body?)

https://codereview.chromium.org/1948793002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/HostListManager.java:298:
String response = String.valueOf(responseBuilder);
On 2016/05/05 00:00:14, Lambros wrote:
> Just my own preference, but I would write this as
> responseBuilder.toString() (you know that responseBuilder is non-null, so you
> don't need the extra null-test that String.valueOf(Object) does). And you
could
> inline this into the return statement below, if you like.

That's right... BTW the code comes from HostListLoader :P

[Yuwei, 2016-05-06 00:31:51]

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

[Yuwei, 2016-05-06 00:31:58]

https://codereview.chromium.org/1948793002/diff/40001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/HostListManager.java
(right):

https://codereview.chromium.org/1948793002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/HostListManager.java:47: *
|error| and |body| can be null and they can't be both non-null.
On 2016/05/05 00:00:14, Lambros wrote:
> The way this is phrased, it sounds like |error| and |body| might sometimes
both
> be null. But sendRequest() never returns an object where error == body ==
null.
> Maybe clearer to phrase this as "exactly one of |error| and |body| is
non-null"
> so clients don't have to worry about handling the both-null case.

I guess it looks clearer now...

https://codereview.chromium.org/1948793002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/HostListManager.java:298:
String response = String.valueOf(responseBuilder);
On 2016/05/05 00:00:14, Lambros wrote:
> Just my own preference, but I would write this as
> responseBuilder.toString() (you know that responseBuilder is non-null, so you
> don't need the extra null-test that String.valueOf(Object) does). And you
could
> inline this into the return statement below, if you like.

Done.

[commit-bot: I haz the power, 2016-05-06 00:32:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1948793002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1948793002/60001

[commit-bot: I haz the power, 2016-05-06 01:25:59]

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

[commit-bot: I haz the power, 2016-05-06 01:26:00]

Dry run: This issue passed the CQ dry run.

[Yuwei, 2016-05-06 17:05:51]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2016-05-06 17:05:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
lambroslambrou@chromium.org
Link to the patchset: https://codereview.chromium.org/1948793002/#ps60001
(title: "Reviewer's feedback")

[commit-bot: I haz the power, 2016-05-06 17:06:16]

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

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

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-06 17:16:12]

Description was changed from

==========
Implement HostListManager by Refactoring HostListLoader

This CL renamed and refactored HostListLoader into HostListManager and
added functionality of updating and deleting a host entry in directory
service.

BUG=607216
==========

to

==========
Implement HostListManager by Refactoring HostListLoader

This CL renamed and refactored HostListLoader into HostListManager and
added functionality of updating and deleting a host entry in directory
service.

BUG=607216

Committed: https://crrev.com/8ee766c67317b0d1c97686c175fa313821cccf61
Cr-Commit-Position: refs/heads/master@{#392083}
==========

[commit-bot: I haz the power, 2016-05-06 17:16:13]

Patchset 4 (id:??) landed as
https://crrev.com/8ee766c67317b0d1c97686c175fa313821cccf61
Cr-Commit-Position: refs/heads/master@{#392083}