Refactor the about:version code out of about_ui.

chrome/browser/ui/webui/version_handler.cc

Index: chrome/browser/ui/webui/version_handler.cc
===================================================================
--- chrome/browser/ui/webui/version_handler.cc	(revision 0)
+++ chrome/browser/ui/webui/version_handler.cc	(revision 0)
@@ -0,0 +1,160 @@
+// 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/version_handler.h"
+
+#include "base/command_line.h"
+#include "base/file_util.h"
+#include "base/metrics/field_trial.h"
+#include "base/string_split.h"
+#include "base/utf_string_conversions.h"
+#include "chrome/browser/plugin_prefs.h"
+#include "chrome/browser/profiles/profile.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/plugin_service.h"
+#include "content/public/browser/web_ui.h"
+#include "googleurl/src/gurl.h"
+#include "grit/generated_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+
+namespace {
+
+// Retrieves the executable and profile paths on the FILE thread.
+void GetFilePaths(FilePath profile_path,

[Evan Stade, 2012/09/14 15:11:30]

const ref

[SteveT, 2012/09/14 15:28:37]

Unfortunately AbsolutePath below takes a FilePath* which doesn't like it when
profile_path is a const ref.

[Evan Stade, 2012/09/14 16:31:04]

ok. This should still be a const ref. Explicitly create a copy to pass into
AbsolutePath.

[SteveT, 2012/09/14 17:03:15]

Okay sounds good. Copy made.

+                  std::string* exec_path_out,
+                  std::string* profile_path_out) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
+
+  FilePath executable_path = CommandLine::ForCurrentProcess()->GetProgram();
+  if (file_util::AbsolutePath(&executable_path)) {
+    *exec_path_out = UTF16ToASCII(executable_path.LossyDisplayName());

[Evan Stade, 2012/09/14 15:11:30]

you can't just convert to ascii. You'd have to convert to UTF8 if you don't want
things to fall over. But really there's no need for either conversion. Do
everything in string16/UTF16

[SteveT, 2012/09/14 15:28:37]

Everything in string16 as discussed. Thanks for catching that.

+  } else {
+    *exec_path_out =
+        l10n_util::GetStringUTF8(IDS_ABOUT_VERSION_PATH_NOTFOUND);
+  }
+
+  if (!profile_path.empty() && file_util::AbsolutePath(&profile_path)) {
+    *profile_path_out = UTF16ToASCII(profile_path.LossyDisplayName());
+  } else {
+    *profile_path_out =
+        l10n_util::GetStringUTF8(IDS_ABOUT_VERSION_PATH_NOTFOUND);
+  }
+}
+
+}  // namespace
+
+VersionHandler::VersionHandler()
+    : weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
+}
+
+VersionHandler::~VersionHandler() {
+}
+
+void VersionHandler::RegisterMessages() {
+  web_ui()->RegisterMessageCallback(
+      "requestVersionInfo",
+      base::Bind(&VersionHandler::HandleRequestVersionInfo,
+      base::Unretained(this)));
+}
+
+void VersionHandler::HandleRequestVersionInfo(const ListValue* args) {
+  // The Flash version information is needed in the response, so make sure
+  // the plugins are loaded.
+  content::PluginService::GetInstance()->GetPlugins(
+      base::Bind(&VersionHandler::OnGotPlugins,
+          weak_ptr_factory_.GetWeakPtr()));
+
+  FilePath profile_path;
+  Profile* profile = Profile::FromWebUI(web_ui());
+  if (profile) {

[Evan Stade, 2012/09/14 15:11:30]

I don't think it's possible for this to fail

[SteveT, 2012/09/14 15:28:37]

The original code checked for this, but it seems like it was being handled in a
WebUIController (AboutUI). Was it safe there as well?

I've removed the check.

+    profile_path = profile->GetPath();
+  } else {
+    scoped_ptr<StringValue> profile_path(

[Evan Stade, 2012/09/14 15:11:30]

shadowing is confusing

[SteveT, 2012/09/14 15:28:37]

Shadowing? You mean use of scoped_ptr here?

I've seen this pattern elsewhere in combination with CreateStringValue and
passing to CallJSFunction. Do you prefer something else?

[Evan Stade, 2012/09/14 16:31:04]

I meant that this local profile_path shadowed the profile_path defined on L68

[SteveT, 2012/09/14 17:03:15]

Oh damn, I didn't notice that. It's been resolved now since we no longer need
this call due to the profile change above.

+        Value::CreateStringValue(
+            l10n_util::GetStringUTF16(IDS_ABOUT_VERSION_PATH_NOTFOUND)));
+    web_ui()->CallJavascriptFunction("returnProfilePath", *profile_path);
+  }
+
+  // Grab the executable path on the FILE thread. It is returned in
+  // OnGotFilePaths.
+  std::string* exec_path_buffer = new std::string();
+  std::string* profile_path_buffer = new std::string();
+  content::BrowserThread::PostTaskAndReply(
+      content::BrowserThread::FILE, FROM_HERE,
+          base::Bind(&GetFilePaths, profile_path,
+                     base::Unretained(exec_path_buffer),
+                     base::Unretained(profile_path_buffer)),
+          base::Bind(&VersionHandler::OnGotFilePaths,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     base::Owned(exec_path_buffer),
+                     base::Owned(profile_path_buffer)));
+
+  // Respond with the variations info immediately.
+  scoped_ptr<ListValue> variations_list(new ListValue());
+#if !defined(NDEBUG)
+  std::string variation_state;
+  base::FieldTrialList::StatesToString(&variation_state);
+
+  std::vector<std::string> tokens;
+  base::SplitString(variation_state,
+                    base::FieldTrialList::kPersistentStringSeparator,
+                    &tokens);
+  // Since StatesToString appends a separator at the end, SplitString will
+  // append an extra empty string in the vector. Drop it. There should
+  // always be an even number of tokens left.
+  tokens.pop_back();
+  DCHECK_EQ(0U, tokens.size() % 2);
+
+  // |variations| is used to store the formatted strings that will be passed to
+  // the Javascript through |variations_list|.
+  std::vector<std::string> variations;
+  for (size_t i = 0; i < tokens.size(); i += 2)
+    variations.push_back(tokens[i] + ":" + tokens[i + 1]);
+
+  for (std::vector<std::string>::const_iterator it = variations.begin();
+      it != variations.end(); ++it) {
+    variations_list->Append(Value::CreateStringValue(*it));
+  }
+#endif
+  // In release mode, this will return an empty list to clear the section.
+  web_ui()->CallJavascriptFunction("returnVariationInfo",
+                                   *variations_list.release());
+}
+
+void VersionHandler::OnGotFilePaths(std::string* executable_path_data,
+                                    std::string* profile_path_data) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  scoped_ptr<StringValue> exec_path(

[Evan Stade, 2012/09/14 15:11:30]

just base::StringValue(*executable_path_data) is sufficient

[SteveT, 2012/09/14 15:28:37]

I believe I did this earlier but had complaints on some platforms (mac_rel?)
about using the StringValue copy ctor (which didn't make sense to me).

Also, see my comment above about using scoped_ptr this way.

[Evan Stade, 2012/09/14 16:31:04]

well it seems to work in some places:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/webui/op...

[SteveT, 2012/09/14 17:03:15]

You're right that it's a simpler approach. Done everywhere.

+      Value::CreateStringValue(*executable_path_data));
+  web_ui()->CallJavascriptFunction("returnExecutablePath", *exec_path);
+  scoped_ptr<StringValue> profile_path(
+      Value::CreateStringValue(*profile_path_data));
+  web_ui()->CallJavascriptFunction("returnProfilePath", *profile_path);
+}
+
+void VersionHandler::OnGotPlugins(
+    const std::vector<webkit::WebPluginInfo>& plugins) {
+#if !defined(OS_ANDROID)
+  // Obtain the version of the first enabled Flash plugin.
+  std::vector<webkit::WebPluginInfo> info_array;
+  content::PluginService::GetInstance()->GetPluginInfoArray(
+      GURL(), "application/x-shockwave-flash", false, &info_array, NULL);
+  string16 flash_version =
+      l10n_util::GetStringUTF16(IDS_PLUGINS_DISABLED_PLUGIN);
+  PluginPrefs* plugin_prefs =
+      PluginPrefs::GetForProfile(Profile::FromWebUI(web_ui()));
+  if (plugin_prefs) {
+    for (size_t i = 0; i < info_array.size(); ++i) {
+      if (plugin_prefs->IsPluginEnabled(info_array[i])) {
+        flash_version = info_array[i].version;
+        break;
+      }
+    }
+  }
+
+  scoped_ptr<StringValue> arg(Value::CreateStringValue(flash_version));
+  web_ui()->CallJavascriptFunction("returnFlashVersion", *arg);
+#endif
+}
Property changes on: chrome/browser/ui/webui/version_handler.cc
___________________________________________________________________
Added: svn:eol-style
   + LF