Refactoring the SysInfoProvider.

chrome/browser/ui/webui/chromeos/system_info_ui.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/ui/webui/chromeos/system_info_ui.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/memory/ref_counted_memory.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop.h"
 #include "base/path_service.h"
 #include "base/string_piece.h"
 #include "base/string_util.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/thread.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/cros/cros_library.h"
-#include "chrome/browser/chromeos/system/sysinfo_provider.h"
-#include "chrome/browser/chromeos/system/syslogs_provider.h"
+#include "chrome/browser/chromeos/system_logs/system_logs_fetcher.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/webui/chrome_url_data_manager.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/jstemplate_builder.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui.h"
 #include "content/public/browser/web_ui_message_handler.h"
 #include "grit/browser_resources.h"
@@ skipping 18 matching lines @@
   virtual void StartDataRequest(const std::string& path,
                                 bool is_incognito,
                                 int request_id) OVERRIDE;
   virtual std::string GetMimeType(const std::string&) const OVERRIDE {
     return "text/html";
   }
 
  private:
   ~SystemInfoUIHTMLSource() {}
 
-  void SyslogsComplete(chromeos::system::LogDictionaryType* sys_info,
-                       std::string* ignored_content);
-  void SysInfoComplete(chromeos::system::SysInfoResponse* response);
+  void SysInfoComplete(chromeos::SystemLogsResponse* response);

[satorux1, 2012/08/14 05:53:21]

I think you can put the code in the file in 'chromeos' namespace, then you can
remove all chromeos:: namespace. That'd be cleaner.

[tudalex(Chromium), 2012/08/15 01:54:33]

Done.

   void RequestComplete();
   void WaitForData();
 
-  CancelableRequestConsumer logs_consumer_;
-  CancelableRequestConsumer sys_info_consumer_;
-
   // Stored data from StartDataRequest()
   std::string path_;
   int request_id_;
 
-  int pending_requests_;
-  chromeos::system::LogDictionaryType* logs_;
-  chromeos::system::SysInfoResponse* sys_info_;
-  scoped_ptr<chromeos::system::SysInfoProvider> sys_info_provider_;
-
+  scoped_ptr<chromeos::SystemLogsResponse> response_;
+  base::WeakPtrFactory<SystemInfoUIHTMLSource> weak_ptr_factory_;
   DISALLOW_COPY_AND_ASSIGN(SystemInfoUIHTMLSource);
 };
 
 // The handler for Javascript messages related to the "system" view.
 class SystemInfoHandler : public WebUIMessageHandler,
                           public base::SupportsWeakPtr<SystemInfoHandler> {
  public:
   SystemInfoHandler();
   virtual ~SystemInfoHandler();
 
   // WebUIMessageHandler implementation.
   virtual void RegisterMessages() OVERRIDE;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SystemInfoHandler);
 };
 
 ////////////////////////////////////////////////////////////////////////////////
 //
 // SystemInfoUIHTMLSource
 //
 ////////////////////////////////////////////////////////////////////////////////
 
 SystemInfoUIHTMLSource::SystemInfoUIHTMLSource()
     : DataSource(chrome::kChromeUISystemInfoHost, MessageLoop::current()),
-      request_id_(0), logs_(NULL), sys_info_(NULL),
-      sys_info_provider_(chromeos::system::SysInfoProvider::Create()) {
+      request_id_(0), response_(NULL),

[satorux1, 2012/08/14 05:51:52]

move response_(NULL) to the next line.

[tudalex(Chromium), 2012/08/15 01:54:33]

Done.

+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
 }
 
 void SystemInfoUIHTMLSource::StartDataRequest(const std::string& path,
                                               bool is_incognito,
                                               int request_id) {
   path_ = path;
   request_id_ = request_id;
-  pending_requests_ = 0;
 
-  chromeos::system::SyslogsProvider* provider =
-      chromeos::system::SyslogsProvider::GetInstance();
-  if (provider) {
-    provider->RequestSyslogs(
-        false,  // don't compress.
-        chromeos::system::SyslogsProvider::SYSLOGS_SYSINFO,
-        &logs_consumer_,
-        base::Bind(&SystemInfoUIHTMLSource::SyslogsComplete,
-                   base::Unretained(this)));
-    pending_requests_++;
-  }
-
-  sys_info_provider_->Fetch(&sys_info_consumer_,
-                            base::Bind(
-                                &SystemInfoUIHTMLSource::SysInfoComplete,
-                                base::Unretained(this)));
-  pending_requests_++;
+  chromeos::SystemLogsFetcher * fetcher = new chromeos::SystemLogsFetcher();
+  fetcher->Fetch(base::Bind(
+                    &SystemInfoUIHTMLSource::SysInfoComplete,
+                    weak_ptr_factory_.GetWeakPtr()));
 }
 
-void SystemInfoUIHTMLSource::SyslogsComplete(
-    chromeos::system::LogDictionaryType* sys_info,
-    std::string* ignored_content) {
-  logs_ = sys_info;
-  RequestComplete();
-}
 
 void SystemInfoUIHTMLSource::SysInfoComplete(
-    chromeos::system::SysInfoResponse* sys_info) {
-  sys_info_ = sys_info;
+    chromeos::SystemLogsResponse* sys_info) {
+  response_.reset(sys_info);
   RequestComplete();
 }
 
 void SystemInfoUIHTMLSource::RequestComplete() {
-  if (--pending_requests_)
-    return;
-
   DictionaryValue strings;
   strings.SetString("title", l10n_util::GetStringUTF16(IDS_ABOUT_SYS_TITLE));
   strings.SetString("description",
                     l10n_util::GetStringUTF16(IDS_ABOUT_SYS_DESC));
   strings.SetString("table_title",
                     l10n_util::GetStringUTF16(IDS_ABOUT_SYS_TABLE_TITLE));
   strings.SetString("expand_all_btn",
                     l10n_util::GetStringUTF16(IDS_ABOUT_SYS_EXPAND_ALL));
   strings.SetString("collapse_all_btn",
                     l10n_util::GetStringUTF16(IDS_ABOUT_SYS_COLLAPSE_ALL));
   strings.SetString("expand_btn",
                     l10n_util::GetStringUTF16(IDS_ABOUT_SYS_EXPAND));
   strings.SetString("collapse_btn",
                     l10n_util::GetStringUTF16(IDS_ABOUT_SYS_COLLAPSE));
   SetFontAndTextDirection(&strings);
-
-  if (logs_ || sys_info_) {
+  if (response_ != NULL) {

[satorux1, 2012/08/14 05:51:52]

It's ok to say if (response_) {

[tudalex(Chromium), 2012/08/15 01:54:33]

I can't say it. It gives me an error that it can't convert scoped_ptr to bool.

[satorux1, 2012/08/15 02:12:36]

Then do this if (response_.get())

[tudalex(Chromium), 2012/08/15 17:20:21]

Done.

     ListValue* details = new ListValue();
     strings.Set("details", details);
-    if (logs_) {
-      chromeos::system::LogDictionaryType::iterator it;
-      for (it = logs_->begin(); it != logs_->end(); ++it) {
-        DictionaryValue* val = new DictionaryValue;
-        // Skip the 'debugd' key; this is a duplicate of all the logs that
-        // we're already getting from the debugd daemon. This will be fixed
-        // with http://code.google.com/p/chromium/issues/detail?id=138582
-        // which incidentally will also remove this code.
-        if (it->first == "debugd")
-          continue;
+    chromeos::SystemLogsResponse::iterator it;

[satorux1, 2012/08/14 05:51:52]

this is not fixed. please move this inside of for

for (chromeos::SystemLogsResponse::iterator it = ...; 

Minimizing scope is a good thing.

[tudalex(Chromium), 2012/08/15 01:54:33]

Sorry. I forgot to save the file in eclipse with the fix. Done now.

+    for (it = response_->begin(); it != response_->end(); ++it) {
+      DictionaryValue* val = new DictionaryValue;
+      val->SetString("stat_name", it->first);
+      val->SetString("stat_value", it->second);
+      details->Append(val);
 

[satorux1, 2012/08/14 05:51:52]

no blank line here.

[tudalex(Chromium), 2012/08/15 01:54:33]

Done.

-        val->SetString("stat_name", it->first);
-        val->SetString("stat_value", it->second);
-        details->Append(val);
-      }
-      delete logs_;
-    }
-    if (sys_info_) {
-      chromeos::system::SysInfoResponse::iterator it;
-      for (it = sys_info_->begin(); it != sys_info_->end(); ++it) {
-        DictionaryValue* val = new DictionaryValue;
-        val->SetString("stat_name", it->first);
-        val->SetString("stat_value", it->second);
-        details->Append(val);
-      }
-      delete sys_info_;
     }
     strings.SetString("anchor", path_);
   }
   static const base::StringPiece systeminfo_html(
       ResourceBundle::GetSharedInstance().GetRawDataResource(
           IDR_ABOUT_SYS_HTML, ui::SCALE_FACTOR_NONE));
   std::string full_html = jstemplate_builder::GetTemplatesHtml(
       systeminfo_html, &strings, "t" /* template root node id */);
-
   SendResponse(request_id_, base::RefCountedString::TakeString(&full_html));
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 //
 // SystemInfoHandler
 //
 ////////////////////////////////////////////////////////////////////////////////
 SystemInfoHandler::SystemInfoHandler() {
 }
@@ skipping 13 matching lines @@
 
 SystemInfoUI::SystemInfoUI(content::WebUI* web_ui) : WebUIController(web_ui) {
   SystemInfoHandler* handler = new SystemInfoHandler();
   web_ui->AddMessageHandler(handler);
   SystemInfoUIHTMLSource* html_source = new SystemInfoUIHTMLSource();
 
   // Set up the chrome://system/ source.
   Profile* profile = Profile::FromWebUI(web_ui);
   ChromeURLDataManager::AddDataSource(profile, html_source);
 }