[telemetry] Fix user agent string for cros.

[telemetry] Fix user agent string for cros.

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

[hartmanng, 2013-01-02 16:26:17]

Please take a look.

[hartmanng, 2013-01-03 14:46:59]

Oops, looks like bulach is still on vacation. Tony, if you have time, could you
take a quick look?

[tonyg, 2013-01-03 18:33:14]

https://codereview.chromium.org/11745002/diff/3001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page_runner_unittest.py (right):

https://codereview.chromium.org/11745002/diff/3001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_runner_unittest.py:122: expected_user_agent =
'\'%s\'' % user_agent.UA_TYPE_MAPPING['tablet']
Recommend quoting this string in double quotes so that you don't have to use
escaping for the single quotes.

https://codereview.chromium.org/11745002/diff/3001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/user_agent.py (right):

https://codereview.chromium.org/11745002/diff/3001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/user_agent.py:27: return ['--user-agent=\'%s\'' %
UA_TYPE_MAPPING[user_agent_type]]
I tested this change on android and it looks like it breaks it. The single tics
themselves appear in the user agent string. Do you have an android device to
test this change with?

[hartmanng, 2013-01-03 18:49:42]

https://codereview.chromium.org/11745002/diff/3001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/user_agent.py (right):

https://codereview.chromium.org/11745002/diff/3001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/user_agent.py:27: return ['--user-agent=\'%s\'' %
UA_TYPE_MAPPING[user_agent_type]]
On 2013/01/03 18:33:14, tonyg wrote:
> I tested this change on android and it looks like it breaks it. The single
tics
> themselves appear in the user agent string. Do you have an android device to
> test this change with?

No, but I think Ian has a nexus 7 I might be able to use.

[bulach, 2013-01-08 10:32:30]

+joth, as he implemented CommandLine.java mentioned below.

some thoughts below:

https://codereview.chromium.org/11745002/diff/3001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/user_agent.py (right):

https://codereview.chromium.org/11745002/diff/3001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/user_agent.py:27: return ['--user-agent=\'%s\'' %
UA_TYPE_MAPPING[user_agent_type]]
On 2013/01/03 18:49:42, hartmanng wrote:
> On 2013/01/03 18:33:14, tonyg wrote:
> > I tested this change on android and it looks like it breaks it. The single
> tics
> > themselves appear in the user agent string. Do you have an android device to
> > test this change with?
> 
> No, but I think Ian has a nexus 7 I might be able to use.

iirc,
content/public/android/java/src/org/chromium/content/common/CommandLine.java,
requires double quotes for its argument parsing..
I don't quite recall the full story behind this, but I think was some degree of
consistency with base/command_line.cc, where it uses double quotes as required
by windows...

sigh :) maybe we could pass a "flag_quote_char" param in here, and have it as a
"virtual" method in browser_backend.py so that platforms can specify whatever is
appropriate?

[joth, 2013-01-09 01:37:50]

On 8 January 2013 02:32, <bulach@chromium.org> wrote:

> +joth, as he implemented CommandLine.java mentioned below.
>
> some thoughts below:
>
>
>
>
> https://codereview.chromium.**org/11745002/diff/3001/tools/**
>
telemetry/telemetry/user_**agent.py<https://codereview.chromium.org/11745002/diff/3001/tools/telemetry/telemetry/user_agent.py>
> File tools/telemetry/telemetry/**user_agent.py (right):
>
> https://codereview.chromium.**org/11745002/diff/3001/tools/**
>
telemetry/telemetry/user_**agent.py#newcode27<https://codereview.chromium.org/11745002/diff/3001/tools/telemetry/telemetry/user_agent.py#newcode27>
> tools/telemetry/telemetry/**user_agent.py:27: return
> ['--user-agent=\'%s\'' % UA_TYPE_MAPPING[user_agent_**type]]
> On 2013/01/03 18:49:42, hartmanng wrote:
>
>> On 2013/01/03 18:33:14, tonyg wrote:
>> > I tested this change on android and it looks like it breaks it. The
>>
> single
>
>> tics
>> > themselves appear in the user agent string. Do you have an android
>>
> device to
>
>> > test this change with?
>>
>
>  No, but I think Ian has a nexus 7 I might be able to use.
>>
>
> iirc,
> content/public/android/java/**src/org/chromium/content/**
> common/CommandLine.java,
> requires double quotes for its argument parsing..
> I don't quite recall the full story behind this, but I think was some
> degree of consistency with base/command_line.cc, where it uses double
> quotes as required by windows...
>
>
almost, not quite. Windows (and mac and unix) rely on the system shell to
provide the argv array so there is no equivalent in command_line.cc. you're
probably thinking of the --foo=bar parsing, which is shared, but has no
special quote handling, it just splits on the first '=' if finds I believe.
But yes, on Android, it supports double quotes only right now. (it predated
even me... I just moved the quote parsing code out of the
app constructor and into CommandLine. think tedbo was original author... :)






> sigh :) maybe we could pass a "flag_quote_char" param in here, and have
> it as a "virtual" method in browser_backend.py so that platforms can
> specify whatever is appropriate?
>
>
https://codereview.chromium.**org/11745002/<https://codereview.chromium.org/1...
>

[hartmanng, 2013-01-10 14:50:49]

Thanks for the input everyone!

It looks like this actually works without quotes at all (see
http://code.google.com/p/chromium/issues/detail?id=167979#c11).

I ran the user_agent_unittests with this patch applied on desktop, android, and
cros. Desktop and Android pass, cros still has the dbus-send comma-split issue,
but at least it doesn't crash anymore.

That said, I also tried running on android with single-ticks and mine didn't
seem to break. Tony, what exactly happened when you ran with single-ticks on
android? Maybe there's an edge case I'm missing.

PTAL

[tonyg, 2013-01-10 18:37:28]

lgtm

In answer to your question, with the other patch, the single tics showed up in
the user agent.

https://codereview.chromium.org/11745002/diff/9001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page_runner_unittest.py (right):

https://codereview.chromium.org/11745002/diff/9001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_runner_unittest.py:122: expected_user_agent =
'%s' % user_agent.UA_TYPE_MAPPING['tablet']
The "'%s' %" has no effect. Just use the variable by itself.

https://codereview.chromium.org/11745002/diff/9001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/user_agent_unittest.py (right):

https://codereview.chromium.org/11745002/diff/9001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/user_agent_unittest.py:15: self.assertEquals(ua, '%s'
% user_agent.UA_TYPE_MAPPING['tablet'])
ditto

[hartmanng, 2013-01-10 18:46:06]

> In answer to your question, with the other patch, the single tics showed up in
> the user agent.

Ah I see, thanks for the explanation and review!

https://codereview.chromium.org/11745002/diff/9001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page_runner_unittest.py (right):

https://codereview.chromium.org/11745002/diff/9001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_runner_unittest.py:122: expected_user_agent =
'%s' % user_agent.UA_TYPE_MAPPING['tablet']
On 2013/01/10 18:37:28, tonyg wrote:
> The "'%s' %" has no effect. Just use the variable by itself.

Done.

https://codereview.chromium.org/11745002/diff/9001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/user_agent_unittest.py (right):

https://codereview.chromium.org/11745002/diff/9001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/user_agent_unittest.py:15: self.assertEquals(ua, '%s'
% user_agent.UA_TYPE_MAPPING['tablet'])
On 2013/01/10 18:37:28, tonyg wrote:
> ditto

Done.

[commit-bot: I haz the power, 2013-01-10 18:47:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/11745002/7002

[commit-bot: I haz the power, 2013-01-10 23:47:23]

Change committed as 176210