Refactoring the SysInfoProvider.

chrome/browser/chromeos/syslogs/syslogs_fetcher.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/syslogs/syslogs_fetcher.h"
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "content/public/browser/browser_thread.h"
+
+using content::BrowserThread;
+
+namespace chromeos {
+
+
+SysLogsFetcher::SysLogsFetcher(SysLogsDataSources backends)
+    : data_sources_(backends), response_(new SysLogsResponse),
+      responses_(backends.size()),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {}
+
+void SysLogsFetcher::Fetch(SysLogsFetcherCallback request) {
+  request_ = request;
+  SysLogsDataSources::iterator it;
+  responses_ = data_sources_.size();
+  for (it = data_sources_.begin(); it != data_sources_.end(); ++it) {
+    (*it)->Fetch(base::Bind(&SysLogsFetcher::AddData,
+                            weak_ptr_factory_.GetWeakPtr()));
+  }
+}
+
+void SysLogsFetcher::AddData(SysLogsResponse* data) {
+  base::AutoLock lock(response_lock_);

[satorux1, 2012/08/07 01:15:07]

why is Lock needed? On what thread this function is called?

This should be called on the same thread where Fetch() is called? Can this be
called from another thread?

Please add a DCHECK to ensure functions are called on expected threads.


I'd recommend to add the following to all functions in the file, assuming the
expected thread is UI thread.

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

[tudalex(Chromium), 2012/08/07 23:46:54]

Lock was not needed. Added checks and also moved the execution of some of the
data sources on another thread. Added a comment in the header to reflect this.

[satorux1, 2012/08/08 22:01:03]

I'd suggest add more of these DCHECKs in other files too. Seems to me that your
code runs on multiple threads. It's very helpful to clarify on what thread
functions are called

+
+  SysLogsResponse::iterator it;
+  for (it = data->begin(); it != data->end(); ++it){
+    (*response_)[it->first] = it->second;
+  }
+  delete data;
+
+  if (--responses_)
+    return;
+
+  SysLogsDataSources::iterator ds_it;
+  for (ds_it = data_sources_.begin(); ds_it != data_sources_.end(); ++ds_it) {
+    delete *ds_it;
+  }
+  request_.Run(response_);
+  BrowserThread::DeleteSoon(BrowserThread::UI, FROM_HERE, this);
+}
+
+}  // namespace chromeos
+