Refactoring the SysInfoProvider.

chrome/browser/chromeos/syslogs/syslogs_fetcher.h

+// 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.
+
+#ifndef CHROME_BROWSER_CHROMEOS_SYSLOGS_SYSLOGS_FETCHER_H_
+#define CHROME_BROWSER_CHROMEOS_SYSLOGS_SYSLOGS_FETCHER_H_
+
+#include <string>
+#include <map>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/memory/weak_ptr.h"
+
+namespace chromeos {
+
+typedef std::map<std::string, std::string> SysLogsResponse;
+typedef base::Callback<void(SysLogsResponse*)> SysLogsFetcherCallback;
+
+// The SysLogsInterface provides a interface for the data sources that the
+// SysLogsFetcher class uses to fetch logs and other information.
+class SysLogsInterface {

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

I should have noticed earlier but the class name is confusing.  In the context
of Unix, "syslog" is an established mechanism for data logging:
http://en.wikipedia.org/wiki/Syslog

Here, this class is nothing to deal with syslog. Besides, names like "SysLogs"
should be avoided in favor of less abbreviation.

Something like SystemLogFetcherInterface would be a better name for this class.

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

It was named after SysInfoProvider which was named after SysLogsProvider :)
Changed it.

+ public:
+  // Fetches data and passes it by to the callback
+  virtual void Fetch(const SysLogsFetcherCallback& request) = 0;
+  virtual ~SysLogsInterface() {}
+};
+
+typedef std::vector<SysLogsInterface*> SysLogsDataSources;
+
+// The SysLogsFetcher receives a list of data sources which must be classes that
+// implement the SysLogsInterface. It's Fetch function receives as a parameter
+// a callback that takes only one parameter SysLogsResponse that is a map of
+// keys and values.
+// Each data source also returns a SysLogsResponse. If two data sources return
+// the same key, the last one will replace the previous one.
+// Each data source gets deleted after all of them are called, the
+// SysLogsFetcher deletes itself after calling the callback it received.
+// The class runs on the UI thread.
+//
+// EXAMPLE:
+// class Example {
+//   public:
+//      ProcessLogs(chrome::SysLogsResponse) {
+//        //do something with the logs
+//      }
+//      GetLogs() {
+//        chrome::SysLogsFetcher* fetcher =
+//          chrome::SysLogsFetcherFactory::GetInstance()->CreateFetcher();
+//        fetcher->Fetch(base::Bind(&Example::ProcessLogs,this));

[rkc, 2012/08/09 21:31:55]

Nit: &Example::ProcessLogs, this

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

Done.

+//      }
+class SysLogsFetcher {

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

This aggregates the data from multiple sources, right? If so, something like
AggregatedSystemLogFetcher may be good.

[rkc, 2012/08/09 23:50:50]

Satoru, AggregatedSystemLogFetcher is not quite an appropriate name for this
class; it is the global system log fetcher that fetches from all other 'log
sources'.

Instead, we could name this SystemLogsFetcher and instead name all the other
fetcher classes as LogSources,

So the interface would be named,

SystemLogsSource
from it, we'll derive, for example,

LsbReleaseLogSource
ChromeLogSource
CommandLineLogSource

WDYT?

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

I wrote it as AggregatedSystemLogFetcher for now.

[satorux1, 2012/08/10 22:58:34]

Source sounds good.

[tudalex(Chromium), 2012/08/11 00:27:06]

Done. Also renamed the folder from syslogs to system_logs and renamed the files
accordingly.

+ public:
+  explicit SysLogsFetcher();
+  ~SysLogsFetcher() {}
+
+  void Fetch(const SysLogsFetcherCallback& request);
+
+ private:
+  // Callback passed to all the data sources. It merges the |data| it recieves
+  // into response_. When all the data sources have responded, it deletes their
+  // objects and returns the response to the request_ callback. After this it
+  // deletes itself.
+  void AddData(SysLogsResponse* data);

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

AddResponse((SysLogsResponse* response) ?

"data" is usually not a good name. Here response looks better.

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

Done.

+
+  SysLogsDataSources data_sources_;
+  SysLogsFetcherCallback request_;
+
+  SysLogsResponse* response_; // The actual response data.
+  size_t responses_;          // The number of callbacks it should get.

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

Please add num_ prefix to make it clear it's a number.

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

Done.

+
+  base::WeakPtrFactory<SysLogsFetcher> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(SysLogsFetcher);
+};
+

[rkc, 2012/08/09 21:31:55]

Extra line.

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

Done.

+
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_SYSLOGS_SYSLOGS_FETCHER_H_
+