Read the ChromeOS channel info earlier during startup.

chrome/browser/chromeos/system/statistics_provider.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/chromeos/system/statistics_provider.h"
 
 #include "base/bind.h"
 #include "base/chromeos/chromeos_version.h"
 #include "base/command_line.h"
 #include "base/file_path.h"
@@ skipping 52 matching lines @@
 
 // Timeout that we should wait for statistics to get loaded
 const int kTimeoutSecs = 3;
 
 }  // namespace
 
 // The StatisticsProvider implementation used in production.
 class StatisticsProviderImpl : public StatisticsProvider {
  public:
   // StatisticsProvider implementation:
+  virtual void Init() OVERRIDE;
   virtual bool GetMachineStatistic(const std::string& name,
                                    std::string* result) OVERRIDE;
 
   static StatisticsProviderImpl* GetInstance();
 
  private:
   friend struct DefaultSingletonTraits<StatisticsProviderImpl>;
 
   StatisticsProviderImpl();
 
+  // Loads the machine info file, which is necessary to get the Chrome channel.

[kochi, 2012/12/13 14:17:30]

Could you also mention in comment why this has to be separated from
StartLoadingMachineStatistics()? Like
"Treat MachineOSInfoFile specially, as
distribution channel information (stable, beta, dev, canary)
is required at earlier stage than everything else.
Rather than posting a delayed task, read and parse the
machine OS info file immediately."

[Alexei Svitkine (slow), 2012/12/13 15:16:03]

Done.

+  void LoadMachineOSInfoFile();
+
   // Starts loading the machine statistcs.
   void StartLoadingMachineStatistics();
 
   // Loads the machine statistcs by examining the system.
   void LoadMachineStatistics();
 
   NameValuePairsParser::NameValueMap machine_info_;
   base::WaitableEvent on_statistics_loaded_;
 
   DISALLOW_COPY_AND_ASSIGN(StatisticsProviderImpl);
 };
 
+void StatisticsProviderImpl::Init() {
+  // Load the machine info file immediately to get the channel info and delay
+  // loading the remaining statistics.
+  LoadMachineOSInfoFile();
+  StartLoadingMachineStatistics();
+}
+
 bool StatisticsProviderImpl::GetMachineStatistic(
     const std::string& name, std::string* result) {
   VLOG(1) << "Statistic is requested for " << name;
   // Block if the statistics are not loaded yet. Per LOG(WARNING) below,
   // the statistics are loaded before requested as of now. For regular
   // sessions (i.e. not OOBE), statistics are first requested when the
   // user is logging in so we have plenty of time to load the data
   // beforehand.
   //
   // If you see the warning appeared for regular sessions, it probably
@@ skipping 19 matching lines @@
     *result = iter->second;
     return true;
   }
   return false;
 }
 
 // manual_reset needs to be true, as we want to keep the signaled state.
 StatisticsProviderImpl::StatisticsProviderImpl()
     : on_statistics_loaded_(true  /* manual_reset */,
                             false /* initially_signaled */) {
-  StartLoadingMachineStatistics();
+}

[kochi, 2012/12/13 14:17:30]

Why do you prefer having another public method Init()
to doing what Init() does in this constructor?

I feel hiding Init() in constructor is safer in that
users cannot explicitly/accidentally call Init() again,
and guarantees that the initialization occurs only once.

[Alexei Svitkine (slow), 2012/12/13 15:16:03]

Here's why I prefer the Init() approach:

1. The style guide recommends using an Init() method, instead of doing work in
the constructor, if the amount work to be done is non-trivial.

2. It makes it explicitly clear when during startup initialization occurs, as
opposed to being done when we're first called, which depends on where this first
gets used (and may not be deterministic or may not be called early enough before
other pieces of code that ask for the channel).

For your concern, I've added a bool to track whether Init() has been called and
some DCHECKs to check for it.

[kochi, 2012/12/14 02:22:52]

Looks good, thanks for explanation!

On 2012/12/13 15:16:03, Alexei Svitkine wrote:

+
+void StatisticsProviderImpl::LoadMachineOSInfoFile() {
+  NameValuePairsParser parser(&machine_info_);
+  if (parser.GetNameValuePairsFromFile(FilePath(kMachineOSInfoFile),
+                                       kMachineOSInfoEq,
+                                       kMachineOSInfoDelim)) {
+#if defined(GOOGLE_CHROME_BUILD)
+    const char kChromeOSReleaseTrack[] = "CHROMEOS_RELEASE_TRACK";
+    NameValuePairsParser::NameValueMap::iterator iter =
+        machine_info_.find(kChromeOSReleaseTrack);
+    if (iter != machine_info_.end())
+      chrome::VersionInfo::SetChannel(iter->second);
+#endif
+  }
 }
 
 void StatisticsProviderImpl::StartLoadingMachineStatistics() {
   VLOG(1) << "Started loading statistics";
   BrowserThread::PostBlockingPoolTask(
       FROM_HERE,
       base::Bind(&StatisticsProviderImpl::LoadMachineStatistics,
                  base::Unretained(this)));
 }
 
@@ skipping 15 matching lines @@
     machine_info_[kHardwareClassKey] = kUnknownHardwareClass;
   else
     machine_info_[kHardwareClassKey] = hardware_class;
 
   parser.GetNameValuePairsFromFile(FilePath(kMachineHardwareInfoFile),
                                    kMachineHardwareInfoEq,
                                    kMachineHardwareInfoDelim);
   parser.GetNameValuePairsFromFile(FilePath(kEchoCouponFile),
                                    kEchoCouponEq,
                                    kEchoCouponDelim);
-  parser.GetNameValuePairsFromFile(FilePath(kMachineOSInfoFile),
-                                   kMachineOSInfoEq,
-                                   kMachineOSInfoDelim);
   parser.GetNameValuePairsFromFile(FilePath(kVpdFile), kVpdEq, kVpdDelim);
 
   // Finished loading the statistics.
   on_statistics_loaded_.Signal();
   VLOG(1) << "Finished loading statistics";
-
-#if defined(GOOGLE_CHROME_BUILD)
-  // TODO(kochi): This is for providing a channel information to
-  // chrome::VersionInfo::GetChannel()/GetVersionStringModifier(),
-  // but this is still late for some early customers such as
-  // prerender::ConfigurePrefetchAndPrerender() and
-  // ThreadWatcherList::ParseCommandLine().
-  // See http://crbug.com/107333 .
-  const char kChromeOSReleaseTrack[] = "CHROMEOS_RELEASE_TRACK";
-  std::string channel;
-  if (GetMachineStatistic(kChromeOSReleaseTrack, &channel)) {
-      chrome::VersionInfo::SetChannel(channel);
-      // Set the product channel for crash reports.  We can't just do this in
-      // ChromeBrowserMainParts::PreCreateThreads like we do for Linux because
-      // the FILE thread hasn't been created yet there so we can't possibly
-      // have read this yet.  Note that this string isn't exactly the same as
-      // 'channel', it's been parsed to be consistent with other platforms
-      // (eg. "canary-channel" becomes "canary", "testimage-channel" becomes
-      // "unknown").
-      child_process_logging::SetChannel(
-          chrome::VersionInfo::GetVersionStringModifier());
-  }
-#endif
 }
 
 StatisticsProviderImpl* StatisticsProviderImpl::GetInstance() {
   return Singleton<StatisticsProviderImpl,
                    DefaultSingletonTraits<StatisticsProviderImpl> >::get();
 }
 
 // The stub StatisticsProvider implementation used on Linux desktop.
 class StatisticsProviderStubImpl : public StatisticsProvider {
  public:
   // StatisticsProvider implementation:
+  virtual void Init() OVERRIDE {
+  }
+
   virtual bool GetMachineStatistic(const std::string& name,
                                    std::string* result) OVERRIDE {
     if (name == "CHROMEOS_RELEASE_BOARD") {
       // Note: syncer::GetSessionNameSynchronously() also uses the mechanism
       // below to determine the CrOs release board. However, it cannot include
       // statistics_provider.h and use this method because of the mutual
       // dependency that creates between sync.gyp:sync and chrome.gyp:browser.
       // TODO(rsimha): Update syncer::GetSessionNameSynchronously() if this code
       // is ever moved into base/. See http://crbug.com/126732.
       const CommandLine* command_line = CommandLine::ForCurrentProcess();
@@ skipping 23 matching lines @@
 StatisticsProvider* StatisticsProvider::GetInstance() {
   if (base::chromeos::IsRunningOnChromeOS()) {
     return StatisticsProviderImpl::GetInstance();
   } else {
     return StatisticsProviderStubImpl::GetInstance();
   }
 }
 
 }  // namespace system
 }  // namespace chromeos