separate about page into its own page (included in chrome://chrome)

chrome/browser/ui/webui/about_page/about_page_handler.cc

 // Copyright (c) 2011 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/options2/chromeos/about_page_handler2.h"
+#include "chrome/browser/ui/webui/about_page/about_page_handler.h"
 
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/i18n/time_formatting.h"
 #include "base/string16.h"
 #include "base/string_number_conversions.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/dbus/dbus_thread_manager.h"
 #include "chrome/browser/chromeos/dbus/power_manager_client.h"
 #include "chrome/browser/chromeos/dbus/update_engine_client.h"
 #include "chrome/browser/chromeos/login/user_manager.h"
 #include "chrome/browser/chromeos/login/wizard_controller.h"
 #include "chrome/browser/google/google_util.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/url_constants.h"
+#include "content/browser/webui/web_ui.h"
 #include "content/public/common/content_client.h"
 #include "googleurl/src/gurl.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "grit/locale_settings.h"
 #include "grit/theme_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "v8/include/v8.h"
 #include "webkit/glue/user_agent.h"
@@ skipping 15 matching lines @@
 
 // Returns a substring [start, end) from |text|.
 std::string StringSubRange(const std::string& text, size_t start,
                            size_t end) {
   DCHECK(end > start);
   return text.substr(start, end - start);
 }
 
 }  // namespace
 
-namespace chromeos {
-namespace options2 {
+using chromeos::DBusThreadManager;
+using chromeos::UpdateEngineClient;
+using chromeos::UserManager;
+using chromeos::VersionLoader;
+using chromeos::WizardController;
 
 class AboutPageHandler::UpdateObserver
     : public UpdateEngineClient::Observer {
  public:
   explicit UpdateObserver(AboutPageHandler* handler) : page_handler_(handler) {}
   virtual ~UpdateObserver() {}
 
   AboutPageHandler* page_handler() const { return page_handler_; }
 
  private:
   virtual void UpdateStatusChanged(
       const UpdateEngineClient::Status& status) OVERRIDE {
     page_handler_->UpdateStatus(status);
   }
 
   AboutPageHandler* page_handler_;
 
   DISALLOW_COPY_AND_ASSIGN(UpdateObserver);
 };
 
 AboutPageHandler::AboutPageHandler()
     : progress_(-1),
       sticky_(false),
-      started_(false)
-{}
+      started_(false) {
+}
 
 AboutPageHandler::~AboutPageHandler() {
   if (update_observer_.get()) {
     DBusThreadManager::Get()->GetUpdateEngineClient()->
         RemoveObserver(update_observer_.get());
   }
 }
 
 void AboutPageHandler::GetLocalizedValues(DictionaryValue* localized_strings) {
   DCHECK(localized_strings);
 
-  static OptionsStringResource resources[] = {
+  struct L10nResources {
+    const char* name;
+    int ids;
+  };
+
+  static L10nResources resources[] = {
+    { "pageTitle", IDS_ABOUT_TAB_TITLE },

[Dan Beam, 2012/01/09 23:05:51]

Do you think it would be worth alpha sorting these to find one easier?

[Evan Stade, 2012/01/09 23:31:16]

no I prefer to keep the related ones together, e.g. stable near beta and os near
platform. Anyway it's not my call as I don't own this code.

[Dan Beam, 2012/01/09 23:53:14]

If it's in preference order or an order I'm not aware of, that's great, just
wanted that to have a significance (which it seems it does).

     { "firmware", IDS_ABOUT_PAGE_FIRMWARE },
     { "product", IDS_PRODUCT_OS_NAME },
     { "os", IDS_PRODUCT_OS_NAME },
     { "platform", IDS_PLATFORM_LABEL },
     { "loading", IDS_ABOUT_PAGE_LOADING },
     { "check_now", IDS_ABOUT_PAGE_CHECK_NOW },
     { "update_status", IDS_UPGRADE_CHECK_STARTED },
     { "restart_now", IDS_RELAUNCH_AND_UPDATE },
     { "browser", IDS_PRODUCT_NAME },
     { "more_info", IDS_ABOUT_PAGE_MORE_INFO },
     { "copyright", IDS_ABOUT_VERSION_COPYRIGHT },
     { "channel", IDS_ABOUT_PAGE_CHANNEL },
     { "stable", IDS_ABOUT_PAGE_CHANNEL_STABLE },
     { "beta", IDS_ABOUT_PAGE_CHANNEL_BETA },
     { "dev", IDS_ABOUT_PAGE_CHANNEL_DEVELOPMENT },
     { "canary", IDS_ABOUT_PAGE_CHANNEL_CANARY },
     { "channel_warning_header", IDS_ABOUT_PAGE_CHANNEL_WARNING_HEADER },
     { "channel_warning_text", IDS_ABOUT_PAGE_CHANNEL_WARNING_TEXT },
     { "user_agent", IDS_ABOUT_VERSION_USER_AGENT },
     { "command_line", IDS_ABOUT_VERSION_COMMAND_LINE },
   };
 
-  RegisterStrings(localized_strings, resources, arraysize(resources));
-  RegisterTitle(localized_strings, "aboutPage", IDS_ABOUT_TAB_TITLE);
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(resources); ++i) {

[Dan Beam, 2012/01/09 23:05:51]

^ didn't know about this macro, cool!

[Evan Stade, 2012/01/09 23:31:16]

fyi, better to use arraysize() where possible

+    localized_strings->SetString(resources[i].name,
+                                 l10n_util::GetStringUTF16(resources[i].ids));

[Dan Beam, 2012/01/09 23:05:51]

Should we both DCHECK()ing that the key doesn't exist here?  Seems like it'd be
pretty easy to write over an existing key accidentally (which may be a bug or
feature, depending on how you look at it, but I think bug).

[Evan Stade, 2012/01/09 23:31:16]

I don't think so, it seems a little overly cautious. I can't recall ever
encountering a bug where a string was overwritten. This function is the only
place where any strings should be written to this dictionary so it's not like a
code change in another file could affect this list.

[Dan Beam, 2012/01/09 23:53:14]

But what happens when you start to merge dictionaries from various files by
passing in a partly populated DictionaryValue* via |localized_strings| (or
you're saying that Just Won't Happen™)?  Why not just return a new
DictionaryValue* to the caller, clear the DictionaryValue* at start, or DCHECK()
the keys being set (or key size at the beginning of the method).  Any of these
seem like good preventative solutions.

[Evan Stade, 2012/01/10 00:04:50]

that's indeed possible, if we wanted to fold about into another page. However to
actually be safe in that case we'd have to add DCHECKs for every SetString
(below as well) which would be onerous. I think if we're going to fold pages
together the person doing it should namespace the strings, i.e. put "about_" in
front of each one of them.
empty check added

+  }
 
   // browser version
 
   chrome::VersionInfo version_info;
   DCHECK(version_info.is_valid());
 
   std::string browser_version = version_info.Version();
   std::string version_modifier =
       chrome::VersionInfo::GetVersionStringModifier();
   if (!version_modifier.empty())
@@ skipping 274 matching lines @@
   if (DBusThreadManager::Get()->GetUpdateEngineClient()
       ->HasObserver(observer)) {
     // If UpdateEngineClient still has the observer, then the page handler
     // is valid.
     AboutPageHandler* handler = observer->page_handler();
     scoped_ptr<Value> channel_string(Value::CreateStringValue(channel));
     handler->web_ui()->CallJavascriptFunction(
         "AboutPage.updateSelectedOptionCallback", *channel_string);
   }
 }
-
-}  // namespace options2
-}  // namespace chromeos