Surface Chrome SVN Revision in dev tools 'version' command

Surface Chrome SVN Revision in dev tools 'version' command

Motivation:
Surface SVN version information to Telemetry, the dev tools protocol [which Telemetry uses] doesn't currently contain the SVN revision.  To get the information to devtools it needs to e accessible via the content layer.

Changes:
* Added ChromeContentClient::GetProductSCMRevision() that returns the SCM revision.
* Minor unrelated cleanup in tweak_info_plist.py

BUG=None

[jeremy, 2013-06-10 09:10:48]

pfeldman: devtools
joi: chrome/content
thakis: tweak_info_plist.py

The obvious way to make this change would be to expand the version info string
returned by ChromeClient::GetProduct(), the problem is that this call is used in
many places to identify Chrome and changing it's format would likely break other
things e.g. it's used as an identifier for Windows Accessibility.

I'm absolutely fine adding the SCM revision to some other version string or
making a more generic change to ChromeClient(), if you'd like me to take a
different approach I'm totally fine with that just please provide a complete
description of the way you'd like it to look.

Many thanks!,
Jeremy

[pfeldman, 2013-06-10 09:28:27]

devtools lgtm

[Jói, 2013-06-10 10:16:14]

LGTM, +jam@ FYI for the interface change.

[Nico, 2013-06-10 14:42:17]

tweak_info_plist.py lgtm

Thanks :-)

[jam, 2013-06-10 14:46:40]

please hold off on this, I'll reply soon with more info

[jeremy, 2013-06-10 14:48:41]

jam: np, thanks!

[jam, 2013-06-10 16:07:33]

A few questions:

does telemtry really need the svn revision? i.e. why isn't the product
information enough, can't one get from it to the svn revision?

The reason I ask is that I prefer keeping the content client interface to a
minimum.

[jeremy, 2013-06-10 17:58:11]

re: keeping content interface to a minimum - completely agree, if we can do
this some other way - I'm all for it.

Telemetry needs the SVN revision so we can cache a generated profile
directory and not need to regenerate it for a certain revision of Chrome.

The Chrome SVN revision isn't accessible via ChromeClient at present AFAIK,
it's not included in any of the strings returned.  The closest we have is
the WebKit SVN revision.


On Mon, Jun 10, 2013 at 7:07 PM, <jam@chromium.org> wrote:

> A few questions:
>
> does telemtry really need the svn revision? i.e. why isn't the product
> information enough, can't one get from it to the svn revision?
>
> The reason I ask is that I prefer keeping the content client interface to a
> minimum.
>
>
https://codereview.chromium.**org/16599010/<https://codereview.chromium.org/1...
>

[jam, 2013-06-10 18:20:27]

On 2013/06/10 17:58:11, jeremy wrote:
> re: keeping content interface to a minimum - completely agree, if we can do
> this some other way - I'm all for it.
> 
> Telemetry needs the SVN revision so we can cache a generated profile
> directory and not need to regenerate it for a certain revision of Chrome.

Does this logic need to be done per svn revision, or can the product version
suffice? i.e. ContentClient::GetProduct() returns something like
"Chrome/29.0.1535.0".

> 
> The Chrome SVN revision isn't accessible via ChromeClient at present AFAIK,
> it's not included in any of the strings returned.  The closest we have is
> the WebKit SVN revision.
> 
> 
> On Mon, Jun 10, 2013 at 7:07 PM, <mailto:jam@chromium.org> wrote:
> 
> > A few questions:
> >
> > does telemtry really need the svn revision? i.e. why isn't the product
> > information enough, can't one get from it to the svn revision?
> >
> > The reason I ask is that I prefer keeping the content client interface to a
> > minimum.
> >
> >
>
https://codereview.chromium.**org/16599010/%3Chttps://codereview.chromium.org...>
> >

[jeremy, 2013-06-10 18:23:10]

No, unfortunately we really need the SVN revision - e.g. someone breaks
something about the profile generation in one version of Chrome and then
we'll be testing against that until the product version gets updated rather
than the next revision which fixes the problem.


On Mon, Jun 10, 2013 at 9:20 PM, <jam@chromium.org> wrote:

> On 2013/06/10 17:58:11, jeremy wrote:
>
>> re: keeping content interface to a minimum - completely agree, if we can
>> do
>> this some other way - I'm all for it.
>>
>
>  Telemetry needs the SVN revision so we can cache a generated profile
>> directory and not need to regenerate it for a certain revision of Chrome.
>>
>
> Does this logic need to be done per svn revision, or can the product
> version
> suffice? i.e. ContentClient::GetProduct() returns something like
> "Chrome/29.0.1535.0".
>
>
>
>  The Chrome SVN revision isn't accessible via ChromeClient at present
>> AFAIK,
>> it's not included in any of the strings returned.  The closest we have is
>> the WebKit SVN revision.
>>
>
>
>  On Mon, Jun 10, 2013 at 7:07 PM, <mailto:jam@chromium.org> wrote:
>>
>
>  > A few questions:
>> >
>> > does telemtry really need the svn revision? i.e. why isn't the product
>> > information enough, can't one get from it to the svn revision?
>> >
>> > The reason I ask is that I prefer keeping the content client interface
>> to a
>> > minimum.
>> >
>> >
>>
>
> https://codereview.chromium.****org/16599010/%3Chttps://codere**
> view.chromium.org/16599010/ <http://codereview.chromium.org/16599010/>>
>
>> >
>>
>
>
>
https://codereview.chromium.**org/16599010/<https://codereview.chromium.org/1...
>

[jam, 2013-06-10 18:29:10]

On 2013/06/10 18:23:10, jeremy wrote:
> No, unfortunately we really need the SVN revision - e.g. someone breaks
> something about the profile generation in one version of Chrome and then
> we'll be testing against that until the product version gets updated rather
> than the next revision which fixes the problem.

Has this actually happened in practice? i.e. profile generation breaking?

[jeremy, 2013-06-10 18:32:55]

Dunno, but IMHO it's not too far fetched :o/

Another scenario would be someone regressing performance by changing the
profile in some way, if we only generate profiles per chrome revision
rather than svn revision that limits the granularity of our tests in
identifying such a regression.


On Mon, Jun 10, 2013 at 9:29 PM, <jam@chromium.org> wrote:

> On 2013/06/10 18:23:10, jeremy wrote:
>
>> No, unfortunately we really need the SVN revision - e.g. someone breaks
>> something about the profile generation in one version of Chrome and then
>> we'll be testing against that until the product version gets updated
>> rather
>> than the next revision which fixes the problem.
>>
>
> Has this actually happened in practice? i.e. profile generation breaking?
>
>
https://codereview.chromium.**org/16599010/<https://codereview.chromium.org/1...
>

[jam, 2013-06-10 19:25:30]

On 2013/06/10 18:32:55, jeremy wrote:
> Dunno, but IMHO it's not too far fetched :o/
> 
> Another scenario would be someone regressing performance by changing the
> profile in some way, if we only generate profiles per chrome revision
> rather than svn revision that limits the granularity of our tests in
> identifying such a regression.

I'd really prefer not to add this unless we know that we need it for sure.