Refactoring the SysInfoProvider.

chrome/browser/chromeos/system_logs/debugd_log_source.cc

+// Copyright (c) 2012 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 "chrome/browser/chromeos/system_logs/debugd_log_source.h"
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/string_util.h"
+#include "chrome/browser/chromeos/system_logs/system_logs_fetcher.h"
+#include "chrome/common/chrome_switches.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/debug_daemon_client.h"
+#include "content/public/browser/browser_thread.h"
+
+namespace {
+
+const char kNotAvailable[] = "<not available>";
+
+}  // namespace
+
+namespace chromeos {
+
+// Fetches logs from the debug daemon over DBus. When all the logs have been
+// fetched, forwards the results to the supplied Request. Used like:
+//   DebugDaemonLogSource* fetcher = new DebugDaemonLogSource(request);
+//   fetcher->Fetch();
+// Note that you do not need to delete the fetcher; it will delete itself after
+// Fetch() has forwarded the result to the request handler.
+
+
+DebugDaemonLogSource::DebugDaemonLogSource()
+    : response_(new SystemLogsResponse()), num_pending_requests_(0),

[satorux1, 2012/08/14 05:51:52]

please move num_pending_requests_(0) to the next line. i.e. please align members
vertically.

[tudalex(Chromium), 2012/08/15 01:54:33]

Done.

+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {}

[satorux1, 2012/08/14 05:51:52]

please also initialize request_(NULL).

Be sure to initialize all pointers and primitive types like int. Please check
other classes too.

[tudalex(Chromium), 2012/08/15 01:54:33]

Request is a callback and can't initialize it to null. It gives an error saying
it can't convert from long int to base::Callback. I initialized it with
SysLogsFetcherCallback.

[satorux1, 2012/08/15 02:11:32]

Sorry, I thought request_  was just a pointer. If it's a callback, you don't
have to initialize explicitly.

BTW, I think the source of the confusion is that request_ is an unusual name for
a callback. Please rename it to something like response_handler_ or something

[satorux1, 2012/08/15 02:22:43]

or just callback_

[tudalex(Chromium), 2012/08/15 17:20:21]

Done. Renamed everything with callback_. Also did it in all the other classes.
Sorry for the confusion.

+
+DebugDaemonLogSource::~DebugDaemonLogSource() {}
+
+void DebugDaemonLogSource::Fetch(const SysLogsFetcherCallback& request) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

[satorux1, 2012/08/14 05:51:52]

DCHECK(!request_);

[tudalex(Chromium), 2012/08/15 01:54:33]

Says error: no match for 'operator!' in '!request'. Guess because it's a
callback. Do you know any equivalent to DCHECK(!request_)?

[satorux1, 2012/08/15 22:01:09]

DCHECK(callback.is_null());

[tudalex(Chromium), 2012/08/16 01:08:57]

Done.

+
+  request_ = request;
+  DebugDaemonClient* client = DBusThreadManager::Get()->GetDebugDaemonClient();
+
+  client->GetRoutes(true,   // Numeric
+                    false,  // No IPv6
+                    base::Bind(&DebugDaemonLogSource::OnGetRoutes,
+                               weak_ptr_factory_.GetWeakPtr()));
+  ++num_pending_requests_;
+  client->GetNetworkStatus(base::Bind(&DebugDaemonLogSource::OnGetNetworkStatus,
+                                      weak_ptr_factory_.GetWeakPtr()));
+  ++num_pending_requests_;
+  client->GetModemStatus(base::Bind(&DebugDaemonLogSource::OnGetModemStatus,
+                                    weak_ptr_factory_.GetWeakPtr()));
+  ++num_pending_requests_;
+  client->GetAllLogs(base::Bind(&DebugDaemonLogSource::OnGetLogs,
+                                weak_ptr_factory_.GetWeakPtr()));
+  ++num_pending_requests_;
+}
+
+void DebugDaemonLogSource::OnGetRoutes(bool succeeded,
+                                       const std::vector<std::string>& routes) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  if (succeeded)
+    (*response_)["routes"] = JoinString(routes, '\n');
+  else
+    (*response_)["routes"] = kNotAvailable;
+  RequestCompleted();
+}
+
+void DebugDaemonLogSource::OnGetNetworkStatus(bool succeeded,
+                                              const std::string& status) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  if (succeeded)
+    (*response_)["network-status"] = status;
+  else
+    (*response_)["network-status"] = kNotAvailable;
+  RequestCompleted();
+}
+
+void DebugDaemonLogSource::OnGetModemStatus(bool succeeded,
+                                            const std::string& status) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  if (succeeded)
+    (*response_)["modem-status"] = status;
+  else
+    (*response_)["modem-status"] = kNotAvailable;
+  RequestCompleted();
+}
+
+void DebugDaemonLogSource::OnGetLogs(bool /* succeeded */,
+                                     const std::map<std::string,
+                                                    std::string>& logs) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  // We ignore 'succeeded' for this callback - we want to display as much of the
+  // debug info as we can even if we failed partway through parsing, and if we
+  // couldn't fetch any of it, none of the fields will even appear.
+  for (std::map<std::string, std::string>::const_iterator it = logs.begin();
+       it != logs.end(); ++it)
+    (*response_)[it->first] = it->second;
+  RequestCompleted();
+}
+
+void DebugDaemonLogSource::RequestCompleted() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  --num_pending_requests_;
+  if (num_pending_requests_ > 0)
+    return;
+  request_.Run(response_.get());
+}
+
+}  // namespace chromeos