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..c524303ab359981bd654c389322ae0002cf0146f
--- /dev/null
+++ b/chrome/browser/chromeos/system_logs/system_logs_fetcher.cc
@@ -0,0 +1,58 @@
+// 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/commandline_log_source.h"
+#include "chrome/browser/chromeos/system_logs/debugd_log_source.h"
+#include "chrome/browser/chromeos/system_logs/memorydetails_log_source.h"
+#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)) {
+
+  // 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();
+}
+
+void SystemLogsFetcher::Fetch(const SysLogsFetcherCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

[satorux1, 2012/08/17 17:46:29]

like elsewhere, might want to add

DCHECK(callback_.is_null()) to ensure this function is not called twice.

[tudalex(Chromium), 2012/08/17 19:43:22]

Done.

+
+  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));
+
+  response_->insert(response->begin(), response->end());

[satorux1, 2012/08/17 17:46:29]

I think this code assume that keys are unique. However, looking at the code,
this assumption is very fragile.

Can you add a prefix for each source, like "routes" ->
"DebugDaemonSource:routes". I guess this requires a lot of changes elsewhere, so
you might want to do this in a separate patch. What do you think?

[tudalex(Chromium), 2012/08/17 19:43:22]

I documented in the SystemLogsFetcher class comment that the  keys that are not
unique are not added.
There was a prefix added for debug which was removed in this CL
https://chromiumcodereview.appspot.com/10825260. 
I think it's up to the person who writes the data source to be careful to
specify a unique name for the key, because we don't want to encourage having
duplicate data in our logs.
rkc@ what do you say?

[satorux1, 2012/08/17 19:47:17]

"be careful" is not a good practice in software development. we should make sure
it won't happen wherever we can.

That said, instead of insert() everything at once, we should add a key-value
one-by-one, and DCHECK if there is no duplicate of a key. you can do it by
checking the return value of insert().

[rkc, 2012/08/17 19:48:27]

If a key appears from another data source, we overwrite the value, that's our
intended behavior.

Prefixing keys with anything is directly visible to the user which will be bad.
We want the user to see the data in the same way that they were getting till
now, all this code is only to make our adding sources and maintaining sources
much easier and cleaner. There should be no user impact if possible.

I'd suggest that we mitigate the duplicate keys issue by DCHECK'ing on it. That
way if someone accidentally adds a duplicate key, they'd know about it
(hopefully) before they commit :)

On 2012/08/17 19:43:22, tudalex1 wrote:

[tudalex(Chromium), 2012/08/17 21:34:54]

Done.

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