Pull HostListDirectoryGrabber out to a separate class.

Pull HostListDirectoryGrabber out to a separate class.

This makes Chromoting.java completely single-threaded. Auth tokens are
still handled in Chromoting.java, but all the host-list fetching takes
place in HostListLoader.java.

The error-handling is improved to match more closely with what the
web-app does in host_list.js. Otherwise, I've tried to keep the
error-handling the same as before. Previously, errors from the
AccountService and host-list fetching were all handled in the same
place. For example, all IOExceptions were assumed to be caused by the
OAuth token being invalid. Now I've added an enumeration of possible
errors from host-list fetching, and we only refresh the auth
token if the error was explicitly an authentication failure.

Future CLs will improve the error-handling, reusing many of the same
localized strings as the web-app.

BUG=304719
NOTRY=true
TBR=sergeyu

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251838

[Lambros, 2014-02-06 21:45:25]

sergeyu: ptal
jamiewalch: FYI

[Sergey Ulanov, 2014-02-11 05:26:28]

https://codereview.chromium.org/157013002/diff/1/remoting/android/java/src/or...
File
remoting/android/java/src/org/chromium/chromoting/HostListDirectoryGrabber.java
(right):

https://codereview.chromium.org/157013002/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/HostListDirectoryGrabber.java:28:
public class HostListDirectoryGrabber {
Call this HostListLoader?

https://codereview.chromium.org/157013002/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/HostListDirectoryGrabber.java:39:
void onHostListReceived(JSONArray hosts);
It doesn't look right that we are returning the list as untyped JSONArray.
Create a class that represents a host and return array of them here? E.g.
currently HostListAdapter has to handle errors in the list returned from the
directory (by catching JSONException) and that logic belongs in this class.

https://codereview.chromium.org/157013002/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/HostListDirectoryGrabber.java:166:
if (aOnline && !bOnline) {
You can merge the two if statements together:
 if (aOnline != bOnline) {
   return aOnline ? -1 : 1;
 }

https://codereview.chromium.org/157013002/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/HostListDirectoryGrabber.java:176:
return 0;
This means that any invalid host entry equals any other host - that's not right.
You wouldn't have this problem if you had a class to represent hosts

[Lambros, 2014-02-12 22:22:07]

https://codereview.chromium.org/157013002/diff/1/remoting/android/java/src/or...
File
remoting/android/java/src/org/chromium/chromoting/HostListDirectoryGrabber.java
(right):

https://codereview.chromium.org/157013002/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/HostListDirectoryGrabber.java:28:
public class HostListDirectoryGrabber {
On 2014/02/11 05:26:28, Sergey Ulanov wrote:
> Call this HostListLoader?

Done.

https://codereview.chromium.org/157013002/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/HostListDirectoryGrabber.java:39:
void onHostListReceived(JSONArray hosts);
On 2014/02/11 05:26:28, Sergey Ulanov wrote:
> It doesn't look right that we are returning the list as untyped JSONArray.
> Create a class that represents a host and return array of them here? E.g.
> currently HostListAdapter has to handle errors in the list returned from the
> directory (by catching JSONException) and that logic belongs in this class.
Great idea! Done.

https://codereview.chromium.org/157013002/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/HostListDirectoryGrabber.java:166:
if (aOnline && !bOnline) {
On 2014/02/11 05:26:28, Sergey Ulanov wrote:
> You can merge the two if statements together:
>  if (aOnline != bOnline) {
>    return aOnline ? -1 : 1;
>  }

Done.

https://codereview.chromium.org/157013002/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/HostListDirectoryGrabber.java:176:
return 0;
On 2014/02/11 05:26:28, Sergey Ulanov wrote:
> This means that any invalid host entry equals any other host - that's not
right.
> You wouldn't have this problem if you had a class to represent hosts

Done.

[Sergey Ulanov, 2014-02-13 19:36:07]

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/Host.java (right):

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/Host.java:8: public class Host
{
Does the style guide allow struct-like classes like this? AFAIK it doesn't so
you need to make th make the data fields private and add public methods to
access them.

[Lambros, 2014-02-13 20:17:41]

On 2014/02/13 19:36:07, Sergey Ulanov wrote:
>
https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
> File remoting/android/java/src/org/chromium/chromoting/Host.java (right):
> 
>
https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
> remoting/android/java/src/org/chromium/chromoting/Host.java:8: public class
Host
> {
> Does the style guide allow struct-like classes like this? AFAIK it doesn't so
> you need to make th make the data fields private and add public methods to
> access them.

I checked the Android style guide, which Chrome uses
(https://source.android.com/source/code-style.html)
and the Google style guide
(http://google-styleguide.googlecode.com/svn/trunk/javaguide.html)
and didn't see anything banning this.

The Android guide documents how public data members should be named (likeThis),
so it doesn't ban them completely.

Using git grep 'public [a-zA-Z]* [a-zA-Z]*;'
I found several examples in Chrome of struct-like inner classes.
And just one example where it's a top-level class:
content/public/android/java/src/org/chromium/content/browser/FileDescriptorInfo.java

So I think it's OK, although a case could be made for making Host an inner class
of HostListLoader.

[Sergey Ulanov, 2014-02-14 08:55:20]

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right):

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:185: } catch
(OperationCanceledException ex) {
I think you can wrap only the first line (future.getResult()) in the try-catch
block. Everything else doesn't need be inside of it.

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/Host.java (right):

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/Host.java:8: public class Host
{
Call it HostInfo?

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/HostListLoader.java
(right):

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/HostListLoader.java:112: //
Interpret what the directory server told us.
suggest rewording: "Parse directory response"

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/HostListLoader.java:124:
host.jabberId = hostJson.getString("jabberId");
jabberId may not be set when the host was just registered. We shouldn't be
treating such cases as an error. Use optString() here.

I think ideally only hostName and hostId should be mandatory and this code
should handle gracefully the case when the other fields are missing.

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/HostListLoader.java:131:
sortHosts(hostList);
Sorting should be UI feature. Move this to HostListAdapter?

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/HostListLoader.java:131:
sortHosts(hostList);
This and code below it doesn't need to be in try-catch

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/HostListLoader.java:143:
Log.e("hostlist", "Unexpected error while fetching host list: " + ex);
Throw RuntimeException?

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/HostListLoader.java:144: }
catch (JSONException ex) {
I suggest having two separate try-catch block. One for the HTTP request, and one
to parse the response. That way you wouldn't need to mix exception handling.

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/HostListLoader.java:168:
Comparator<Host> compareHosts = new Comparator<Host>() {
nit: hostComparator

[Lambros, 2014-02-15 01:40:20]

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right):

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:185: } catch
(OperationCanceledException ex) {
On 2014/02/14 08:55:20, Sergey Ulanov wrote:
> I think you can wrap only the first line (future.getResult()) in the try-catch
> block. Everything else doesn't need be inside of it.

Done.

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/Host.java (right):

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/Host.java:8: public class Host
{
On 2014/02/14 08:55:20, Sergey Ulanov wrote:
> Call it HostInfo?

Done.

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/HostListLoader.java
(right):

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/HostListLoader.java:112: //
Interpret what the directory server told us.
On 2014/02/14 08:55:20, Sergey Ulanov wrote:
> suggest rewording: "Parse directory response"

Done.

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/HostListLoader.java:124:
host.jabberId = hostJson.getString("jabberId");
On 2014/02/14 08:55:20, Sergey Ulanov wrote:
> jabberId may not be set when the host was just registered. We shouldn't be
> treating such cases as an error. Use optString() here.
> 
> I think ideally only hostName and hostId should be mandatory and this code
> should handle gracefully the case when the other fields are missing.

Done.

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/HostListLoader.java:131:
sortHosts(hostList);
On 2014/02/14 08:55:20, Sergey Ulanov wrote:
> Sorting should be UI feature. Move this to HostListAdapter?

This was already decided in the CL here:
https://codereview.chromium.org/148313004
Jamie said there was only ever going to be one reasonable way to sort the list,
so we might as well do it just once at loading-time.

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/HostListLoader.java:131:
sortHosts(hostList);
On 2014/02/14 08:55:20, Sergey Ulanov wrote:
> This and code below it doesn't need to be in try-catch

Done.

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/HostListLoader.java:143:
Log.e("hostlist", "Unexpected error while fetching host list: " + ex);
On 2014/02/14 08:55:20, Sergey Ulanov wrote:
> Throw RuntimeException?

Done.

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/HostListLoader.java:144: }
catch (JSONException ex) {
On 2014/02/14 08:55:20, Sergey Ulanov wrote:
> I suggest having two separate try-catch block. One for the HTTP request, and
one
> to parse the response. That way you wouldn't need to mix exception handling. 

Done.

https://codereview.chromium.org/157013002/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/HostListLoader.java:168:
Comparator<Host> compareHosts = new Comparator<Host>() {
On 2014/02/14 08:55:20, Sergey Ulanov wrote:
> nit: hostComparator

Done.

[Jamie, 2014-02-18 19:24:28]

LGTM since you've addressed all of Sergey's comments, but I'm adding him TBR to
sanity-check.

[Lambros, 2014-02-18 20:00:41]

The CQ bit was checked by lambroslambrou@chromium.org

[commit-bot: I haz the power, 2014-02-18 20:01:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/157013002/...

[commit-bot: I haz the power, 2014-02-18 21:02:50]

Change committed as 251838