Read the ChromeOS channel info earlier during startup.

chrome/browser/chromeos/system/statistics_provider.cc

Index: chrome/browser/chromeos/system/statistics_provider.cc
===================================================================
--- chrome/browser/chromeos/system/statistics_provider.cc	(revision 172608)
+++ chrome/browser/chromeos/system/statistics_provider.cc	(working copy)
@@ -70,6 +70,7 @@
 class StatisticsProviderImpl : public StatisticsProvider {
  public:
   // StatisticsProvider implementation:
+  virtual void Init() OVERRIDE;
   virtual bool GetMachineStatistic(const std::string& name,
                                    std::string* result) OVERRIDE;
 
@@ -80,6 +81,9 @@
 
   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();
 
@@ -92,6 +96,13 @@
   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;
@@ -131,9 +142,23 @@
 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(
@@ -167,37 +192,11 @@
   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() {
@@ -209,6 +208,9 @@
 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") {