Chrome version info getter for Chrome OS.

Chrome version info getter for Chrome OS.

Properly handles getting channel information for Chrome OS.
Unlike linux desktop version, it has to check the information stored in
/etc/lsb-release (keyed as "CHROMEOS_RELEASE_TRACK").

Asking the track info from udpate_engine_client via dbus would not work
here because it contains *next* channel if the user manually changed
the channel (e.g. stable -> dev, update_engine_client returns "dev-channel").

This fixes chromium-os:22883, but some early GetChannel() users still
misses correct channel information.

BUG=chromium:107333, chromium-os:22883
TEST=chrome://sync and see if sync server points to https://clients4.google.com/chrome-sync/dev on dev channel.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114388

[kochi, 2011-12-12 06:18:34]

Hi Haraken-san,

I would like to review chrome version getter for Chrome OS,
as the author of ones for other platforms.

Darin,

Could you check if this is the correct way to obtain the
channel information of Chrome OS?

Tim,

Could you review or route to an appropriate reviewer?

For testing, I manually edited /etc/lsb-release "CHROMEOS_RELEASE_TRACK" line to
stable-channel,
beta-channel, dev-channel, canary-channel
(which does not exist for Chrome OS yet, though), and
developer-build, and checked about:sync shows 
https://clients4.google.com/chrome-sync for stable and beta
channels and https://clients4.google.com/chrome-sync/dev for
others.

[kochi, 2011-12-12 06:27:03]

I got a build error of dependency check
(including a chromeos header).

I'll fix this and please wait for an update.

[haraken, 2011-12-12 06:30:48]

http://codereview.chromium.org/8883043/diff/8001/chrome/common/chrome_version...
File chrome/common/chrome_version_info_chromeos.cc (right):

http://codereview.chromium.org/8883043/diff/8001/chrome/common/chrome_version...
chrome/common/chrome_version_info_chromeos.cc:14: std::string
VersionInfo::GetVersionStringModifier() {
In order to make it consistent with other platforms (win/linux/mac), you might
need to surround the following code by GOOGLE_CHROME_BUILD, like this:

#if defined(GOOGLE_CHROME_BUILD)
    ...
#else
    return std::string();
#endif

http://codereview.chromium.org/8883043/diff/8001/chrome/common/chrome_version...
chrome/common/chrome_version_info_chromeos.cc:32: VersionInfo::Channel
VersionInfo::GetChannel() {
Ditto.

[petkov, 2011-12-12 11:00:06]

> Darin,
> 
> Could you check if this is the correct way to obtain the
> channel information of Chrome OS?

Yes, this should correctly obtain the channel of the currently running OS
version.

[kochi, 2011-12-13 09:32:49]

Thanks Darin,

Haraken-san, Tim, could you take another look?

I admit that this is not the ideal solution for all the
users of GetChannel(), as some early callers still miss
the correct version.  But modifying the bootstrap path
is very fragile and I'd like to separate the task from
this.

I filed another bug crbug.com/107333 to track the remaining
issue.

http://codereview.chromium.org/8883043/diff/8001/chrome/common/chrome_version...
File chrome/common/chrome_version_info_chromeos.cc (right):

http://codereview.chromium.org/8883043/diff/8001/chrome/common/chrome_version...
chrome/common/chrome_version_info_chromeos.cc:14: std::string
VersionInfo::GetVersionStringModifier() {
On 2011/12/12 06:30:48, haraken1 wrote:
> In order to make it consistent with other platforms (win/linux/mac), you might
> need to surround the following code by GOOGLE_CHROME_BUILD, like this:
> 
> #if defined(GOOGLE_CHROME_BUILD)
>     ...
> #else
>     return std::string();
> #endif

Done.

http://codereview.chromium.org/8883043/diff/8001/chrome/common/chrome_version...
chrome/common/chrome_version_info_chromeos.cc:32: VersionInfo::Channel
VersionInfo::GetChannel() {
On 2011/12/12 06:30:48, haraken1 wrote:
> Ditto.

Done.

[haraken, 2011-12-13 09:32:57]

http://codereview.chromium.org/8883043/diff/6005/chrome/common/chrome_version...
File chrome/common/chrome_version_info_chromeos.cc (right):

http://codereview.chromium.org/8883043/diff/6005/chrome/common/chrome_version...
chrome/common/chrome_version_info_chromeos.cc:27: return "unknown";
I guess that this might be 'return std::string()'. Other platforms are returning
'std::string()' when GOOGLE_CHROME_BUILD is not defined.

[kochi, 2011-12-13 10:22:00]

http://codereview.chromium.org/8883043/diff/6005/chrome/common/chrome_version...
File chrome/common/chrome_version_info_chromeos.cc (right):

http://codereview.chromium.org/8883043/diff/6005/chrome/common/chrome_version...
chrome/common/chrome_version_info_chromeos.cc:27: return "unknown";
On 2011/12/13 09:32:57, haraken1 wrote:
> I guess that this might be 'return std::string()'. Other platforms are
returning
> 'std::string()' when GOOGLE_CHROME_BUILD is not defined.

Done.

[haraken, 2011-12-13 10:25:35]

LGTM (though I am not an OWNER)

[commit-bot: I haz the power, 2011-12-14 03:42:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kochi@chromium.org/8883043/4013

[kochi, 2011-12-14 03:43:06]

On 2011/12/13 10:25:35, haraken1 wrote:
> LGTM (though I am not an OWNER)

It seems chrome and chrome/common have no OWNERS file.

[commit-bot: I haz the power, 2011-12-14 06:01:49]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is win_rel, revision is 114351, job name
was 8883043-4013 (retry) (previous was lost) (previous was lost) (previous was
lost).

[tim (not reviewing), 2011-12-14 08:02:26]

On 2011/12/14 06:01:49, I haz the power (commit-bot) wrote:
> The commit queue went berserk retrying too often for a
> seemingly flaky test. Builder is win_rel, revision is 114351, job name
> was 8883043-4013 (retry) (previous was lost) (previous was lost) (previous was
> lost).

Could someone comment on how this was handled previously?  I'm trying to
understand how this regressed so dramatically...

[M-A Ruel, 2011-12-14 15:59:46]

On 2011/12/14 08:02:26, timsteele wrote:
> On 2011/12/14 06:01:49, I haz the power (commit-bot) wrote:
> > The commit queue went berserk retrying too often for a
> > seemingly flaky test. Builder is win_rel, revision is 114351, job name
> > was 8883043-4013 (retry) (previous was lost) (previous was lost) (previous
was
> > lost).
> 
> Could someone comment on how this was handled previously?  I'm trying to
> understand how this regressed so dramatically...

It's related to the try server upgraded that occurred last night, I forgot to
upgrade the CQ accordingly. Sorry for the annoyance.

[tim (not reviewing), 2011-12-14 16:44:04]

On 2011/12/14 15:59:46, Marc-Antoine Ruel wrote:
> On 2011/12/14 08:02:26, timsteele wrote:
> > On 2011/12/14 06:01:49, I haz the power (commit-bot) wrote:
> > > The commit queue went berserk retrying too often for a
> > > seemingly flaky test. Builder is win_rel, revision is 114351, job name
> > > was 8883043-4013 (retry) (previous was lost) (previous was lost) (previous
> was
> > > lost).
> > 
> > Could someone comment on how this was handled previously?  I'm trying to
> > understand how this regressed so dramatically...
> 
> It's related to the try server upgraded that occurred last night, I forgot to
> upgrade the CQ accordingly. Sorry for the annoyance.

Just so I'm clear my question was referring to the original bug, not the CQ
error :)

[kochi, 2011-12-15 05:20:29]

On 2011/12/14 16:44:04, timsteele wrote:
> On 2011/12/14 15:59:46, Marc-Antoine Ruel wrote:
> > On 2011/12/14 08:02:26, timsteele wrote:
> > > On 2011/12/14 06:01:49, I haz the power (commit-bot) wrote:
> > > > The commit queue went berserk retrying too often for a
> > > > seemingly flaky test. Builder is win_rel, revision is 114351, job name
> > > > was 8883043-4013 (retry) (previous was lost) (previous was lost)
(previous
> > was
> > > > lost).
> > > 
> > > Could someone comment on how this was handled previously?  I'm trying to
> > > understand how this regressed so dramatically...
> > 
> > It's related to the try server upgraded that occurred last night, I forgot
to
> > upgrade the CQ accordingly. Sorry for the annoyance.
> 
> Just so I'm clear my question was referring to the original bug, not the CQ
> error :)

Added a comment on the original issue.
If it's not answering your question, let's continue on the issue 22883.