WebView: Add channel info for UMA

WebView: Add channel info for UMA

Inspect the WebView provider package name to determine the channel. This
only works for Monochrome (on Android N+); stand-alone WebView uses the
same package across channels, and will still be reported as "unknown".

Move version_utils from metrics/net/ to metrics/, and move the
channel-checking code from chrome/browser/ to version_info/, so that
WebView can use them.

Split off the part of version_utils that depends on //ui/base, since
most users of version_utils don't need that part, and it pulls in a huge
tree of dependencies.

BUG=711084
Review-Url: https://codereview.chromium.org/2879703002
Cr-Commit-Position: refs/heads/master@{#477124}
Committed: https://chromium.googlesource.com/chromium/src/+/234ec23e9f56bb01af5c59922681b114da3aa540

[paulmiller, 2017-05-11 22:08:21]

Description was changed from

==========
WebView: Add channel info for UMA

Inspect the WebView provider package name to determine the channel. This
only works for Monochrome (on Android N+); stand-alone WebView uses the
same package across channels, and will still be reported as "unknown".

Move version_utils from metrics/net/ to metrics/, and move the
channel-checking code from chrome/browser/ to version_info/, so that
WebView can use them.

BUG=711084
==========

to

==========
WebView: Add channel info for UMA

Inspect the WebView provider package name to determine the channel. This
only works for Monochrome (on Android N+); stand-alone WebView uses the
same package across channels, and will still be reported as "unknown".

Move version_utils from metrics/net/ to metrics/, and move the
channel-checking code from chrome/browser/ to version_info/, so that
WebView can use them.

BUG=711084
==========

[paulmiller, 2017-05-11 22:08:21]

paulmiller@chromium.org changed reviewers:
+ asvitkine@chromium.org, rohitrao@chromium.org, sgurun@chromium.org,
thakis@chromium.org

[paulmiller, 2017-05-11 22:11:49]

On 2017/05/11 22:08:21, paulmiller wrote:
> mailto:paulmiller@chromium.org changed reviewers:
> + mailto:asvitkine@chromium.org, mailto:rohitrao@chromium.org,
mailto:sgurun@chromium.org,
> mailto:thakis@chromium.org

PTAL

asvitkine@ for chrome/browser/metrics and components/metrics/
rohitrao@ for ios/chrome/
sgurun@ for android_webview/
thakis@ for chrome/common/ and components/version_info/

sgurun@, this is based on gsennton@'s pending
https://codereview.chromium.org/2833893002/, which adds sWebViewPackageName to
AwBrowserProcess.

[Nico, 2017-05-11 22:36:22]

chrome lgtm

[Nico, 2017-05-11 22:37:02]

(I'm a bit surprised you bake the channel into the executable like that
though. Do we do that on Desktop too?)

On Thu, May 11, 2017 at 6:36 PM, <thakis@chromium.org> wrote:

> chrome lgtm
>
> https://codereview.chromium.org/2879703002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[paulmiller, 2017-05-11 22:42:35]

On 2017/05/11 22:37:02, Nico wrote:
> (I'm a bit surprised you bake the channel into the executable like that
> though. Do we do that on Desktop too?)
> 

I'm not familiar with desktop. On Android, the only way to tell is by the
package name, AFAIK.

[rohitrao (ping after 24h), 2017-05-12 00:56:52]

ios/ lgtm

[Alexei Svitkine (slow), 2017-05-12 14:26:22]

lgtm

https://codereview.chromium.org/2879703002/diff/1/android_webview/browser/aw_...
File android_webview/browser/aw_metrics_service_client_impl.cc (right):

https://codereview.chromium.org/2879703002/diff/1/android_webview/browser/aw_...
android_webview/browser/aw_metrics_service_client_impl.cc:99: channel_ =
version_info::ChannelFromPackageName(package_name.c_str());
Nit: This seems like a good candidate for a helper function in the anon
namespace.

https://codereview.chromium.org/2879703002/diff/1/components/version_info/cha...
File components/version_info/channel_android.h (right):

https://codereview.chromium.org/2879703002/diff/1/components/version_info/cha...
components/version_info/channel_android.h:12: Channel
ChannelFromPackageName(const char* name);
Nit: package_name

Also add a short 1-line comment.

[paulmiller, 2017-05-12 19:45:36]

https://codereview.chromium.org/2879703002/diff/1/android_webview/browser/aw_...
File android_webview/browser/aw_metrics_service_client_impl.cc (right):

https://codereview.chromium.org/2879703002/diff/1/android_webview/browser/aw_...
android_webview/browser/aw_metrics_service_client_impl.cc:99: channel_ =
version_info::ChannelFromPackageName(package_name.c_str());
On 2017/05/12 14:26:22, Alexei Svitkine (slow) wrote:
> Nit: This seems like a good candidate for a helper function in the anon
> namespace.

Done.

https://codereview.chromium.org/2879703002/diff/1/components/version_info/cha...
File components/version_info/channel_android.h (right):

https://codereview.chromium.org/2879703002/diff/1/components/version_info/cha...
components/version_info/channel_android.h:12: Channel
ChannelFromPackageName(const char* name);
On 2017/05/12 14:26:22, Alexei Svitkine (slow) wrote:
> Nit: package_name
> 
> Also add a short 1-line comment.

Done.

[Torne, 2017-05-12 19:55:32]

On 2017/05/11 22:42:35, paulmiller wrote:
> On 2017/05/11 22:37:02, Nico wrote:
> > (I'm a bit surprised you bake the channel into the executable like that
> > though. Do we do that on Desktop too?)
> > 
> 
> I'm not familiar with desktop. On Android, the only way to tell is by the
> package name, AFAIK.

Yeah, on Android it's the name of the application package, which is just
specified in an XML file in the .APK archive and isn't actually included in the
compiled code.

On Linux Desktop the channel is set in the environment by the shell script which
launches the actual binary. I don't know about Windows or Mac but I suspect it's
also derived from similar not-actually-the-executable data that's included in
the installation package.

[paulmiller, 2017-05-12 22:32:19]

paulmiller@chromium.org changed reviewers:
+ torne@chromium.org
- sgurun@chromium.org

[paulmiller, 2017-05-12 22:33:26]

replacing sgurun@ with torne@, at sgurun@'s request

[paulmiller, 2017-05-13 00:53:48]

The CQ bit was checked by paulmiller@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-13 00:54:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-13 00:59:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-13 00:59:12]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[paulmiller, 2017-05-13 01:19:13]

The CQ bit was checked by paulmiller@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-13 01:19:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-13 01:42:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-13 01:42:55]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[Torne, 2017-05-15 18:33:36]

LGTM

[paulmiller, 2017-05-19 23:50:39]

Blegh. It looks like, through an obscure chain of dependencies, making metrics
depend on version_info causes cronet to depend on libwebp. cronet is configured
with arm_use_neon=false, but libwebp doesn't respect that, and tries to use NEON
anyway.

[Torne, 2017-05-22 15:48:15]

On 2017/05/19 23:50:39, paulmiller wrote:
> Blegh. It looks like, through an obscure chain of dependencies, making metrics
> depend on version_info causes cronet to depend on libwebp. cronet is
configured
> with arm_use_neon=false, but libwebp doesn't respect that, and tries to use
NEON
> anyway.

The dependency chain there seems pretty wrong, in that case. Even if libwebp
dealt with arm_use_neon correctly this seems like it'd be a binary size
regression for cronet ;)

[paulmiller, 2017-05-22 20:57:35]

On 2017/05/22 15:48:15, Torne wrote:
> On 2017/05/19 23:50:39, paulmiller wrote:
> > Blegh. It looks like, through an obscure chain of dependencies, making
metrics
> > depend on version_info causes cronet to depend on libwebp. cronet is
> configured
> > with arm_use_neon=false, but libwebp doesn't respect that, and tries to use
> NEON
> > anyway.
> 
> The dependency chain there seems pretty wrong, in that case. Even if libwebp
> dealt with arm_use_neon correctly this seems like it'd be a binary size
> regression for cronet ;)

The chain is:

//components/cronet/android:cronet
//components/cronet/android:cronet_static
//components/metrics:metrics
//components/version_info:version_info
//ui/base:base
//skia:skia
//third_party/libwebp:libwebp

The version_info > ui part strikes me as the sketchiest link here, and indeed
they're doing that only conditionally and only to get one random function,
l10n_util::GetStringUTF8. I'd like to refactor that dependency into a separate
target inside version_info, as most users of version_info don't seem to need it.
Unfortunately, most users of version_info depend on the whole directory, so I'd
have to update all of those to get the full savings.

Regardless, the libwebp thing is still a bug. I've been talking to jzern@ and
he's planning to fix that.

[paulmiller, 2017-05-22 22:06:45]

Description was changed from

==========
WebView: Add channel info for UMA

Inspect the WebView provider package name to determine the channel. This
only works for Monochrome (on Android N+); stand-alone WebView uses the
same package across channels, and will still be reported as "unknown".

Move version_utils from metrics/net/ to metrics/, and move the
channel-checking code from chrome/browser/ to version_info/, so that
WebView can use them.

BUG=711084
==========

to

==========
WebView: Add channel info for UMA

Inspect the WebView provider package name to determine the channel. This
only works for Monochrome (on Android N+); stand-alone WebView uses the
same package across channels, and will still be reported as "unknown".

Move version_utils from metrics/net/ to metrics/, and move the
channel-checking code from chrome/browser/ to version_info/, so that
WebView can use them.

Split off the part of version_utils that depends on //ui/base, since
most users of version_utils don't need that part, and it pulls in a huge tree of
dependencies.

BUG=711084
==========

[paulmiller, 2017-05-22 22:07:14]

The CQ bit was checked by paulmiller@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 22:07:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 22:13:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 22:13:04]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[paulmiller, 2017-05-22 22:17:11]

The CQ bit was checked by paulmiller@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 22:17:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 00:13:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 00:13:18]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Alexei Svitkine (slow), 2017-05-23 15:49:58]

Another option would be to move l10n_util::GetStringUTF8() to a separate
target, so that version_info doesn't need to depend on all of ui/base.

On Mon, May 22, 2017 at 4:57 PM, <paulmiller@chromium.org> wrote:

> On 2017/05/22 15:48:15, Torne wrote:
> > On 2017/05/19 23:50:39, paulmiller wrote:
> > > Blegh. It looks like, through an obscure chain of dependencies, making
> metrics
> > > depend on version_info causes cronet to depend on libwebp. cronet is
> > configured
> > > with arm_use_neon=false, but libwebp doesn't respect that, and tries
> to use
> > NEON
> > > anyway.
> >
> > The dependency chain there seems pretty wrong, in that case. Even if
> libwebp
> > dealt with arm_use_neon correctly this seems like it'd be a binary size
> > regression for cronet ;)
>
> The chain is:
>
> //components/cronet/android:cronet
> //components/cronet/android:cronet_static
> //components/metrics:metrics
> //components/version_info:version_info
> //ui/base:base
> //skia:skia
> //third_party/libwebp:libwebp
>
> The version_info > ui part strikes me as the sketchiest link here, and
> indeed
> they're doing that only conditionally and only to get one random function,
> l10n_util::GetStringUTF8. I'd like to refactor that dependency into a
> separate
> target inside version_info, as most users of version_info don't seem to
> need it.
> Unfortunately, most users of version_info depend on the whole directory,
> so I'd
> have to update all of those to get the full savings.
>
> Regardless, the libwebp thing is still a bug. I've been talking to jzern@
> and
> he's planning to fix that.
>
> https://codereview.chromium.org/2879703002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[paulmiller, 2017-05-26 00:42:42]

paulmiller@chromium.org changed reviewers:
+ pavely@chromium.org

[paulmiller, 2017-05-26 00:51:47]

Adding pavely@ to PTAL at components/sync/. The context here is that I'm
splitting off the version_info:version_string target so that
version_info:version_info doesn't have to depend on all of ui/base/.

thakis@ might like to take a look at the updated components/version_info/ as
well.

On 2017/05/23 15:49:58, Alexei Svitkine (slow) wrote:
> Another option would be to move l10n_util::GetStringUTF8() to a separate
> target, so that version_info doesn't need to depend on all of ui/base.
>

That would be nice, but it looks like it might be hard to separate, since
l10n_util::GetStringUTF8() depends on l10n_util::GetStringUTF16(), which depends
on ui::ResourceBundle, which depends on ui/base/ and ui/gfx/...

[pavely, 2017-05-30 16:36:16]

components/sync lgtm

[paulmiller, 2017-05-30 17:24:51]

Description was changed from

==========
WebView: Add channel info for UMA

Inspect the WebView provider package name to determine the channel. This
only works for Monochrome (on Android N+); stand-alone WebView uses the
same package across channels, and will still be reported as "unknown".

Move version_utils from metrics/net/ to metrics/, and move the
channel-checking code from chrome/browser/ to version_info/, so that
WebView can use them.

Split off the part of version_utils that depends on //ui/base, since
most users of version_utils don't need that part, and it pulls in a huge tree of
dependencies.

BUG=711084
==========

to

==========
WebView: Add channel info for UMA

Inspect the WebView provider package name to determine the channel. This
only works for Monochrome (on Android N+); stand-alone WebView uses the
same package across channels, and will still be reported as "unknown".

Move version_utils from metrics/net/ to metrics/, and move the
channel-checking code from chrome/browser/ to version_info/, so that
WebView can use them.

Split off the part of version_utils that depends on //ui/base, since
most users of version_utils don't need that part, and it pulls in a huge
tree of dependencies.

BUG=711084
==========

[paulmiller, 2017-05-30 17:53:59]

The CQ bit was checked by paulmiller@chromium.org

[paulmiller, 2017-05-30 17:54:00]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
asvitkine@chromium.org, rohitrao@chromium.org, torne@chromium.org,
pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2879703002/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-30 17:54:32]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 19:24:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-30 19:24:22]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[paulmiller, 2017-05-30 20:46:49]

The CQ bit was checked by paulmiller@chromium.org

[commit-bot: I haz the power, 2017-05-30 20:47:11]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 20:58:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-30 20:58:18]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[paulmiller, 2017-05-30 21:43:29]

(This is failing in different places, so probably flakes, so I'm just going to
keep mashing the commit button. :P)

[paulmiller, 2017-05-30 21:43:37]

The CQ bit was checked by paulmiller@chromium.org

[commit-bot: I haz the power, 2017-05-30 21:44:22]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 23:25:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-30 23:25:15]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[paulmiller, 2017-05-31 01:07:46]

The CQ bit was checked by paulmiller@chromium.org

[commit-bot: I haz the power, 2017-05-31 01:08:32]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 01:18:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 01:18:52]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[paulmiller, 2017-06-05 21:36:07]

The CQ bit was checked by paulmiller@chromium.org

[commit-bot: I haz the power, 2017-06-05 21:36:58]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-05 23:50:17]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1496698567511510,
"parent_rev": "96caa989b066a9cbd1066aefd30f9b5893739582", "commit_rev":
"234ec23e9f56bb01af5c59922681b114da3aa540"}

[commit-bot: I haz the power, 2017-06-05 23:50:36]

Description was changed from

==========
WebView: Add channel info for UMA

Inspect the WebView provider package name to determine the channel. This
only works for Monochrome (on Android N+); stand-alone WebView uses the
same package across channels, and will still be reported as "unknown".

Move version_utils from metrics/net/ to metrics/, and move the
channel-checking code from chrome/browser/ to version_info/, so that
WebView can use them.

Split off the part of version_utils that depends on //ui/base, since
most users of version_utils don't need that part, and it pulls in a huge
tree of dependencies.

BUG=711084
==========

to

==========
WebView: Add channel info for UMA

Inspect the WebView provider package name to determine the channel. This
only works for Monochrome (on Android N+); stand-alone WebView uses the
same package across channels, and will still be reported as "unknown".

Move version_utils from metrics/net/ to metrics/, and move the
channel-checking code from chrome/browser/ to version_info/, so that
WebView can use them.

Split off the part of version_utils that depends on //ui/base, since
most users of version_utils don't need that part, and it pulls in a huge
tree of dependencies.

BUG=711084

Review-Url: https://codereview.chromium.org/2879703002
Cr-Commit-Position: refs/heads/master@{#477124}
Committed:
https://chromium.googlesource.com/chromium/src/+/234ec23e9f56bb01af5c59922681...
==========

[commit-bot: I haz the power, 2017-06-05 23:50:38]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/234ec23e9f56bb01af5c59922681...

[paulmiller, 2017-06-05 23:56:11]

On 2017/06/05 23:50:38, commit-bot: I haz the power wrote:
> Committed patchset #9 (id:160001) as
>
https://chromium.googlesource.com/chromium/src/+/234ec23e9f56bb01af5c59922681...

Woooo, 5th try! Nailed it.