Fix threading bugs in product label UI.

chrome/browser/ui/webui/help/help_handler.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/help/help_handler.h"
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
@@ skipping 29 matching lines @@
 #include "ui/base/l10n/l10n_util.h"
 #include "v8/include/v8.h"
 
 #if defined(OS_MACOSX)
 #include "chrome/browser/mac/obsolete_system.h"
 #endif
 
 #if defined(OS_CHROMEOS)
 #include "base/files/file_util_proxy.h"
 #include "base/i18n/time_formatting.h"
+#include "base/path_service.h"
 #include "base/prefs/pref_service.h"
 #include "base/sys_info.h"
+#include "base/task_runner_util.h"
 #include "chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h"
 #include "chrome/browser/chromeos/ownership/owner_settings_service_chromeos_factory.h"
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/webui/chromeos/image_source.h"
 #include "chrome/browser/ui/webui/help/help_utils_chromeos.h"
 #include "chrome/browser/ui/webui/help/version_updater_chromeos.h"
+#include "chromeos/chromeos_paths.h"
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/power_manager_client.h"
 #include "components/user_manager/user_manager.h"
 #endif
 
 using base::ListValue;
 using content::BrowserThread;
 
 namespace {
@@ skipping 48 matching lines @@
         chromeos::OwnerSettingsServiceChromeOSFactory::GetInstance()
             ->GetForBrowserContext(profile);
     // On non managed machines we have local owner who is the only one to change
     // anything. Ensure that ReleaseChannelDelegated is false.
     if (service && service->IsOwner())
       return !value;
   }
   return false;
 }
 
+// Reads the file containing the FCC label text, if found.

[stevenjb, 2014/12/05 23:28:07]

nit: Comment that this is called from the blocking pool.

[michaelpg, 2014/12/08 00:58:19]

Done.

+std::string ReadFCCLabelText() {
+  base::FilePath shared_assets_dir;
+  std::string contents;
+  if (PathService::Get(chromeos::DIR_SHARED_ASSETS, &shared_assets_dir) &&

[satorux1, 2014/12/08 00:43:57]

Why not just use chrome::kChromeOSAssetPath?

Something like below should work.

const base::FilePath asset_dir(FILE_PATH_LITERAL(chrome::kChromeOSAssetPath));
const base::FilePath label_file_path = asset_dir.AppendASCII(kFCCLabelTextPath);

[michaelpg, 2014/12/08 00:58:19]

I don't know whether it would be better to use chrome/common/url_constants.h or
chromeos/chromeos_paths.cc. It looked like chromeos_paths is more idiomatic
because it relates to path on disk, not URL to get to that path.

But I'll do whatever you think is best.

[satorux1, 2014/12/08 01:12:27]

I'd suggest to keep using chrome::kChromeOSAssetPath. There are already some
other file path constants in url_constants.cc so removing kChromeOSAssetPath
alone doesn't make the code much cleaner. Besides, I think it'd be nicer to keep
this patch small to make it easier to merge.

[michaelpg, 2014/12/08 21:14:57]

Done.

+      base::ReadFileToString(shared_assets_dir.AppendASCII(kFCCLabelTextPath),
+                             &contents)) {
+    return contents;
+  }
+  return std::string();
+}
+
 #endif  // defined(OS_CHROMEOS)
 
 }  // namespace
 
 HelpHandler::HelpHandler()
     : version_updater_(VersionUpdater::Create(nullptr)), weak_factory_(this) {
 }
 
 HelpHandler::~HelpHandler() {
 }
@@ skipping 257 matching lines @@
       "help.HelpPage.updateIsEnterpriseManaged",
       base::FundamentalValue(IsEnterpriseManaged()));
   // First argument to GetChannel() is a flag that indicates whether
   // current channel should be returned (if true) or target channel
   // (otherwise).
   version_updater_->GetChannel(true,
       base::Bind(&HelpHandler::OnCurrentChannel, weak_factory_.GetWeakPtr()));
   version_updater_->GetChannel(false,
       base::Bind(&HelpHandler::OnTargetChannel, weak_factory_.GetWeakPtr()));
 
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&HelpHandler::LoadFCCLabelText, weak_factory_.GetWeakPtr()));
+  base::PostTaskAndReplyWithResult(
+      content::BrowserThread::GetBlockingPool(),
+      FROM_HERE,
+      base::Bind(&ReadFCCLabelText),
+      base::Bind(&HelpHandler::OnFCCLabelTextRead,
+                 weak_factory_.GetWeakPtr()));
 #endif
 }
 
 #if defined(OS_MACOSX)
 void HelpHandler::PromoteUpdater(const base::ListValue* args) {
   version_updater_->PromoteUpdater();
 }
 #endif
 
 void HelpHandler::RelaunchNow(const base::ListValue* args) {
@@ skipping 161 matching lines @@
 void HelpHandler::OnCurrentChannel(const std::string& channel) {
   web_ui()->CallJavascriptFunction(
       "help.HelpPage.updateCurrentChannel", base::StringValue(channel));
 }
 
 void HelpHandler::OnTargetChannel(const std::string& channel) {
   web_ui()->CallJavascriptFunction(
       "help.HelpPage.updateTargetChannel", base::StringValue(channel));
 }
 
-void HelpHandler::LoadFCCLabelText() {
-  base::FilePath path(std::string(chrome::kChromeOSAssetPath) +
-                      kFCCLabelTextPath);
-  std::string contents;
-  if (base::ReadFileToString(path, &contents)) {
-    // Remove unnecessary whitespace.
-    base::StringValue label(base::CollapseWhitespaceASCII(contents, true));
-    web_ui()->CallJavascriptFunction("help.HelpPage.setProductLabelText",
-                                     label);
-  }
+void HelpHandler::OnFCCLabelTextRead(std::string text) {
+  // Remove unnecessary whitespace.
+  text = base::CollapseWhitespaceASCII(text, true);
+  if (text.empty())
+    return;
+  base::StringValue label(text);
+  web_ui()->CallJavascriptFunction("help.HelpPage.setProductLabelText", label);
 }
 
 #endif // defined(OS_CHROMEOS)