Sort the host list.

Sort the host list.

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

[Jamie, 2014-02-05 23:27:14]

ptal

[Lambros, 2014-02-06 00:14:05]

lgtm with style quibbles

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

https://codereview.chromium.org/148313004/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:330: private
JSONArray sortHosts(JSONArray hosts) {
nit: This should be static.

I'm in two minds whether this sorting belongs in HostListDirectoryGrabber, or in
updateUi().

With the former, the advantage is that the sorting will happen on the network
thread, potentially keeping the UI more responsive. But it seems less flexible?

https://codereview.chromium.org/148313004/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:336: // Ignore
non-object entries.
nit: 4-space indent

https://codereview.chromium.org/148313004/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:346: return
-1;
nit: 4-space indent

https://codereview.chromium.org/148313004/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:347: } else if
(bOnline && !aOnline) {
nit: No 'else' after return.

https://codereview.chromium.org/148313004/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:348: return 1;
nit: 4-space indent

https://codereview.chromium.org/148313004/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:363: 
nit: Remove blank line.

[Jamie, 2014-02-06 00:47:15]

fyi

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

https://codereview.chromium.org/148313004/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:330: private
JSONArray sortHosts(JSONArray hosts) {
On 2014/02/06 00:14:06, Lambros wrote:
> nit: This should be static.

Apparently, non-static inner classes can't have static methods.

> I'm in two minds whether this sorting belongs in HostListDirectoryGrabber, or
in
> updateUi().
> 
> With the former, the advantage is that the sorting will happen on the network
> thread, potentially keeping the UI more responsive. But it seems less
flexible?

I think it makes sense to leave it where it is. We can revisit if we want to do
anything more dynamic when displaying hosts.

https://codereview.chromium.org/148313004/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:336: // Ignore
non-object entries.
On 2014/02/06 00:14:06, Lambros wrote:
> nit: 4-space indent

Done.

https://codereview.chromium.org/148313004/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:346: return
-1;
On 2014/02/06 00:14:06, Lambros wrote:
> nit: 4-space indent

Done.

https://codereview.chromium.org/148313004/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:347: } else if
(bOnline && !aOnline) {
On 2014/02/06 00:14:06, Lambros wrote:
> nit: No 'else' after return.

Done.

https://codereview.chromium.org/148313004/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:348: return 1;
On 2014/02/06 00:14:06, Lambros wrote:
> nit: 4-space indent

Done.

https://codereview.chromium.org/148313004/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:363: 
On 2014/02/06 00:14:06, Lambros wrote:
> nit: Remove blank line.

Done.

[Jamie, 2014-02-06 00:47:20]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2014-02-06 00:55:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/148313004/70001

[commit-bot: I haz the power, 2014-02-06 09:55:26]

Change committed as 249321