Retrieved Hostlist information from Directory service.

remoting/test/chromoting_host_info.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/test/chromoting_host_info.h"
+
+#include "base/logging.h"
+
+namespace {
+const char kOnlineStatusResponse[] = "ONLINE";

[Sergey Ulanov, 2015/07/07 00:01:45]

In general it's not useful to declare consts for strings like this, except when
they are used in multiple places. So I suggest moving "ONLINE" inline - that
would make the code more readable.

[tonychun, 2015/07/08 03:12:12]

Done.

[tonychun, 2015/07/08 03:12:13]

Done.

+const char kOfflineStatusResponse[] = "OFFLINE";
+}
+
+namespace remoting {
+namespace test {
+
+ChromotingHostInfo::ChromotingHostInfo(const base::DictionaryValue& item) {

[joedow, 2015/07/06 22:19:12]

instead of item, can you call it host_info or something more descriptive?

[tonychun, 2015/07/08 03:12:13]

Done.

+  const base::ListValue* listValue = nullptr;

[joedow, 2015/07/06 22:19:12]

listValue should be list_value.  This codebase does not use camel case for var
names.

[tonychun, 2015/07/08 03:12:13]

Done.

+  item.GetList("tokenUrlPatterns", &listValue);
+
+  // Add TokenUrlPatterns to ChromotingHostInfo
+  if (listValue != nullptr) {

[joedow, 2015/07/06 22:19:12]

This is cleaner as 'if (listValue) {'

[Sergey Ulanov, 2015/07/07 00:01:45]

DictionaryValue::GetList() returns bool, so this can be
  if (item.GetList("tokenUrlPatterns", &listValue)) {
   ...
  }

[tonychun, 2015/07/08 03:12:12]

Done.

[tonychun, 2015/07/08 03:12:12]

Done.

+    int size = listValue->GetSize();
+    if (size > 0) {
+      std::string tokenUrlPattern;

[joedow, 2015/07/06 22:19:12]

token_url_pattern

[tonychun, 2015/07/08 03:12:13]

Done.

+      for (int i = 0; i < size; ++i) {
+        listValue->GetString(i, &tokenUrlPattern);
+        if (!tokenUrlPattern.empty()) {
+          tokenUrlPatterns.push_back(tokenUrlPattern);
+        }
+      }
+    }
+  }
+
+  item.GetString("hostId", &host_id);

[Sergey Ulanov, 2015/07/07 00:01:45]

Need to handle the case when any of the fields is missing. To do that you need
to be able to return errors from this function, and it's not possible to return
an error from a constructor. I suggest moving this parsing code to a separate
Parse() method and initializing all fields to defaults in the constructor. E.g.
see JingleMessage struct for example:
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/protocol/...

[tonychun, 2015/07/08 03:12:13]

Done.

+  item.GetString("jabberId", &host_jid);
+  item.GetString("hostName", &host_name);
+  item.GetString("hostOfflineReason", &offline_reason);
+  item.GetString("publicKey", &public_key);
+
+  std::string response_status;
+  item.GetString("status", &response_status);
+  if (response_status == kOnlineStatusResponse) {
+    status = kChromotingHostStatusOnline;
+  } else {
+    DCHECK(response_status == kOfflineStatusResponse);

[joedow, 2015/07/06 22:19:12]

A switch might be cleaner here (with a default case that logs the
response_status value and adds an UNREACHED macro.

[Sergey Ulanov, 2015/07/07 00:01:45]

You shouldn't DCHECK on server response (or on any information received from
outside). Logging a message and returning an error would be more appropriate
here.

[tonychun, 2015/07/08 03:12:13]

I put in NOTREACHED().

[tonychun, 2015/07/08 03:12:13]

Done.

+    status = kChromotingHostStatusOffline;
+  }
+}
+
+ChromotingHostInfo::~ChromotingHostInfo() {
+}
+
+}  // namespace test
+}  // namespace remoting