Refactoring the SysInfoProvider.

chrome/browser/chromeos/system_logs/system_logs_fetcher.cc

Index: chrome/browser/chromeos/system_logs/system_logs_fetcher.cc
diff --git a/chrome/browser/chromeos/system_logs/system_logs_fetcher.cc b/chrome/browser/chromeos/system_logs/system_logs_fetcher.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e3c1862a0a336ee209eef5aa36f153e7035a81b2
--- /dev/null
+++ b/chrome/browser/chromeos/system_logs/system_logs_fetcher.cc
@@ -0,0 +1,69 @@
+// 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/system_logs_fetcher.h"
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "chrome/browser/chromeos/system_logs/command_line_log_source.h"
+#include "chrome/browser/chromeos/system_logs/debug_daemon_log_source.h"
+#include "chrome/browser/chromeos/system_logs/memory_details_log_source.h"

[Lei Zhang, 2012/08/21 19:56:55]

nit: alphabetical ordering

[tudalex(Chromium), 2012/08/21 21:21:12]

Done.

+#include "chrome/browser/chromeos/system_logs/lsb_release_log_source.h"
+#include "content/public/browser/browser_thread.h"
+
+using content::BrowserThread;
+
+namespace chromeos {
+
+SystemLogsFetcher::SystemLogsFetcher()
+    : response_(new SystemLogsResponse),
+      num_pending_requests_(0),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
+

[Lei Zhang, 2012/08/21 19:56:55]

nit: no blank lines at the top of functions.

[tudalex(Chromium), 2012/08/21 21:21:12]

Done.

+  // Debug Daemon data source.
+  data_sources_.push_back(new DebugDaemonLogSource());
+
+  // Chrome data sources.
+  data_sources_.push_back(new CommandLineLogSource());
+  data_sources_.push_back(new LsbReleaseLogSource());
+  data_sources_.push_back(new MemoryDetailsLogSource());
+  num_pending_requests_ = data_sources_.size();
+}
+
+SystemLogsFetcher::~SystemLogsFetcher() {}
+
+void SystemLogsFetcher::Fetch(const SysLogsFetcherCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(callback_.is_null());
+  DCHECK(!callback.is_null());
+
+  callback_ = callback;
+  for (size_t i = 0; i < data_sources_.size(); ++i) {
+    data_sources_[i]->Fetch(base::Bind(&SystemLogsFetcher::AddResponse,
+                                       weak_ptr_factory_.GetWeakPtr()));
+  }
+}
+
+void SystemLogsFetcher::AddResponse(SystemLogsResponse* response) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  for (SystemLogsResponse::iterator it = response->begin();

[Lei Zhang, 2012/08/21 19:56:55]

const_iterator?

[tudalex(Chromium), 2012/08/21 21:21:12]

Done.

+       it != response->end();
+       ++it) {
+    // It is false if the insert if there is already an element with the same
+    // key.
+    DCHECK(response_->insert(*it).second);

[Lei Zhang, 2012/08/21 19:56:55]

If you do this inside a DCHECK(), it won't actually get called in production
releases!

[rkc1, 2012/08/21 20:02:33]

That's intended, we don't want to crash in production on this. This is put in
here so developers know that there are conflicting keys.

[Lei Zhang, 2012/08/21 20:08:15]

In that case, put the entire loop inside a #if !defined(NDEBUG)

[satorux1, 2012/08/21 20:48:31]

I think Lei was right, and it was a great catch. We should not use DCHECK here.
Otherwise, the key/value pair is not inserted at all.

[rkc1, 2012/08/21 20:50:01]

Yeah, I realized that. tudalex@ is correcting it.

[tudalex(Chromium), 2012/08/21 21:21:12]

Lei was right in the first place. Moved it outside.

+  }
+
+
+  --num_pending_requests_;
+  if (num_pending_requests_ > 0)
+    return;
+
+  callback_.Run(response_.Pass());
+  BrowserThread::DeleteSoon(BrowserThread::UI, FROM_HERE, this);
+}
+
+}  // namespace chromeos
+