Refactoring the SysInfoProvider.

chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc

Index: chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc
diff --git a/chrome/browser/chromeos/system/sysinfo_provider.cc b/chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc
similarity index 54%
rename from chrome/browser/chromeos/system/sysinfo_provider.cc
rename to chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc
index 4869a104161b0cedbee86c8539e154295d58cfa3..3569690d5d8b0fa308318f9876600ff97e015f66 100644
--- a/chrome/browser/chromeos/system/sysinfo_provider.cc
+++ b/chrome/browser/chromeos/syslogs/debugd_log_fetcher.cc
@@ -2,29 +2,21 @@
 // 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/sysinfo_provider.h"
+#include "chrome/browser/chromeos/syslogs/debugd_log_fetcher.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
-#include "base/command_line.h"
-#include "base/file_path.h"
-#include "base/file_util.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/string_util.h"
-#include "base/synchronization/waitable_event.h"
-#include "base/threading/thread.h"
-#include "chrome/browser/memory_details.h"
+#include "chrome/browser/chromeos/syslogs/syslogs_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"
 
-using content::BrowserThread;
-
 namespace chromeos {
-namespace system {
 
 // Fetches logs from the debug daemon over DBus. When all the logs have been
 // fetched, forwards the results to the supplied Request. Used like:
@@ -32,45 +24,21 @@ namespace system {
 //   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.
-class DebugDaemonLogFetcher {
- public:
-  typedef
-      scoped_refptr<CancelableRequest<SysInfoProvider::FetchCompleteCallback> >
-      Request;
-
-  explicit DebugDaemonLogFetcher(Request request);
-  ~DebugDaemonLogFetcher();
-
-  // Fetches logs from the daemon over dbus. After the fetch is complete, the
-  // results will be forwarded to the request supplied to the constructor and
-  // this instance will free itself.
-  void Fetch();
- private:
-  // Callbacks
-  void GotRoutes(bool succeeded, const std::vector<std::string>& routes);
-  void GotNetworkStatus(bool succeeded, const std::string& status);
-  void GotModemStatus(bool succeeded, const std::string& status);
-  void GotLogs(bool succeeded, const std::map<std::string, std::string>& logs);
-  void RequestCompleted();
-
-  SysInfoResponse* response_;
-  Request request_;
-  int pending_requests_;
-  base::WeakPtrFactory<DebugDaemonLogFetcher> weak_ptr_factory_;
-};
-
-DebugDaemonLogFetcher::DebugDaemonLogFetcher(Request request)
-    : response_(new SysInfoResponse()), request_(request),
-      weak_ptr_factory_(this) { }
-
-DebugDaemonLogFetcher::~DebugDaemonLogFetcher() { }
-
-void DebugDaemonLogFetcher::Fetch() {
+
+
+DebugDaemonLogFetcher::DebugDaemonLogFetcher()
+    : response_(new SysLogsResponse()),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {}
+
+DebugDaemonLogFetcher::~DebugDaemonLogFetcher() {}
+
+void DebugDaemonLogFetcher::Fetch(const SysLogsFetcherCallback& request) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  request_ = request;
   DebugDaemonClient* client = DBusThreadManager::Get()->GetDebugDaemonClient();
   pending_requests_ = 4;
-  // Note that this use of base::Unretained(this) is safe, since |this| is
-  // deleted in RequestCompleted if and only if all outstanding callbacks are
-  // done.
+
   client->GetRoutes(true, false,

[satorux1, 2012/08/09 21:15:07]

nit: when parameters cannot fit in one line, it's usually a good idea to align
them vertically. More importantly, true/false here are cryptic. Please add some
comment to clarify.

client->GetRoutes(
    true,  // add comment here
    false,  // add comment here
    base::Bind(...)

[tudalex(Chromium), 2012/08/10 00:30:31]

Done.

       base::Bind(&DebugDaemonLogFetcher::GotRoutes,
                  weak_ptr_factory_.GetWeakPtr()));
@@ -85,10 +53,11 @@ void DebugDaemonLogFetcher::Fetch() {
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
-const char *kNotAvailable = "<not available>";
+const char* kNotAvailable = "<not available>";

[satorux1, 2012/08/09 21:15:07]

please put this in anonymous namespace. also please do:

const char[] kNotAvailable = "..."

this way is more efficient for an esoteric reason. 
http://udrepper.livejournal.com/13851.html

[tudalex(Chromium), 2012/08/10 00:30:31]

Done.

 
 void DebugDaemonLogFetcher::GotRoutes(bool succeeded,
                                       const std::vector<std::string>& routes) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

[satorux1, 2012/08/09 21:15:07]

nit: add a blank line after DCHECK. Please fix elsewhere too

[tudalex(Chromium), 2012/08/10 00:30:31]

Done.

   if (succeeded)
     (*response_)["routes"] = JoinString(routes, '\n');
   else
@@ -98,6 +67,7 @@ void DebugDaemonLogFetcher::GotRoutes(bool succeeded,
 
 void DebugDaemonLogFetcher::GotNetworkStatus(bool succeeded,
                                              const std::string& status) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   if (succeeded)
     (*response_)["network-status"] = status;
   else
@@ -107,6 +77,7 @@ void DebugDaemonLogFetcher::GotNetworkStatus(bool succeeded,
 
 void DebugDaemonLogFetcher::GotModemStatus(bool succeeded,
                                            const std::string& status) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   if (succeeded)
     (*response_)["modem-status"] = status;
   else
@@ -120,6 +91,7 @@ void DebugDaemonLogFetcher::GotLogs(bool /* succeeded */,
   // 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.
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

[satorux1, 2012/08/09 21:15:07]

move this to the beginning of the function followed by a blank line. DCHECK()s
are usually place at the beginning, followed by a blank line

[tudalex(Chromium), 2012/08/10 00:30:31]

Done.

   for (std::map<std::string, std::string>::const_iterator it = logs.begin();
        it != logs.end(); ++it)
     (*response_)[it->first] = it->second;
@@ -127,35 +99,11 @@ void DebugDaemonLogFetcher::GotLogs(bool /* succeeded */,
 }
 
 void DebugDaemonLogFetcher::RequestCompleted() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   if (--pending_requests_)
     return;
-  request_->ForwardResult(response_);
-  BrowserThread::DeleteSoon(BrowserThread::UI, FROM_HERE, this);
-}
-
-class SysInfoProviderImpl : public SysInfoProvider {
- public:
-  virtual Handle Fetch(CancelableRequestConsumerBase* consumer,
-                       const FetchCompleteCallback& callback);
-};
-
-CancelableRequestProvider::Handle SysInfoProviderImpl::Fetch(
-    CancelableRequestConsumerBase* consumer,
-    const FetchCompleteCallback& callback) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  scoped_refptr<CancelableRequest<FetchCompleteCallback> >
-      request(new CancelableRequest<FetchCompleteCallback>(callback));
-  AddRequest(request, consumer);
-  // Deleted by DebugDaemonLogFetcher::RequestCompleted()
-  DebugDaemonLogFetcher* fetcher = new DebugDaemonLogFetcher(request);
-  fetcher->Fetch();
-
-  return request->handle();
-}
+  request_.Run(response_);
 
-SysInfoProvider* SysInfoProvider::Create() {
-  return new SysInfoProviderImpl();
 }
 
-} // namespace system
 } // namespace chromeos