Record how often we end up in Chrome from external navigations.

Record how often we end up in Chrome from external navigations.

Also records the package names of the apps that cause external navigations
and package names for which the user chose Chrome to handle the URL over
the native application.

BUG=622906
Committed: https://crrev.com/aeb136f1c06107603ba4be85ce9873124b96235a
Cr-Commit-Position: refs/heads/master@{#403254}

[Maria, 2016-06-28 21:34:59]

mariakhomenko@chromium.org changed reviewers:
+ dfalcantara@chromium.org, isherman@chromium.org

[Maria, 2016-06-28 21:35:35]

https://codereview.chromium.org/2096073002/diff/40001/tools/metrics/rappor/ra...
File tools/metrics/rappor/rappor.xml (right):

https://codereview.chromium.org/2096073002/diff/40001/tools/metrics/rappor/ra...
tools/metrics/rappor/rappor.xml:141: type="UMA_RAPPOR_TYPE">
@isherman, is this the right type to use here?

[Ilya Sherman, 2016-06-28 21:50:55]

isherman@chromium.org changed reviewers:
+ holte@chromium.org

[Ilya Sherman, 2016-06-28 21:50:55]

+holte: Steve, is it okay to report the same string for multiple metrics?  That
is, does RAPPOR memoize its randomized responses on a global per-string basis,
or on a per-string per-metric basis?

https://codereview.chromium.org/2096073002/diff/40001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2096073002/diff/40001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:8727: <action
name="MobileExternalNavigationReceived">
You mentioned there was a parallel user action already in place.  What is the
name of that action, and would it make sense to name this action in parallel to
it?

https://codereview.chromium.org/2096073002/diff/40001/tools/metrics/rappor/ra...
File tools/metrics/rappor/rappor.xml (right):

https://codereview.chromium.org/2096073002/diff/40001/tools/metrics/rappor/ra...
tools/metrics/rappor/rappor.xml:140: <rappor-metric
name="Android.ExternalNavigationNotChosen"
I wonder whether the "Android" prefix is really necessary.  Perhaps "Intents" or
"ExternalNavigation" would be a better top-level group name for this metric?

https://codereview.chromium.org/2096073002/diff/40001/tools/metrics/rappor/ra...
tools/metrics/rappor/rappor.xml:141: type="UMA_RAPPOR_TYPE">
On 2016/06/28 21:35:35, Maria wrote:
> @isherman, is this the right type to use here?

I think so, yes.  Steve can double-check me on that.

[Maria, 2016-06-28 23:00:26]

https://codereview.chromium.org/2096073002/diff/40001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2096073002/diff/40001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:8727: <action
name="MobileExternalNavigationReceived">
On 2016/06/28 21:50:55, Ilya Sherman wrote:
> You mentioned there was a parallel user action already in place.  What is the
> name of that action, and would it make sense to name this action in parallel
to
> it?

It's actually a histogram called Android.TabNavigationInterceptResult.

https://codereview.chromium.org/2096073002/diff/40001/tools/metrics/rappor/ra...
File tools/metrics/rappor/rappor.xml (right):

https://codereview.chromium.org/2096073002/diff/40001/tools/metrics/rappor/ra...
tools/metrics/rappor/rappor.xml:140: <rappor-metric
name="Android.ExternalNavigationNotChosen"
On 2016/06/28 21:50:55, Ilya Sherman wrote:
> I wonder whether the "Android" prefix is really necessary.  Perhaps "Intents"
or
> "ExternalNavigation" would be a better top-level group name for this metric?

The reason I am using Android is that what the histogram is using for a related
metric. Also I thought we discouraged new prefixes.

[gone, 2016-06-29 16:30:44]

I think your commit message needs to be updated.  It's not only recording how
often Chrome is launched by external Intents, but also what could have handled
the Intent, etc...

lgtm

https://chromiumcodereview.appspot.com/2096073002/diff/40001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java
(right):

https://chromiumcodereview.appspot.com/2096073002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:300:
private void recordChromeIntentPackageHandlers(Intent intent) {
Private, but add a javadoc saying exactly what this records?  Alternatively,
rename this function?  I don't know why "Chrome" is in the title given what's
being recorded.

recordPackageHandlersForIntentDeliveredToChrome()?

https://chromiumcodereview.appspot.com/2096073002/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java
(right):

https://chromiumcodereview.appspot.com/2096073002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java:326:
boolean result;
activityWasLaunched?  result isn't too meaningful without looking up the API
documentation.

https://chromiumcodereview.appspot.com/2096073002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java:351:
if (packages != null) {
early return instead to avoid the extra indentation?

https://chromiumcodereview.appspot.com/2096073002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java:544:
public void maybeSetAppHandlers(Intent intent, List<ResolveInfo> infos) {
maybeRecordAppHandlersInIntent?

[Maria, 2016-06-29 16:54:43]

Description was changed from

==========
Record how often we end up in Chrome from external navigations.

Also records the package names of the packages that could have handled
the intent that ended up in Chrome.

BUG=622906
==========

to

==========
Record how often we end up in Chrome from external navigations.

Also records the package names of the apps that cause external navigations
and package names for which the user chose Chrome to handle the URL over
the native application.

BUG=622906
==========

[Maria, 2016-06-29 17:24:14]

ping for Steven, would appreciate your input.

[Ilya Sherman, 2016-06-29 20:21:16]

https://codereview.chromium.org/2096073002/diff/40001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2096073002/diff/40001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:8727: <action
name="MobileExternalNavigationReceived">
On 2016/06/28 23:00:26, Maria wrote:
> On 2016/06/28 21:50:55, Ilya Sherman wrote:
> > You mentioned there was a parallel user action already in place.  What is
the
> > name of that action, and would it make sense to name this action in parallel
> to
> > it?
> 
> It's actually a histogram called Android.TabNavigationInterceptResult.

Ah.  It's a bit hard to compare user actions and histograms, as they're not
recorded in exactly the same way.  For example, there are differences in terms
of which metrics are recorded in Incognito mode.  I'd recommend recording any
metrics that you wish to compare using the same underlying mechanism. 
Otherwise, you'll need to make various affordances to be able to actually
compare the data.

https://codereview.chromium.org/2096073002/diff/40001/tools/metrics/rappor/ra...
File tools/metrics/rappor/rappor.xml (right):

https://codereview.chromium.org/2096073002/diff/40001/tools/metrics/rappor/ra...
tools/metrics/rappor/rappor.xml:140: <rappor-metric
name="Android.ExternalNavigationNotChosen"
On 2016/06/28 23:00:26, Maria wrote:
> On 2016/06/28 21:50:55, Ilya Sherman wrote:
> > I wonder whether the "Android" prefix is really necessary.  Perhaps
"Intents"
> or
> > "ExternalNavigation" would be a better top-level group name for this metric?
> 
> The reason I am using Android is that what the histogram is using for a
related
> metric. Also I thought we discouraged new prefixes.

Hmm, I guess it's okay if the name is parallel to an existing histogram group
name.  New prefixes are generally discouraged for histograms, which have a
pretty well established set of prefixes.  And, yes, matching across RAPPOR and
other types of metrics is nice =)

[chromium-reviews, 2016-06-29 20:49:17]

The Rappor noise has two parts, one is deterministic and shared across
multiple metrics.  This part is not weakened by being reported by many
metrics.  The other part uses fresh randomness, so having many reports that
you know to be the same value weakens the noise.  So, it's discouraged to
report the same value in overlapping metrics, but less of an issue for more
independent metrics.

On Jun 29, 2016 1:21 PM, <isherman@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2096073002/diff/40001/tools/metrics/actions/a...
> File tools/metrics/actions/actions.xml (right):
>
>
>
https://codereview.chromium.org/2096073002/diff/40001/tools/metrics/actions/a...
> tools/metrics/actions/actions.xml:8727: <action
> name="MobileExternalNavigationReceived">
> On 2016/06/28 23:00:26, Maria wrote:
> > On 2016/06/28 21:50:55, Ilya Sherman wrote:
> > > You mentioned there was a parallel user action already in place.
> What is the
> > > name of that action, and would it make sense to name this action in
> parallel
> > to
> > > it?
> >
> > It's actually a histogram called Android.TabNavigationInterceptResult.
>
> Ah. It's a bit hard to compare user actions and histograms, as they're
> not recorded in exactly the same way. For example, there are
> differences in terms of which metrics are recorded in Incognito mode.
> I'd recommend recording any metrics that you wish to compare using the
> same underlying mechanism. Otherwise, you'll need to make various
> affordances to be able to actually compare the data.
>
>
>
https://codereview.chromium.org/2096073002/diff/40001/tools/metrics/rappor/ra...
> File tools/metrics/rappor/rappor.xml (right):
>
>
>
https://codereview.chromium.org/2096073002/diff/40001/tools/metrics/rappor/ra...
> tools/metrics/rappor/rappor.xml:140: <rappor-metric
> name="Android.ExternalNavigationNotChosen"
> On 2016/06/28 23:00:26, Maria wrote:
> > On 2016/06/28 21:50:55, Ilya Sherman wrote:
> > > I wonder whether the "Android" prefix is really necessary. Perhaps
> "Intents"
> > or
> > > "ExternalNavigation" would be a better top-level group name for this
> metric?
> >
> > The reason I am using Android is that what the histogram is using for
> a related
> > metric. Also I thought we discouraged new prefixes.
>
> Hmm, I guess it's okay if the name is parallel to an existing histogram
> group name. New prefixes are generally discouraged for histograms,
> which have a pretty well established set of prefixes. And, yes,
> matching across RAPPOR and other types of metrics is nice =)
>
> https://codereview.chromium.org/2096073002/
>

-- 
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.

[Steven Holte, 2016-06-29 20:54:40]

For these metrics, you could probably avoid the overlap by the metrics be Chosen
and NotChosen.

[Maria, 2016-06-29 20:58:19]

On 2016/06/29 20:54:40, Steven Holte wrote:
> For these metrics, you could probably avoid the overlap by the metrics be
Chosen
> and NotChosen.

Except that we can't actually record a metric when we are not chosen because the
user is in a different app at that point.

Hmm.

[Ilya Sherman, 2016-06-29 21:39:31]

On 2016/06/29 20:58:19, Maria wrote:
> On 2016/06/29 20:54:40, Steven Holte wrote:
> > For these metrics, you could probably avoid the overlap by the metrics be
> Chosen
> > and NotChosen.
> 
> Except that we can't actually record a metric when we are not chosen because
the
> user is in a different app at that point.
> 
> Hmm.

Hmm.  I guess it's technically possible to increase the noise for each of the
two metrics, so that the overall noise is roughly on par with a single
regularly-noised metric.  I'm not sure how much of an effect that will have on
the analysis, though -- it might make it hard to get much meaningful data.

Sorry that I didn't think of this potential issue until after suggesting this
implementation strategy over on Monorail!

[Maria, 2016-06-29 22:09:46]

Ok, I switched to use two actions as you suggested. (Btw, for my education,
what's the difference in how histograms vs user actions get handled in
incognito? I think we've done these sorts of comparisons all the time on Clank
team, so it'd be great to know what the potential pitfalls are)

Also removed one of the rapport metrics. Going to only record one of them. We'll
have to do with this data for now.

PTAL.

[gone, 2016-06-29 22:12:59]

still lgtm

[Ilya Sherman, 2016-06-29 22:25:30]

Metrics LGTM, thanks.

On 2016/06/29 22:09:46, Maria wrote:
> Ok, I switched to use two actions as you suggested. (Btw, for my education,
> what's the difference in how histograms vs user actions get handled in
> incognito? I think we've done these sorts of comparisons all the time on Clank
> team, so it'd be great to know what the potential pitfalls are)

IIRC, user actions are not recorded in Incognito mode, whereas histograms are:
https://bugs.chromium.org/p/chromium/issues/detail?id=604001

The backend pipelines that process user actions vs. histograms are also somewhat
different, and filter out spammy/noisy data in slightly different ways.  We try
to share most of the code paths, but there are inevitably differences that sneak
in.

https://codereview.chromium.org/2096073002/diff/80001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2096073002/diff/80001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:8761: Records  when a user was given a choice
between using Chrome and an
nit: s/Records  when/Records when

[Maria, 2016-06-30 16:22:36]

The CQ bit was checked by mariakhomenko@chromium.org

[Maria, 2016-06-30 16:22:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2096073002/#ps120001
(title: "Remove extra space")

[commit-bot: I haz the power, 2016-06-30 16:23:00]

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, 2016-06-30 17:33:21]

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

[commit-bot: I haz the power, 2016-06-30 17:33:23]

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...)

[Maria, 2016-06-30 18:56:04]

The CQ bit was checked by mariakhomenko@chromium.org

[Maria, 2016-06-30 18:56:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2096073002/#ps140001
(title: "Add null check for activity info.")

[commit-bot: I haz the power, 2016-06-30 18:56:42]

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, 2016-06-30 19:42:02]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-30 19:42:18]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 19:46:00]

Description was changed from

==========
Record how often we end up in Chrome from external navigations.

Also records the package names of the apps that cause external navigations
and package names for which the user chose Chrome to handle the URL over
the native application.

BUG=622906
==========

to

==========
Record how often we end up in Chrome from external navigations.

Also records the package names of the apps that cause external navigations
and package names for which the user chose Chrome to handle the URL over
the native application.

BUG=622906

Committed: https://crrev.com/aeb136f1c06107603ba4be85ce9873124b96235a
Cr-Commit-Position: refs/heads/master@{#403254}
==========

[commit-bot: I haz the power, 2016-06-30 19:46:01]

Patchset 8 (id:??) landed as
https://crrev.com/aeb136f1c06107603ba4be85ce9873124b96235a
Cr-Commit-Position: refs/heads/master@{#403254}