Move Android user agent generation to native code

Move Android user agent generation to native code

Gets rid of a pathway that forced us to set the user agent
from the Java side, replacing it with one that's entirely
native.  Functions have been added to base to allow accessing
information about the device and Android build, while the
user agent generation code was moved to the webkit_glue
namespace.

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

[gone, 2012-08-10 01:17:55]

This is part of a set of CLs that changes how Android user agents are generated.
 This one in particular addresses some cruft in crbug.com/131312, but it's a
stepping stone to address crbug.com/128570.

Thanks!

[Yaron, 2012-08-14 01:44:44]

android-y parts lgtm

[jamesr, 2012-08-14 02:06:50]

webkit/ LGTM

[Charlie Reis, 2012-08-14 17:07:16]

LGTM.  Happy to see it moving out of content.  Just curious about whether this
will help with other platforms.

https://chromiumcodereview.appspot.com/10832235/diff/3001/base/sys_info.h
File base/sys_info.h (right):

https://chromiumcodereview.appspot.com/10832235/diff/3001/base/sys_info.h#new...
base/sys_info.h:85: static std::string OperatingSystemInformation();
Maybe move this near OperatingSystemName and OperatingSystemVersion above? 
(Perhaps with a comment about how it differs from them.  This is specifically
for user agent strings, right?)

Is there a reason it's Android specific, or are you planning to change that
later?  Sounds like it might be useful for
https://chromiumcodereview.appspot.com/10827146/.

[gone, 2012-08-14 17:31:01]

https://chromiumcodereview.appspot.com/10832235/diff/3001/base/sys_info.h
File base/sys_info.h (right):

https://chromiumcodereview.appspot.com/10832235/diff/3001/base/sys_info.h#new...
base/sys_info.h:85: static std::string OperatingSystemInformation();
On 2012/08/14 17:07:16, creis wrote:
> Maybe move this near OperatingSystemName and OperatingSystemVersion above? 
> (Perhaps with a comment about how it differs from them.  This is specifically
> for user agent strings, right?)
> 
> Is there a reason it's Android specific, or are you planning to change that
> later?  Sounds like it might be useful for
> https://chromiumcodereview.appspot.com/10827146/.

Moved it upwards in the following patch set.

It's used in two different places: the main use is for the user agent, but we
also display it in our about menu.  The latter use case is the other reason why
this is moving actually: this string was inaccessible from where we needed it
because of layer separation.

The call is guarded for Android for the moment; I'm still trying to figure out
how to expose it for http://crbug.com/128570, but I'll likely have to yank out
bits of the function for that CL.

[gone, 2012-08-14 17:33:25]

Added base OWNERS; wasn't sure who specifically to ping.

[willchan no longer on Chromium, 2012-08-14 17:51:09]

https://chromiumcodereview.appspot.com/10832235/diff/3003/base/sys_info.h
File base/sys_info.h (right):

https://chromiumcodereview.appspot.com/10832235/diff/3003/base/sys_info.h#new...
base/sys_info.h:54: #if defined(OS_ANDROID)
Why does this live here instead of webkit/glue, where the rest of the user agent
string is defined?

https://chromiumcodereview.appspot.com/10832235/diff/3003/base/sys_info_andro...
File base/sys_info_android.cc (right):

https://chromiumcodereview.appspot.com/10832235/diff/3003/base/sys_info_andro...
base/sys_info_android.cc:142: if (strlen(os_build_id_str) > 0) {
You don't need strlen, just see if (os_build_id_str[0]). No need to check string
size if you just want to know if it's empty. Better yet, use the return value of
__system_property_get().

[gone, 2012-08-14 17:58:37]

https://chromiumcodereview.appspot.com/10832235/diff/3003/base/sys_info.h
File base/sys_info.h (right):

https://chromiumcodereview.appspot.com/10832235/diff/3003/base/sys_info.h#new...
base/sys_info.h:54: #if defined(OS_ANDROID)
On 2012/08/14 17:51:09, willchan wrote:
> Why does this live here instead of webkit/glue, where the rest of the user
agent
> string is defined?

It's not used solely for the user agent, but I'll move it over.  Probably would
have had to go there for the subsequent CL, anyway.

https://chromiumcodereview.appspot.com/10832235/diff/3003/base/sys_info_andro...
File base/sys_info_android.cc (right):

https://chromiumcodereview.appspot.com/10832235/diff/3003/base/sys_info_andro...
base/sys_info_android.cc:142: if (strlen(os_build_id_str) > 0) {
On 2012/08/14 17:51:09, willchan wrote:
> You don't need strlen, just see if (os_build_id_str[0]). No need to check
string
> size if you just want to know if it's empty. Better yet, use the return value
of
> __system_property_get().

Done.

[gone, 2012-08-15 21:04:22]

+klobag as FYI for the user agent string changes

Moved the bulk of the OS string generation to webkit_glue and cleaned it up
slightly for future usage.  PTAL.

[jamesr, 2012-08-15 21:07:50]

https://chromiumcodereview.appspot.com/10832235/diff/3004/webkit/glue/user_ag...
File webkit/glue/user_agent.h (right):

https://chromiumcodereview.appspot.com/10832235/diff/3004/webkit/glue/user_ag...
webkit/glue/user_agent.h:20: std::string BuildOSCpuInfoForAndroid();
why is this publicly exposed as a separate entry point from BuildOSCpuInfo()? 
If you're on android, obviously this will return you the android OS/Cpu info. 
If you aren't on android, then this function isn't of any immediate use
(obviously somebody will want to add spoofing support in the future, but that's
different).

[gone, 2012-08-15 21:12:01]

On 2012/08/15 21:07:50, jamesr wrote:
>
https://chromiumcodereview.appspot.com/10832235/diff/3004/webkit/glue/user_ag...
> File webkit/glue/user_agent.h (right):
> 
>
https://chromiumcodereview.appspot.com/10832235/diff/3004/webkit/glue/user_ag...
> webkit/glue/user_agent.h:20: std::string BuildOSCpuInfoForAndroid();
> why is this publicly exposed as a separate entry point from BuildOSCpuInfo()? 
> If you're on android, obviously this will return you the android OS/Cpu info. 
> If you aren't on android, then this function isn't of any immediate use
> (obviously somebody will want to add spoofing support in the future, but
that's
> different).

It's exposed explicitly for spoofing in a follow-up CL, but the string this
function generates is used by other parts of the Android code that aren't
user-agent related (which is why I didn't put it here in the first place).

Do you want me to combine this with BuildOSCpuInfo, just guard it off with #if
defined(OS_ANDROID), or something else?

[jamesr, 2012-08-15 21:15:57]

I don't think you should add non-user-agent related code to
webkit/glue/user_agent.h.  I also dislike speculatively adding API that will be
used by some future patch - add the API in the patch that uses it.

The bigger issue is I don't understand how the two functions BuildOSCpuInfo()
and BuildOSCpuInfoForAndroid() are supposed to be related to each other on
android.

[gone, 2012-08-15 21:27:04]

Bigger picture is that BuildOSCpuInfo will have to return two different strings
for Android: one with all the Android build info for when we're using a real
user agent "(Linux; Android 4.0.4; Galaxy Whatever Build/IMM76D)" and one that
hides the fact that we're on Android to spoof a desktop user agent "(Linux arm)"
that is currently generated in the #else clause of BuildOSCpuInfo().

Logically, I think the function makes sense when it's located in base for the
current use case, but there were objections to putting it there so I don't know
where it should go.

[gone, 2012-08-15 21:37:14]

For context, the string generated by that function is used in
chrome/browser/ui/webui/about_ui.cc downstream and is used in the About page of
Chrome for Android.  I could upstream those four lines as part of this patch,
but I don't think that would change your objections about putting this in the
user_agent.* files.

[jamesr, 2012-08-15 21:40:22]

It's used as part of the user agent, but what does that substring "(Linux;
Android 4.0.4; Galaxy Whatever Build/IMM76D)" or "(Linux arm)" represent?  Is it
the OS line of about:version?

[gone, 2012-08-15 21:52:39]

On 2012/08/15 21:40:22, jamesr wrote:
> It's used as part of the user agent, but what does that substring "(Linux;
> Android 4.0.4; Galaxy Whatever Build/IMM76D)" or "(Linux arm)" represent?  Is
it
> the OS line of about:version?

Yeah, "Android 4.0.4; Galaxy Whatever Build/IMM76D" is what gets reported on our
master downstream branch for the OS version on the about:version page (the
publicly available version of Chrome for Android just says "Android").  "Linux
arm" would only be used for spoofing a user agent for Android's Request Desktop
Site.

[jamesr, 2012-08-15 22:26:17]

On 2012/08/15 21:52:39, dfalcantara wrote:
> On 2012/08/15 21:40:22, jamesr wrote:
> > It's used as part of the user agent, but what does that substring "(Linux;
> > Android 4.0.4; Galaxy Whatever Build/IMM76D)" or "(Linux arm)" represent? 
Is
> it
> > the OS line of about:version?
> 
> Yeah, "Android 4.0.4; Galaxy Whatever Build/IMM76D" is what gets reported on
our
> master downstream branch for the OS version on the about:version page (the
> publicly available version of Chrome for Android just says "Android").  "Linux
> arm" would only be used for spoofing a user agent for Android's Request
Desktop
> Site.

So this code:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/chrome_versi...
?  Why does your string need to be so particular - we seem to just report the
major OS version on other systems.  The full user agent string is also there on
about:version so I don't really get why would you need both.

I think the way to deal with "Linux arm" is to have a version of this function
that lets you pass in an OSInfo string to use instead of the real one, but not
to add additional entry points for substrings if you don't need them.

[gone, 2012-08-15 22:52:06]

> So this code:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/chrome_versi...
> ?  Why does your string need to be so particular - we seem to just report the
> major OS version on other systems.  The full user agent string is also there
on
> about:version so I don't really get why would you need both.
Not sure that's the right place; this is the bug that called for the change:
http://crbug.com/140042, but I don't know how that string gets plumbed around. 
The change affects both about:version and the Chrome for Android About fragment
(accessed through the menu), but the latter doesn't display user agent info.

> I think the way to deal with "Linux arm" is to have a version of this function
> that lets you pass in an OSInfo string to use instead of the real one, but not
> to add additional entry points for substrings if you don't need them.
That sounds good to me.  Would you object to keeping the Android string
generation in user_agent.cc as a separate function and removing it from the
header?

[gone, 2012-08-15 22:53:03]

On 2012/08/15 22:52:06, dfalcantara wrote:
> > So this code:
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/chrome_versi...
> > ?  Why does your string need to be so particular - we seem to just report
the
> > major OS version on other systems.  The full user agent string is also there
> on
> > about:version so I don't really get why would you need both.
> Not sure that's the right place; this is the bug that called for the change:
> http://crbug.com/140042, but I don't know how that string gets plumbed around.

> The change affects both about:version and the Chrome for Android About
fragment
> (accessed through the menu), but the latter doesn't display user agent info.
> 
> > I think the way to deal with "Linux arm" is to have a version of this
function
> > that lets you pass in an OSInfo string to use instead of the real one, but
not
> > to add additional entry points for substrings if you don't need them.
> That sounds good to me.  Would you object to keeping the Android string
> generation in user_agent.cc as a separate function and removing it from the
> header?

Er, I guess removing it from the header is only possible if we move this back
into base since it's called from outside...

[jamesr, 2012-08-15 22:59:09]

That crbug seems misguided to me.  I would argue that about:version should list
"OS: Android" and the full user agent string just as it does on every other
platform. Then you don't need to expose a separate bit.

[srikanth, 2012-08-15 23:08:48]

On 2012/08/15 22:59:09, jamesr wrote:
> That crbug seems misguided to me.  I would argue that about:version should
list
> "OS: Android" and the full user agent string just as it does on every other
> platform. Then you don't need to expose a separate bit.

"OS: Android" is not descriptive enough given the way the OS gets deployed. We
do have the OS version details in the UA string as well, but do need a more
complete listing of the OS version so users reporting issues can do so clearly.

[jamesr, 2012-08-15 23:10:33]

On 2012/08/15 23:08:48, srikanth wrote:
> On 2012/08/15 22:59:09, jamesr wrote:
> > That crbug seems misguided to me.  I would argue that about:version should
> list
> > "OS: Android" and the full user agent string just as it does on every other
> > platform. Then you don't need to expose a separate bit.
> 
> "OS: Android" is not descriptive enough given the way the OS gets deployed. We
> do have the OS version details in the UA string as well, but do need a more
> complete listing of the OS version so users reporting issues can do so
clearly.

Let's take this discussion to a crbug - could you file? http://crbug.com/140042
was closed, even though it wasn't fixed upstream.

[jar (doing other things), 2012-08-15 23:35:17]

https://chromiumcodereview.appspot.com/10832235/diff/3004/base/sys_info.h
File base/sys_info.h (right):

https://chromiumcodereview.appspot.com/10832235/diff/3004/base/sys_info.h#new...
base/sys_info.h:102: static const int kDefaultAndroidBugfixVersion = 3;
Why isn't this stuff inside the OS_ANDROID ifdef?

https://chromiumcodereview.appspot.com/10832235/diff/3004/base/sys_info_andro...
File base/sys_info_android.cc (right):

https://chromiumcodereview.appspot.com/10832235/diff/3004/base/sys_info_andro...
base/sys_info_android.cc:24: minor_version, bugfix_version);
nit: better to wrap to paren when you can (and you can here).

https://chromiumcodereview.appspot.com/10832235/diff/3004/base/sys_info_andro...
base/sys_info_android.cc:105: int internal_major_version,
internal_minor_version, internal_bugfix_version;
What is the advantage of having these temporaries?

Why did you define the private function to take int, rather than int32?  ... or
why did you use int32 for this function's arg, rather than int?

https://chromiumcodereview.appspot.com/10832235/diff/3004/webkit/glue/user_ag...
File webkit/glue/user_agent.cc (right):

https://chromiumcodereview.appspot.com/10832235/diff/3004/webkit/glue/user_ag...
webkit/glue/user_agent.cc:109: std::string BuildOSCpuInfoForAndroid() {
Who would ever call this, if not in android?  ... so why not make this ifdef
based on OS_ANDROID?

https://chromiumcodereview.appspot.com/10832235/diff/3004/webkit/glue/user_ag...
webkit/glue/user_agent.cc:138: if (!semicolon_inserted) {
Logic for this seems confused.
semicolon_inserted is always false here.

Did you plan to sometimes set it around lines 127-134?

https://chromiumcodereview.appspot.com/10832235/diff/3004/webkit/glue/user_ag...
webkit/glue/user_agent.cc:149: semicolon_inserted = true;
Why are you setting this variable, given that you never use it again?  The
compiler will discard this setting... but the code  just seems larger for no
good reason.

[gone, 2012-08-16 00:29:31]

Uploaded another patch set addressing jar's comments and guarding all of the
code for Android until code lands that needs them exposed.  Still trying to
figure out where to put the string generation code, though.

https://chromiumcodereview.appspot.com/10832235/diff/3004/base/sys_info.h
File base/sys_info.h (right):

https://chromiumcodereview.appspot.com/10832235/diff/3004/base/sys_info.h#new...
base/sys_info.h:102: static const int kDefaultAndroidBugfixVersion = 3;
On 2012/08/15 23:35:17, jar wrote:
> Why isn't this stuff inside the OS_ANDROID ifdef?

Supposed to be used for spoofing later; I can move it for now.

https://chromiumcodereview.appspot.com/10832235/diff/3004/base/sys_info_andro...
File base/sys_info_android.cc (right):

https://chromiumcodereview.appspot.com/10832235/diff/3004/base/sys_info_andro...
base/sys_info_android.cc:24: minor_version, bugfix_version);
On 2012/08/15 23:35:17, jar wrote:
> nit: better to wrap to paren when you can (and you can here).

Ah, wasn't sure whether the style guide allowed it.

https://chromiumcodereview.appspot.com/10832235/diff/3004/base/sys_info_andro...
base/sys_info_android.cc:105: int internal_major_version,
internal_minor_version, internal_bugfix_version;
On 2012/08/15 23:35:17, jar wrote:
> What is the advantage of having these temporaries?
> 
> Why did you define the private function to take int, rather than int32?  ...
or
> why did you use int32 for this function's arg, rather than int?

They're defined as int32s for all of the other platforms, and I'm not sure if
sscanf will safely take int32s.  One option is to move the temporaries upwards
to the sscanf, I guess.

https://chromiumcodereview.appspot.com/10832235/diff/3004/webkit/glue/user_ag...
File webkit/glue/user_agent.cc (right):

https://chromiumcodereview.appspot.com/10832235/diff/3004/webkit/glue/user_ag...
webkit/glue/user_agent.cc:109: std::string BuildOSCpuInfoForAndroid() {
On 2012/08/15 23:35:17, jar wrote:
> Who would ever call this, if not in android?  ... so why not make this ifdef
> based on OS_ANDROID?

It's supposed to be used for spoofing the UA in a follow-up CL.  I can just
#ifdef it out until that one lands.

https://chromiumcodereview.appspot.com/10832235/diff/3004/webkit/glue/user_ag...
webkit/glue/user_agent.cc:138: if (!semicolon_inserted) {
On 2012/08/15 23:35:17, jar wrote:
> Logic for this seems confused.
> semicolon_inserted is always false here.
> 
> Did you plan to sometimes set it around lines 127-134?

Good catch, moved it downward.

https://chromiumcodereview.appspot.com/10832235/diff/3004/webkit/glue/user_ag...
webkit/glue/user_agent.cc:149: semicolon_inserted = true;
On 2012/08/15 23:35:17, jar wrote:
> Why are you setting this variable, given that you never use it again?  The
> compiler will discard this setting... but the code  just seems larger for no
> good reason.

Done.

[gone, 2012-08-17 00:26:19]

Based on discussion:
* The code for generating the string has been fully meshed into the
BuildOSCpuInfo() function and guarded with #ifdefs just like the other OSes do. 
Spoofing will try following jamesr's suggestion of passing in some string to be
used as the OS/CPU string instead of running the #if defined(OS_ANDROID)
pathway.

* Code for pulling information from Android that the user agent code requires
has been stuffed into base/sys_info* since it doesn't make sense to put them in
user_agent.cc.

* Any code that hasn't been upstreamed that uses this string will be changed
downstream to go down a different pathway, making the code's use exclusively for
generating user agents.

Please take another look.

[jamesr, 2012-08-20 22:59:06]

webkit/ lgtm

[willchan no longer on Chromium, 2012-08-20 23:07:42]

base/ LGTM

https://chromiumcodereview.appspot.com/10832235/diff/14021/base/sys_info_andr...
File base/sys_info_android.cc (right):

https://chromiumcodereview.appspot.com/10832235/diff/14021/base/sys_info_andr...
base/sys_info_android.cc:28: if (strlen(os_version_str) > 0) {
How about: if (os_version_str[0])?
No need to waste time doing a strlen if you just want to see that it's
non-empty.

[Yaron, 2012-08-20 23:13:10]

lgtm

Feel free to do as a follow-up

https://chromiumcodereview.appspot.com/10832235/diff/14021/base/sys_info_andr...
File base/sys_info_android.cc (right):

https://chromiumcodereview.appspot.com/10832235/diff/14021/base/sys_info_andr...
base/sys_info_android.cc:87: char os_version_codename_str[PROP_VALUE_MAX];
Would be good to reconcile these with build/android/build_info.h (perhaps
replace build_info with sys_info since that's the more common api used across
ports)

[Charlie Reis, 2012-08-21 01:05:01]

Still LGTM.

[gone, 2012-08-21 18:46:46]

Thanks for the reviews, sending it up.

https://chromiumcodereview.appspot.com/10832235/diff/14021/base/sys_info_andr...
File base/sys_info_android.cc (right):

https://chromiumcodereview.appspot.com/10832235/diff/14021/base/sys_info_andr...
base/sys_info_android.cc:28: if (strlen(os_version_str) > 0) {
On 2012/08/20 23:07:42, willchan wrote:
> How about: if (os_version_str[0])?
> No need to waste time doing a strlen if you just want to see that it's
> non-empty.

This change got lost in my last shuffle.  Fixed.

https://chromiumcodereview.appspot.com/10832235/diff/14021/base/sys_info_andr...
base/sys_info_android.cc:87: char os_version_codename_str[PROP_VALUE_MAX];
On 2012/08/20 23:13:10, Yaron wrote:
> Would be good to reconcile these with build/android/build_info.h (perhaps
> replace build_info with sys_info since that's the more common api used across
> ports)

Yeah, need to figure out if the strings are accessible when Chrome crashes,
though.

[commit-bot: I haz the power, 2012-08-21 18:48:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/10832235/8035

[commit-bot: I haz the power, 2012-08-21 20:27:52]

Try job failure for 10832235-8035 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-21 20:46:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/10832235/8035

[commit-bot: I haz the power, 2012-08-21 23:55:55]

Change committed as 152675