Revert 88545 - Revert 88470 (broke ServiceProcessStateTest.ForceShutdown on CrOS) - Fix for syste...

Revert 88545 - Revert 88470 (broke ServiceProcessStateTest.ForceShutdown on CrOS) - Fix for system version unit tests in cros.
Unit test the revert was to fix is still failing after the revert. Also confirmed that the failing test has nothing to do with this CL.

BUG=chromium-os:15789
TEST=Ran try servers to make sure the unit test is not failing anymore, plus viewed all the screens and took screenshots. Screen shots of all effected screens are attached to the cros bug
TBR=rkc@chromium.org

TBR=rkc@chromium.org
Review URL: http://codereview.chromium.org/7044086

TBR=thakis@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88595

[Nico, 2011-06-09 21:37:58]

lgtm

On Thu, Jun 9, 2011 at 2:31 PM,  <rkc@chromium.org> wrote:
> Reviewers: Nico,
>
> Description:
> Revert 88545 - Revert 88470 (broke ServiceProcessStateTest.ForceShutdown on
> CrOS) - Fix for system version unit tests in cros.
> Unit test the revert was to fix is still failing after the revert. Also
> confirmed that the failing test has nothing to do with this CL.
>
> BUG=chromium-os:15789
> TEST=Ran try servers to make sure the unit test is not failing anymore, plus
> viewed all the screens and took screenshots. Screen shots of all effected
> screens are attached to the cros bug
> TBR=rkc@chromium.org
>
> TBR=rkc@chromium.org
> Review URL: http://codereview.chromium.org/7044086
>
> TBR=thakis@chromium.org
>
> Please review this at http://codereview.chromium.org/7044097/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>
> Affected files:
>  M     base/sys_info_chromeos.cc
>  M     base/sys_info_unittest.cc
>  M     chrome/browser/browser_about_handler.cc
>  M     chrome/browser/chromeos/login/background_view.cc
>  M     chrome/browser/chromeos/version_loader.h
>  M     chrome/browser/chromeos/version_loader.cc
>  M     chrome/browser/resources/options/about_page.html
>  M     chrome/browser/ui/webui/options/about_page_handler.cc
>
>
> Index: base/sys_info_chromeos.cc
> ===================================================================
> --- base/sys_info_chromeos.cc   (revision 88592)
> +++ base/sys_info_chromeos.cc   (working copy)
> @@ -94,17 +94,20 @@
>   size_t length = lsb_release.find_first_of('\n', start_index) -
> start_index;
>   std::string version = lsb_release.substr(start_index, length);
>   StringTokenizer tokenizer(version, ".");
> -  for (int i = 0; i < 3 && tokenizer.GetNext(); i++) {
> -    if (0 == i) {
> +  // TODO(rkc): Ignore the 0. here; fix this once we move Chrome OS version
> +  // numbers from the 0.xx.yyy.zz format to the xx.yyy.zz format.
> +  // Refer to http://code.google.com/p/chromium-os/issues/detail?id=15789
> +  for (int i = 0; i < 4 && tokenizer.GetNext(); i++) {
> +    if (1 == i) {
>       StringToInt(tokenizer.token_begin(),
>                   tokenizer.token_end(),
>                   major_version);
>       *minor_version = *bugfix_version = 0;
> -    } else if (1 == i) {
> +    } else if (2 == i) {
>       StringToInt(tokenizer.token_begin(),
>                   tokenizer.token_end(),
>                   minor_version);
> -    } else {  // 2 == i
> +    } else {  // 3 == i
>       StringToInt(tokenizer.token_begin(),
>                   tokenizer.token_end(),
>                   bugfix_version);
> Index: base/sys_info_unittest.cc
> ===================================================================
> --- base/sys_info_unittest.cc   (revision 88592)
> +++ base/sys_info_unittest.cc   (working copy)
> @@ -1,4 +1,4 @@
> -// Copyright (c) 2010 The Chromium Authors. All rights reserved.
> +// 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.
>
> @@ -67,9 +67,9 @@
>                                  &os_major_version,
>                                  &os_minor_version,
>                                  &os_bugfix_version);
> -  EXPECT_EQ(1, os_major_version);
> -  EXPECT_EQ(2, os_minor_version);
> -  EXPECT_EQ(3, os_bugfix_version);
> +  EXPECT_EQ(2, os_major_version);
> +  EXPECT_EQ(3, os_minor_version);
> +  EXPECT_EQ(4, os_bugfix_version);
>  }
>
>  TEST_F(SysInfoTest, GoogleChromeOSVersionNumbersFirst) {
> @@ -83,9 +83,9 @@
>                                  &os_major_version,
>                                  &os_minor_version,
>                                  &os_bugfix_version);
> -  EXPECT_EQ(1, os_major_version);
> -  EXPECT_EQ(2, os_minor_version);
> -  EXPECT_EQ(3, os_bugfix_version);
> +  EXPECT_EQ(2, os_major_version);
> +  EXPECT_EQ(3, os_minor_version);
> +  EXPECT_EQ(4, os_bugfix_version);
>  }
>
>  TEST_F(SysInfoTest, GoogleChromeOSNoVersionNumbers) {
> Index: chrome/browser/browser_about_handler.cc
> ===================================================================
> --- chrome/browser/browser_about_handler.cc     (revision 88592)
> +++ chrome/browser/browser_about_handler.cc     (working copy)
> @@ -1239,6 +1239,7 @@
>                                                          int request_id)
>     : source_(source),
>       request_id_(request_id) {
> +  loader_.EnablePlatformVersions(true);
>   loader_.GetVersion(&consumer_,
>                      NewCallback(this,
> &ChromeOSAboutVersionHandler::OnVersion),
>                      chromeos::VersionLoader::VERSION_FULL);
> Index: chrome/browser/chromeos/login/background_view.cc
> ===================================================================
> --- chrome/browser/chromeos/login/background_view.cc    (revision 88592)
> +++ chrome/browser/chromeos/login/background_view.cc    (working copy)
> @@ -28,6 +28,7 @@
>  #include "chrome/browser/profiles/profile_manager.h"
>  #include "chrome/browser/ui/views/dom_view.h"
>  #include "chrome/browser/ui/views/window.h"
> +#include "chrome/common/chrome_version_info.h"
>  #include "googleurl/src/gurl.h"
>  #include "grit/chromium_strings.h"
>  #include "grit/generated_resources.h"
> @@ -47,6 +48,7 @@
>  namespace {
>
>  const SkColor kVersionColor = 0xff5c739f;
> +const char kPlatformLabel[] = "cros:";
>
>  // Returns the corresponding step id for step constant.
>  int GetStepId(size_t step) {
> @@ -339,10 +341,11 @@
>   }
>
>   if (CrosLibrary::Get()->EnsureLoaded()) {
> +    version_loader_.EnablePlatformVersions(true);
>     version_loader_.GetVersion(
>         &version_consumer_,
>         NewCallback(this, &BackgroundView::OnVersion),
> -        is_official_build_?
> +        is_official_build_ ?
>             VersionLoader::VERSION_SHORT_WITH_DATE :
>             VersionLoader::VERSION_FULL);
>     if (!is_official_build_) {
> @@ -405,12 +408,18 @@
>   if (version_text_.empty())
>     return;
>
> -  // TODO(jungshik): Is string concatenation OK here?
> -  std::string label_text = l10n_util::GetStringUTF8(IDS_PRODUCT_OS_NAME);
> +  chrome::VersionInfo version_info;
> +  std::string label_text = l10n_util::GetStringUTF8(IDS_PRODUCT_NAME);
>   label_text += ' ';
> -  label_text += l10n_util::GetStringUTF8(IDS_VERSION_FIELD_PREFIX);
> +  label_text += version_info.Version();
> +  label_text += " (";
> +  // TODO(rkc): Fix this. This needs to be in a resource file, but we have
> had
> +  // to put it in for merge into R12. Also, look at rtl implications for
> this
> +  // entire string composition code.
> +  label_text += kPlatformLabel;
>   label_text += ' ';
>   label_text += version_text_;
> +  label_text += ')';
>
>   if (!enterprise_domain_text_.empty()) {
>     label_text += ' ';
> Index: chrome/browser/chromeos/version_loader.cc
> ===================================================================
> --- chrome/browser/chromeos/version_loader.cc   (revision 88592)
> +++ chrome/browser/chromeos/version_loader.cc   (working copy)
> @@ -1,4 +1,4 @@
> -// Copyright (c) 2010 The Chromium Authors. All rights reserved.
> +// 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.
>
> @@ -21,6 +21,10 @@
>  // File to look for version number in.
>  static const char kPathVersion[] = "/etc/lsb-release";
>
> +// TODO(rkc): Remove once we change over the Chrome OS version format.
> +// Done for http://code.google.com/p/chromium-os/issues/detail?id=15789
> +static const size_t kTrimVersion = 2;
> +
>  // File to look for firmware number in.
>  static const char kPathFirmware[] = "/var/log/bios_info.txt";
>
> @@ -78,6 +82,10 @@
>   return request->handle();
>  }
>
> +void VersionLoader::EnablePlatformVersions(bool enable) {
> +  backend_.get()->set_parse_as_platform(enable);
> +}
> +
>  // static
>  std::string VersionLoader::ParseVersion(const std::string& contents,
>                                         const std::string& prefix) {
> @@ -139,6 +147,20 @@
>     version = ParseVersion(
>         contents,
>         (format == VERSION_FULL) ? kFullVersionPrefix : kVersionPrefix);
> +
> +    // TODO(rkc): Fix this once we move to xx.yyy version numbers for
> Chrome OS
> +    // instead of 0.xx.yyy
> +    // Done for http://code.google.com/p/chromium-os/issues/detail?id=15789
> +    if (parse_as_platform_) {
> +      if (version.size() > kTrimVersion) {
> +        version = version.substr(kTrimVersion);
> +        // Strip the major version.
> +        size_t first_dot = version.find(".");
> +        if (first_dot != std::string::npos) {
> +          version = version.substr(first_dot + 1);
> +        }
> +      }
> +    }
>   }
>
>   if (format == VERSION_SHORT_WITH_DATE) {
> Index: chrome/browser/chromeos/version_loader.h
> ===================================================================
> --- chrome/browser/chromeos/version_loader.h    (revision 88592)
> +++ chrome/browser/chromeos/version_loader.h    (working copy)
> @@ -59,6 +59,12 @@
>   Handle GetFirmware(CancelableRequestConsumerBase* consumer,
>                      GetFirmwareCallback* callback);
>
> +  // Parse the version information as a Chrome platfrom, not Chrome OS
> +  // TODO(rkc): Change this and everywhere it is used once we switch Chrome
> OS
> +  // over to xx.yyy.zz version numbers instead of 0.xx.yyy.zz
> +  // Refer to http://code.google.com/p/chromium-os/issues/detail?id=15789
> +  void EnablePlatformVersions(bool enable);
> +
>   static const char kFullVersionPrefix[];
>   static const char kVersionPrefix[];
>   static const char kFirmwarePrefix[];
> @@ -72,7 +78,7 @@
>   // and extract the version.
>   class Backend : public base::RefCountedThreadSafe<Backend> {
>    public:
> -    Backend() {}
> +    Backend() : parse_as_platform_(false) {}
>
>     // Calls ParseVersion to get the version # and notifies request.
>     // This is invoked on the file thread.
> @@ -84,9 +90,13 @@
>     // This is invoked on the file thread.
>     void GetFirmware(scoped_refptr<GetFirmwareRequest> request);
>
> +    void set_parse_as_platform(bool value) { parse_as_platform_ = value; }
> +
>    private:
>     friend class base::RefCountedThreadSafe<Backend>;
>
> +    bool parse_as_platform_;
> +
>     ~Backend() {}
>
>     DISALLOW_COPY_AND_ASSIGN(Backend);
> Index: chrome/browser/resources/options/about_page.html
> ===================================================================
> --- chrome/browser/resources/options/about_page.html    (revision 88592)
> +++ chrome/browser/resources/options/about_page.html    (working copy)
> @@ -4,14 +4,14 @@
>     <section>
>       <div>
>         <!-- White space is significant between spans. -->
> -        <div><span i18n-content="firmware"></span> <span id="osFirmware0">
> -          <span class="loading" i18n-content="loading"></span></span></div>
> -        <div><span i18n-content="os"></span> <span id="osVersion0">
> -          <span class="loading" i18n-content="loading"></span></span></div>
>         <div>
>           <span i18n-content="browser"></span>
>           <span i18n-content="browser_version"></span>
>         </div>
> +        <div><span i18n-content="os"></span> <span id="osVersion0">
> +          <span class="loading" i18n-content="loading"></span></span></div>
> +        <div><span i18n-content="firmware"></span> <span id="osFirmware0">
> +          <span class="loading" i18n-content="loading"></span></span></div>
>         <div>
>           <button id="moreInfoButton" class="link-button"
>               i18n-content="more_info"></button>
> @@ -35,9 +35,8 @@
>       </div>
>     </section>
>     <section>
> -      <h3 i18n-content="firmware"></h3>
> -      <div id="osFirmware1">
> -        <span class="loading" i18n-content="loading"></span>
> +      <h3 i18n-content="browser"></h3>
> +      <div i18n-content="browser_version"></div>
>     </section>
>     <section>
>       <h3 i18n-content="os"></h3>
> @@ -46,8 +45,9 @@
>       </div>
>     </section>
>     <section>
> -      <h3 i18n-content="browser"></h3>
> -      <div i18n-content="browser_version"></div>
> +      <h3 i18n-content="firmware"></h3>
> +      <div id="osFirmware1">
> +        <span class="loading" i18n-content="loading"></span>
>     </section>
>     <section>
>       <h3>WebKit</h3>
> Index: chrome/browser/ui/webui/options/about_page_handler.cc
> ===================================================================
> --- chrome/browser/ui/webui/options/about_page_handler.cc       (revision
> 88592)
> +++ chrome/browser/ui/webui/options/about_page_handler.cc       (working
> copy)
> @@ -271,6 +271,7 @@
>  void AboutPageHandler::PageReady(const ListValue* args) {
>  #if defined(OS_CHROMEOS)
>   // Version information is loaded from a callback
> +  loader_.EnablePlatformVersions(true);
>   loader_.GetVersion(&consumer_,
>                      NewCallback(this, &AboutPageHandler::OnOSVersion),
>                      chromeos::VersionLoader::VERSION_FULL);
>
>
>