Implemented a "Last Updated" field for the about page of Chrome OS.

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 16 matching lines @@
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "grit/google_chrome_strings.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"
 #include "webkit/glue/webkit_glue.h"
 
 #if defined(OS_CHROMEOS)
+#include "base/i18n/time_formatting.h"
+#include "base/file_util_proxy.h"
+#include "base/sys_info.h"
 #include "chrome/browser/chromeos/login/user_manager.h"
 #include "chrome/browser/chromeos/cros_settings.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/prefs/pref_service.h"
+#include "content/public/browser/browser_thread.h"
 #endif
 
 using base::ListValue;
+using content::BrowserThread;
 
 namespace {
 
 // Returns the browser version as a string.
 string16 BuildBrowserVersionString() {
   chrome::VersionInfo version_info;
   DCHECK(version_info.is_valid());
 
   std::string browser_version = version_info.Version();
   std::string version_modifier =
@@ skipping 29 matching lines @@
     std::string user = chromeos::UserManager::Get()->GetLoggedInUser().email();
     size_t at_pos = user.find('@');
     if (at_pos != std::string::npos && at_pos + 1 < user.length())
       domain = user.substr(user.find('@') + 1);
     return domain == g_browser_process->browser_policy_connector()->
         GetEnterpriseDomain();
   }
   return false;
 }
 
+// Pointer to a |StringValue| holding the date of the last update to Chromium
+// OS. Because this value is obtained by reading a file, it is cached here to
+// prevent the need to read from the file system multiple times unnecessarily.
+Value* g_last_updated_string = NULL;
+
 #endif  // defined(OS_CHROMEOS)
 
 }  // namespace
 
 HelpHandler::HelpHandler()
-    : version_updater_(VersionUpdater::Create()) {
+    : version_updater_(VersionUpdater::Create())
+#if defined(OS_CHROMEOS)
+      , ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this))
+#endif // defined(OS_CHROMEOS)
+                                                            {
 }
 
 HelpHandler::~HelpHandler() {
 }
 
 void HelpHandler::GetLocalizedValues(DictionaryValue* localized_strings) {
   DCHECK(localized_strings);
   DCHECK(localized_strings->empty());
 
   struct L10nResources {
@@ skipping 20 matching lines @@
     { "firmware", IDS_ABOUT_PAGE_FIRMWARE },
     { "showMoreInfo", IDS_SHOW_MORE_INFO },
     { "hideMoreInfo", IDS_HIDE_MORE_INFO },
     { "channel", IDS_ABOUT_PAGE_CHANNEL },
     { "stable", IDS_ABOUT_PAGE_CHANNEL_STABLE },
     { "beta", IDS_ABOUT_PAGE_CHANNEL_BETA },
     { "dev", IDS_ABOUT_PAGE_CHANNEL_DEVELOPMENT },
     { "webkit", IDS_WEBKIT },
     { "userAgent", IDS_ABOUT_VERSION_USER_AGENT },
     { "commandLine", IDS_ABOUT_VERSION_COMMAND_LINE },
+    { "lastUpdated", IDS_ABOUT_VERSION_LAST_UPDATED },
 #endif
 #if defined(OS_MACOSX)
     { "promote", IDS_ABOUT_CHROME_PROMOTE_UPDATER },
 #endif
   };
 
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(resources); ++i) {
     localized_strings->SetString(resources[i].name,
                                  l10n_util::GetStringUTF16(resources[i].ids));
   }
@@ skipping 67 matching lines @@
 #endif
           );
       break;
     }
     default:
       NOTREACHED();
   }
 }
 
 void HelpHandler::OnPageLoaded(const ListValue* args) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

[Evan Stade, 2012/04/19 03:40:27]

I disagree this is necessary, the one down below is OK because it's a function
that gets posted from a different thread, but we don't generally put these
guards on functions that are not inter-thread callbacks. But I don't care too
much.

[Mark Mentovai, 2012/04/19 03:44:50]

Evan Stade wrote:
To clarify, I really only thought it was necessary for the DCHECK to be in
ProcessLsbFileInfo tool.

[Kyle Horimoto, 2012/04/19 18:31:29]

Done.

[Kyle Horimoto, 2012/04/19 18:31:29]

Done.

+
 #if defined(OS_CHROMEOS)
   // Version information is loaded from a callback
   loader_.GetVersion(&consumer_, base::Bind(&HelpHandler::OnOSVersion,
                                             base::Unretained(this)),
                      chromeos::VersionLoader::VERSION_FULL);
   loader_.GetFirmware(&consumer_, base::Bind(&HelpHandler::OnOSFirmware,
                                              base::Unretained(this)));
 
   scoped_ptr<base::Value> can_change_channel_value(
       base::Value::CreateBooleanValue(CanChangeReleaseChannel()));
   web_ui()->CallJavascriptFunction(
       "help.HelpPage.updateEnableReleaseChannel", *can_change_channel_value);
+
+  if (g_last_updated_string == NULL) {
+    // If |g_last_updated_string| is |NULL|, the date has not yet been assigned.
+    // Get the date of the last lsb-release file modification. Note that there
+    // is a chance that multiple tabs could request the file information at

[Evan Stade, 2012/04/19 03:40:27]

leaner comments. While this comment is all true, everything after the second
sentence is kind of excessive.

[Kyle Horimoto, 2012/04/19 18:31:29]

Done.

+    // roughly the same time, causing this request to go through multiple times.
+    // While this may cause the file to be read multiple times unnecessarily,
+    // it does not leak resources and is a rare in that it only happens if
+    // multiple "Help" tabs are opened simultaneously.
+    base::FileUtilProxy::GetFileInfo(
+        BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE),
+        base::SysInfo::GetLsbReleaseFilePath(),
+        base::Bind(&HelpHandler::ProcessLsbFileInfo,
+                   weak_factory_.GetWeakPtr()));
+  } else {
+    // If it is not NULL, the date has already been retrieved from the file

[Evan Stade, 2012/04/19 03:40:27]

I would remove this comment; it's already covered by your variable-level comment
above.

[Kyle Horimoto, 2012/04/19 18:31:29]

Done.

+    // system, so just use the cached value.
+    web_ui()->CallJavascriptFunction("help.HelpPage.setLastUpdated",
+                                     *g_last_updated_string);
+  }
 #endif  // defined(OS_CHROMEOS)
 
   version_updater_->CheckForUpdate(
       base::Bind(&HelpHandler::SetUpdateStatus, base::Unretained(this))
 #if defined(OS_MACOSX)
       , base::Bind(&HelpHandler::SetPromotionState, base::Unretained(this))
 #endif
       );
 
 #if defined(OS_CHROMEOS)
@@ skipping 125 matching lines @@
   scoped_ptr<Value> firmware_string(Value::CreateStringValue(firmware));
   web_ui()->CallJavascriptFunction("help.HelpPage.setOSFirmware",
                                    *firmware_string);
 }
 
 void HelpHandler::OnReleaseChannel(const std::string& channel) {
   scoped_ptr<Value> channel_string(Value::CreateStringValue(channel));
   web_ui()->CallJavascriptFunction(
       "help.HelpPage.updateSelectedChannel", *channel_string);
 }
+
+void HelpHandler::ProcessLsbFileInfo(
+    base::PlatformFileError error, const base::PlatformFileInfo& file_info) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  base::Time time;
+  if (error == base::PLATFORM_FILE_OK) {
+    // Retrieves the approximate time at which Chrome OS was last updated. Each
+    // time a new build is created, /etc/lsb-release is modified with the new
+    // version numbers of the release. Thus, this is a close approximation of

[Evan Stade, 2012/04/19 03:40:27]

remove last sentence

[Kyle Horimoto, 2012/04/19 18:31:29]

Done.

+    // the time that the last Chrome OS update occurred.
+    time = file_info.last_modified;
+  } else {
+    // If the time of the last update cannot be retrieved, punt and just set
+    // the last update time to be the current time.

[Evan Stade, 2012/04/19 03:40:27]

instead of telling a lie, it's better to not show the last updated time at all
if we can't retrieve it.

[Kyle Horimoto, 2012/04/19 18:31:29]

I made the "Last Updated" section hidden by default, and only shown if
help.HelpPage.setLastUpdated() is called.

+    time = base::Time::Now();
+  }
+
+  // Note that this string will be internationalized.
+  string16 last_updated = base::TimeFormatFriendlyDate(time);
+
+  // If |g_last_updated_string| is |NULL|, then the file's information has not
+  // already been retrieved by another tab. In this case, allocate the Value

[Evan Stade, 2012/04/19 03:40:27]

invert the first sentence (if string is not null...), and drop second sentence.

[Kyle Horimoto, 2012/04/19 18:31:29]

Done.

+  // here.
+  if (g_last_updated_string == NULL)

[Evan Stade, 2012/04/19 03:40:27]

if g_last_updated_string is not null, you needn't have done any of the work up
to this point in the function. Meaning if g_last_updated_string is non null,
skip straight to the punchline rather than constructing last_updated only to
throw it out.

[Kyle Horimoto, 2012/04/19 18:31:29]

Done.

[Evan Stade, 2012/04/19 19:30:57]

I don't see any changes. What I meant was something like:

if (!g_last_updated_string) {
  base::Time time;
  if (error == base::PLATFORM_FILE_OK) {
  ...
}

web_ui()->CallJavascriptFunction(...);

[Kyle Horimoto, 2012/04/19 20:07:33]

Oops, misunderstood you the first time. Got it now.

+    g_last_updated_string = Value::CreateStringValue(last_updated);
+
+  web_ui()->CallJavascriptFunction("help.HelpPage.setLastUpdated",
+                                   *g_last_updated_string);
+}
 #endif // defined(OS_CHROMEOS)