Fix feedback log collection.

chrome/browser/feedback/feedback_data.cc

Index: chrome/browser/feedback/feedback_data.cc
diff --git a/chrome/browser/feedback/feedback_data.cc b/chrome/browser/feedback/feedback_data.cc
index c2d320784aae028c181cf9dc22ee7ef9a9d7f1d2..3d09c60c80ecf9be470c823d005d82cf92c18aad 100644
--- a/chrome/browser/feedback/feedback_data.cc
+++ b/chrome/browser/feedback/feedback_data.cc
@@ -4,226 +4,105 @@
 
 #include "chrome/browser/feedback/feedback_data.h"
 
+#include "base/file_util.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
-#include "chrome/browser/extensions/extension_service.h"
-#include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/feedback/feedback_util.h"
 #include "chrome/browser/profiles/profile_manager.h"
-#include "chrome/browser/sync/about_sync_util.h"
-#include "chrome/browser/sync/profile_sync_service_factory.h"
-#include "chrome/common/extensions/extension.h"
-#include "chrome/common/extensions/extension_set.h"
 #include "content/public/browser/browser_thread.h"
 
-using content::BrowserThread;
-
-#if defined(OS_CHROMEOS)
-// TODO(rkc): Remove all the code that gather sync data and move it to a
-// log data source once crbug.com/138582 is fixed.
-namespace {
-
-const char kExtensionsListKey[] = "extensions";
-
-void AddSyncLogs(chromeos::system::LogDictionaryType* logs) {
-  Profile* profile = ProfileManager::GetDefaultProfile();
-  if (!ProfileSyncServiceFactory::GetInstance()->HasProfileSyncService(
-      profile))
-    return;
-
-  ProfileSyncService* service =
-      ProfileSyncServiceFactory::GetInstance()->GetForProfile(profile);
-  scoped_ptr<DictionaryValue> sync_logs(
-      sync_ui_util::ConstructAboutInformation(service));
-
-  // Remove credentials section.
-  ListValue* details = NULL;
-  sync_logs->GetList(kDetailsKey, &details);
-  if (!details)
-    return;
-  for (ListValue::iterator it = details->begin();
-      it != details->end(); ++it) {
-    DictionaryValue* dict = NULL;
-    if ((*it)->GetAsDictionary(&dict)) {
-      std::string title;
-      dict->GetString("title", &title);
-      if (title == kCredentialsTitle) {
-        details->Erase(it, NULL);
-        break;
-      }
-    }
-  }
-
-  // Add sync logs to logs.
-  std::string sync_logs_string;
-  JSONStringValueSerializer serializer(&sync_logs_string);
-  serializer.Serialize(*sync_logs.get());
-  (*logs)[kSyncDataKey] = sync_logs_string;
-}
-
-void AddExtensionInfoLogs(chromeos::system::LogDictionaryType* logs) {
-  bool reporting_enabled = false;
-  chromeos::CrosSettings::Get()->GetBoolean(chromeos::kStatsReportingPref,
-                                            &reporting_enabled);
-  if (!reporting_enabled)
-    return;
-
-  Profile* default_profile =
-      g_browser_process->profile_manager()->GetDefaultProfile();
-  if (!default_profile)
-    return;
-
-  ExtensionService* service =
-      extensions::ExtensionSystem::Get(default_profile)->extension_service();
-  if (!service)
-    return;
-
-  std::string extensions_list;
-  const ExtensionSet* extensions = service->extensions();
-  for (ExtensionSet::const_iterator it = extensions->begin();
-       it != extensions->end();
-       ++it) {
-    const extensions::Extension* extension = *it;
-    if (extensions_list.empty()) {
-      extensions_list = extension->name();
-    } else {
-      extensions_list += ", " + extension->name();
-    }
-  }
-
-  if (!extensions_list.empty())
-    (*logs)[kExtensionsListKey] = extensions_list;
-}
-
-}
-#endif // OS_CHROMEOS
-
-FeedbackData::FeedbackData()
-    : profile_(NULL)
-#if defined(OS_CHROMEOS)
-    , sys_info_(NULL)
-    , zip_content_(NULL)
-    , sent_report_(false)
-    , send_sys_info_(false)
+#if defined(USE_ASH)
+#include "ash/shell.h"
+#include "ash/shell_delegate.h"
 #endif
-{
-}
 
-FeedbackData::FeedbackData(Profile* profile,
-                           const std::string& category_tag,
-                           const std::string& page_url,
-                           const std::string& description,
-                           const std::string& user_email,
-                           ScreenshotDataPtr image
-#if defined(OS_CHROMEOS)
-                           , chromeos::system::LogDictionaryType* sys_info
-                           , std::string* zip_content
-                           , const std::string& timestamp
-                           , const std::string& attached_filename
-                           , std::string* attached_filedata
-#endif
-                           ) {
-  UpdateData(profile,
-             category_tag,
-             page_url,
-             description,
-             user_email,
-             image
-#if defined(OS_CHROMEOS)
-             , sys_info
-             , true
-             , timestamp
-             , attached_filename
-             , attached_filedata
-#endif
-             );
+using content::BrowserThread;
+
+FeedbackData::FeedbackData() : profile_(NULL),
+                               feedback_page_data_complete_(false) {

[xiyuan, 2013/03/19 21:15:09]

Should we initialize attached_filedata_(NULL) here?

[rkc, 2013/03/19 21:20:47]

Done.

 #if defined(OS_CHROMEOS)
-  sys_info_ = sys_info;
-  zip_content_ = zip_content;
+    sys_info_.reset(NULL);
+    send_sys_info_ = true;
+    syslogs_collection_complete_ = false;
+    read_attached_file_complete_ = false;

[xiyuan, 2013/03/19 21:15:09]

nit: 2-space indent

[rkc, 2013/03/19 21:20:47]

Done.

 #endif
 }
 
-
 FeedbackData::~FeedbackData() {
 }
 
-void FeedbackData::UpdateData(Profile* profile,
-                              const std::string& category_tag,
-                              const std::string& page_url,
-                              const std::string& description,
-                              const std::string& user_email,
-                              ScreenshotDataPtr image
-#if defined(OS_CHROMEOS)
-                              , const bool send_sys_info
-                              , const bool sent_report
-                              , const std::string& timestamp
-                              , const std::string& attached_filename
-                              , std::string* attached_filedata
-#endif
-                              ) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  profile_ = profile;
-  category_tag_ = category_tag;
-  page_url_ = page_url;
-  description_ = description;
-  user_email_ = user_email;
-  image_ = image;
+bool FeedbackData::DataCollectionComplete() {
 #if defined(OS_CHROMEOS)
-  send_sys_info_ = send_sys_info;
-  sent_report_ = sent_report;
-  timestamp_ = timestamp;
-  attached_filename_ = attached_filename;
-  attached_filedata_ = attached_filedata;
+  return (syslogs_collection_complete_ || !send_sys_info_) &&
+      read_attached_file_complete_ &&
+      feedback_page_data_complete_;
+#else
+  return feedback_page_data_complete_;
 #endif
 }
-
 void FeedbackData::SendReport() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-#if defined(OS_CHROMEOS)
-  if (sent_report_)
-    return;  // We already received the syslogs and sent the report.
+  if (DataCollectionComplete()) {
+    FeedbackUtil::SendReport(this);
+  }

[xiyuan, 2013/03/19 21:15:09]

nit: no enclosing braces for one-line branch

[rkc, 2013/03/19 21:20:47]

Done.

+}
 
-  // Set send_report_ to ensure that we only send one report.
-  sent_report_ = true;
+void FeedbackData::FeedbackPageDataComplete() {
+#if defined(OS_CHROMEOS)
+  if (attached_filename_.size() &&
+      base::FilePath::IsSeparator(attached_filename_[0]) &&
+      !attached_filedata_) {
+    // Read the attached file and then send this report.
+    attached_filedata_ = new std::string;

[xiyuan, 2013/03/19 21:15:09]

I assume this is going to released later.

[rkc, 2013/03/19 21:20:47]

Yep, in FeedbackUtil::SendReport. Added a comment to clarify that.

Done.

+
+    base::FilePath root =
+        ash::Shell::GetInstance()->delegate()->
+            GetCurrentBrowserContext()->GetPath();
+    base::FilePath filepath = root.Append(attached_filename_.substr(1));
+    attached_filename_ = filepath.BaseName().value();
+
+    // Read the file into file_data, then call send report again with the
+    // stripped filename and file data (which will skip this code path).
+    content::BrowserThread::PostTaskAndReply(
+        content::BrowserThread::FILE, FROM_HERE,
+        base::Bind(&FeedbackData::ReadAttachedFile, this, filepath),
+        base::Bind(&FeedbackData::ReadFileComplete, this));
+  } else {
+    read_attached_file_complete_ = true;
+  }
 #endif
-
-  FeedbackUtil::SendReport(*this);
-
-  // Either the report is sent, and and this object may delete itself, or the
-  // report is pending the attached file read, and another FeedbackData has
-  // been created to hold this data - hence delete this object.
-  delete this;
+  feedback_page_data_complete_ = true;
+  SendReport();
 }
 
 #if defined(OS_CHROMEOS)
-// SyslogsComplete may be called before UpdateData, in which case, we do not
-// want to delete the logs that were gathered, and we do not want to send the
-// report either. Instead simply populate |sys_info_| and |zip_content_|.
-void FeedbackData::SyslogsComplete(chromeos::system::LogDictionaryType* logs,
-                                   std::string* zip_content) {
+void FeedbackData::SyslogsComplete(
+    scoped_ptr<chromeos::SystemLogsResponse> sys_info) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (sent_report_) {
-    // We already sent the report, just delete the data.
-    if (logs)
-      delete logs;
-    if (zip_content)
-      delete zip_content;
-  } else {
 
-    // TODO(rkc): Move to the correct place once crbug.com/138582 is done.
-    AddSyncLogs(logs);
-    AddExtensionInfoLogs(logs);
+  if (send_sys_info_) {
+    sys_info_ = sys_info.Pass();
+    syslogs_collection_complete_ = true;
+    SendReport();
+  }
+}
+
+void FeedbackData::ReadFileComplete() {
+  read_attached_file_complete_ = true;
+  SendReport();
+}
 
-    // Will get deleted when SendReport() is called.
-    zip_content_ = zip_content;
-    sys_info_ = logs;
+void FeedbackData::StartSyslogsCollection() {
+  chromeos::SystemLogsFetcher* fetcher = new chromeos::SystemLogsFetcher();
+  fetcher->Fetch(base::Bind(&FeedbackData::SyslogsComplete, this));
+}
 
-    if (send_sys_info_) {
-      // We already prepared the report, send it now.
-      this->SendReport();
-    }
-  }
+// private
+void FeedbackData::ReadAttachedFile(const base::FilePath& from) {
+  if (!file_util::ReadFileToString(from, attached_filedata_))

[xiyuan, 2013/03/19 21:15:09]

nit: add enclosing {} for the nested if.

[rkc, 2013/03/19 21:20:47]

Done.

+    if (attached_filedata_)
+      attached_filedata_->clear();
 }
 #endif