[Sync] Add version info to about:sync

[Sync] Add version info to about:sync

Version is in format <Browser Name> <Version> (<OS>) <Channel/Modifier info>

BUG=97158
TEST=verify version info is in about:sync


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110456

[Nicolas Zea, 2011-11-17 01:20:52]

PTAL.

[akalin, 2011-11-17 02:35:02]

http://codereview.chromium.org/8587017/diff/1/chrome/browser/sync/sync_ui_uti...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/8587017/diff/1/chrome/browser/sync/sync_ui_uti...
chrome/browser/sync/sync_ui_util.cc:496: strings->SetString("version",
chrome_version.Name() + " " +
Does this have more information than the user agent?  (See
content::GetUserAgent() from content/public/common/content_client.h, and its use
in sync_backend_hsot.cc).

I would prefer the user agent, as it can be more easily compared with server
logs, etc.

WDYT?

[Nicolas Zea, 2011-11-17 03:59:16]

PTAL

http://codereview.chromium.org/8587017/diff/1/chrome/browser/sync/sync_ui_uti...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/8587017/diff/1/chrome/browser/sync/sync_ui_uti...
chrome/browser/sync/sync_ui_util.cc:496: strings->SetString("version",
chrome_version.Name() + " " +
On 2011/11/17 02:35:02, akalin wrote:
> Does this have more information than the user agent?  (See
> content::GetUserAgent() from content/public/common/content_client.h, and its
use
> in sync_backend_hsot.cc).
> 
> I would prefer the user agent, as it can be more easily compared with server
> logs, etc.
> 
> WDYT?

Hmm, wasn't aware of that. Interestingly, sync_backend_host.cc uses both the
content_client's GetUserAgent, as well as a special user agent for syncapi that
also makes use of VersionInfo (see MakeUserAgentForSyncApi). The string here is
almost the same as the syncapi user agent, with the exception that I also add
the channel, which I think is worth keeping around. I've gone ahead and matched
their formats though.

The content_client user agent specifies a lot more than we care to display
(webkit version, mozilla/safari things, etc.), and does not include the channel.

In addition, the server logs are keyed primarily off the cached_guid, which is
in neither of the above, but would be useful to provide. Added as a separate
field.

[akalin, 2011-11-17 04:12:42]

Okay, matching the syncapi version string makes more sense.

http://codereview.chromium.org/8587017/diff/4001/chrome/browser/resources/syn...
File chrome/browser/resources/sync_internals/about.html (right):

http://codereview.chromium.org/8587017/diff/4001/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/about.html:47: <td jscontent="version"/>
Don't you need a block for the unique ID too?

http://codereview.chromium.org/8587017/diff/4001/chrome/browser/sync/sync_ui_...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/8587017/diff/4001/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util.cc:497: chrome::VersionInfo chrome_version;
Maybe a special string if VersionInfo is invalid

http://codereview.chromium.org/8587017/diff/4001/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util.cc:498: std::string version_modifier =
Can you decomp the string-building logic into a separate function (in this file)
and add a TODO to look into unifying it with MakeUserAgentForSyncApi?

http://codereview.chromium.org/8587017/diff/4001/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util.cc:499: " " +
chrome::VersionInfo::GetVersionStringModifier();
Hmm docs for GetVersionStringModifier() say that it returns an empty string for
stable builds also.  May have to use GetChannel() to disambiguiate?

http://codereview.chromium.org/8587017/diff/4001/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util.cc:522: full_status.unique_id == "" ? "none" :
full_status.unique_id);
unique_id.empty()

[Nicolas Zea, 2011-11-17 04:58:10]

All comments addressed, PTAL.

http://codereview.chromium.org/8587017/diff/4001/chrome/browser/resources/syn...
File chrome/browser/resources/sync_internals/about.html (right):

http://codereview.chromium.org/8587017/diff/4001/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/about.html:47: <td jscontent="version"/>
On 2011/11/17 04:12:43, akalin wrote:
> Don't you need a block for the unique ID too?

I added it to the details, so no block is needed.

http://codereview.chromium.org/8587017/diff/4001/chrome/browser/sync/sync_ui_...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/8587017/diff/4001/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util.cc:497: chrome::VersionInfo chrome_version;
On 2011/11/17 04:12:43, akalin wrote:
> Maybe a special string if VersionInfo is invalid

Done.

http://codereview.chromium.org/8587017/diff/4001/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util.cc:498: std::string version_modifier =
On 2011/11/17 04:12:43, akalin wrote:
> Can you decomp the string-building logic into a separate function (in this
file)
> and add a TODO to look into unifying it with MakeUserAgentForSyncApi?

Done.

http://codereview.chromium.org/8587017/diff/4001/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util.cc:499: " " +
chrome::VersionInfo::GetVersionStringModifier();
On 2011/11/17 04:12:43, akalin wrote:
> Hmm docs for GetVersionStringModifier() say that it returns an empty string
for
> stable builds also.  May have to use GetChannel() to disambiguiate?

Good catch. Done.

http://codereview.chromium.org/8587017/diff/4001/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util.cc:522: full_status.unique_id == "" ? "none" :
full_status.unique_id);
On 2011/11/17 04:12:43, akalin wrote:
> unique_id.empty()

Done.

[akalin, 2011-11-17 05:02:59]

LGTM

[commit-bot: I haz the power, 2011-11-17 05:08:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/8587017/6001

[commit-bot: I haz the power, 2011-11-17 06:15:00]

Change committed as 110456