Read the ChromeOS channel info earlier during startup.

Read the ChromeOS channel info earlier during startup. 

This makes the channel available much earlier, which 
allows users of that info, such as the VariationsService 
to work correctly. 

This CL also refactors StatisticsProvider initialization 
to be started explicitly, instead of as a result of the 
first call to GetInstance(). 

Previously, the initialization ended up happening as a 
result of BrowserPolicyConnector::CompleteInitialization() 
which was triggered by BrowserPolicyConnector::Init() when 
the local state was being initialized. 

BUG=107333
TEST=Manual, by instrumenting code and once this lands, checking that channel-targeted Finch studies get applied on ChromeOS.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173507

[kochi, 2012-12-13 02:17:36]

On 2012/12/12 21:26:40, Alexei Svitkine wrote:

I'll take a look today and review, but a quick compile testing failed:

../../chrome/browser/chromeos/customization_document_unittest.cc: In member
function 'virtual void
chromeos::StartupCustomizationDocumentTest_Basic_Test::TestBody()':
../../chrome/browser/chromeos/customization_document_unittest.cc:82:34: error:
cannot declare variable 'mock_statistics_provider' to be of abstract type
'chromeos::system::MockStatisticsProvider'
../../chrome/browser/chromeos/system/mock_statistics_provider.h:15:7: note:  
because the following virtual functions are pure within
'chromeos::system::MockStatisticsProvider':
../../chrome/browser/chromeos/system/statistics_provider.h:17:16: note:       
virtual void chromeos::system::StatisticsProvider::Init()
../../chrome/browser/chromeos/customization_document_unittest.cc: In member
function 'virtual void
chromeos::StartupCustomizationDocumentTest_VPD_Test::TestBody()':
../../chrome/browser/chromeos/customization_document_unittest.cc:112:34: error:
cannot declare variable 'mock_statistics_provider' to be of abstract type
'chromeos::system::MockStatisticsProvider'
../../chrome/browser/chromeos/system/mock_statistics_provider.h:15:7: note:  
since type 'chromeos::system::MockStatisticsProvider' has pure virtual functions
../../chrome/browser/chromeos/customization_document_unittest.cc: In member
function 'virtual void
chromeos::StartupCustomizationDocumentTest_BadManifest_Test::TestBody()':
../../chrome/browser/chromeos/customization_document_unittest.cc:138:34: error:
cannot declare variable 'mock_statistics_provider' to be of abstract type
'chromeos::system::MockStatisticsProvider'
../../chrome/browser/chromeos/system/mock_statistics_provider.h:15:7: note:  
since type 'chromeos::system::MockStatisticsProvider' has pure virtual functions

If possible, could you address these tests compilation issue
(probably the same as trybots failures) in the meantime.

[kochi, 2012-12-13 14:17:29]

https://chromiumcodereview.appspot.com/11549025/diff/1/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/11549025/diff/1/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:730:
chromeos::system::StatisticsProvider::GetInstance()->Init();
Could you add a comment why the initialization of
StatisticsProvider has to be here?

https://chromiumcodereview.appspot.com/11549025/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/system/statistics_provider.cc (right):

https://chromiumcodereview.appspot.com/11549025/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/system/statistics_provider.cc:84: // Loads the machine
info file, which is necessary to get the Chrome channel.
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."

https://chromiumcodereview.appspot.com/11549025/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/system/statistics_provider.cc:145: }
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.

https://chromiumcodereview.appspot.com/11549025/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/system/statistics_provider.h (right):

https://chromiumcodereview.appspot.com/11549025/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/system/statistics_provider.h:17: virtual void Init() =
0;
If you remove this as commented in .cc file,
you don't get compilation error for MockStatisticsProvider.

[Alexei Svitkine (slow), 2012-12-13 15:16:02]

https://codereview.chromium.org/11549025/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system/statistics_provider.cc (right):

https://codereview.chromium.org/11549025/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/statistics_provider.cc:84: // Loads the machine
info file, which is necessary to get the Chrome channel.
On 2012/12/13 14:17:30, Takayoshi Kochi wrote:
> 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."

Done.

https://codereview.chromium.org/11549025/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/statistics_provider.cc:145: }
On 2012/12/13 14:17:30, Takayoshi Kochi wrote:
> 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.

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.

https://codereview.chromium.org/11549025/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system/statistics_provider.h (right):

https://codereview.chromium.org/11549025/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/statistics_provider.h:17: virtual void Init() =
0;
On 2012/12/13 14:17:30, Takayoshi Kochi wrote:
> If you remove this as commented in .cc file,
> you don't get compilation error for MockStatisticsProvider.

I've modified MockStatisticsProvider now. See my other comment on why I think
this is preferable.

[Alexei Svitkine (slow), 2012-12-13 15:45:49]

https://codereview.chromium.org/11549025/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11549025/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main.cc:730:
chromeos::system::StatisticsProvider::GetInstance()->Init();
On 2012/12/13 14:17:30, Takayoshi Kochi wrote:
> Could you add a comment why the initialization of
> StatisticsProvider has to be here?

Done.

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

LGTM

https://codereview.chromium.org/11549025/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system/statistics_provider.cc (right):

https://codereview.chromium.org/11549025/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/statistics_provider.cc:145: }
Looks good, thanks for explanation!

On 2012/12/13 15:16:03, Alexei Svitkine wrote:
> On 2012/12/13 14:17:30, Takayoshi Kochi wrote:
> > 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.
> 
> 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.

[Alexei Svitkine (slow), 2012-12-14 15:04:57]

+sky for chrome/ OWNERS

[sky, 2012-12-14 17:44:39]

Is it possible to move this to chrome_browser_main_chromeos?

[Alexei Svitkine (slow), 2012-12-14 18:08:21]

On 2012/12/14 17:44:39, sky wrote:
> Is it possible to move this to chrome_browser_main_chromeos?

Done.

This causes the info to be read a bit earlier in startup, but that's okay.

[Alexei Svitkine (slow), 2012-12-14 20:21:36]

s/sky/nkostylev for a more appropriate chromeos owner

[Alexei Svitkine (slow), 2012-12-17 16:29:26]

Nikita: ping for ChromeOS owners approval

[Nikita (slow), 2012-12-17 17:14:47]

lgtm

[commit-bot: I haz the power, 2012-12-17 17:30:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/11549025/16013

[commit-bot: I haz the power, 2012-12-17 19:18:24]

Change committed as 173507