Add User Actions and adding more details to UMA metrics for overscroll navigation

Add User Actions and adding more details to UMA metrics for overscroll navigation

One of the goals is to get more detailed information on gesture navigation
usage. Another is to run a Finch experiment comparing the current gesture
navigation UI agains the new MD gesture navigation UI implemented in
https://codereview.chromium.org/2656463002/

UMA changes: instead of simply logging navigation direction, we now also log the
source (touch screen vs. trackpad) for each interaction.

User Actions: four new actions added:
 - Overscroll_Navigated.Back
 - Overscroll_Navigated.Forward
 - Overscroll_Cancelled.Back
 - Overscroll_Cancelled.Forward

BUG=668296
Review-Url: https://codereview.chromium.org/2698673006
Cr-Commit-Position: refs/heads/master@{#454342}
Committed: https://chromium.googlesource.com/chromium/src/+/09f0d64a9f882c2957469e72a05d7ad85bc35eff

[mfomitchev, 2017-02-16 23:32:19]

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

[commit-bot: I haz the power, 2017-02-16 23:34:51]

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-02-17 04:16:53]

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

[commit-bot: I haz the power, 2017-02-17 04:16:54]

Dry run: This issue passed the CQ dry run.

[mfomitchev, 2017-02-17 16:00:28]

mfomitchev@chromium.org changed reviewers:
+ mohsen@chromium.org

[mfomitchev, 2017-02-17 16:00:29]

Mohsen, can you PTAL?

[mohsen, 2017-02-21 20:47:58]

https://codereview.chromium.org/2698673006/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/overscroll_controller.cc (right):

https://codereview.chromium.org/2698673006/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/overscroll_controller.cc:371: if (overscroll_mode_
== OVERSCROLL_NONE)
nit: Curly braces for multi-line block.

https://codereview.chromium.org/2698673006/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/overscroll_controller.cc:373: new_mode,
is_touchpad ? OVERSCROLL_TOUCHPAD : OVERSCROLL_TOUCHSCREEN);
Can the new_mode be NONE? If yes, can the source be non-NONE?

https://codereview.chromium.org/2698673006/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/overscroll_controller.h (right):

https://codereview.chromium.org/2698673006/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/overscroll_controller.h:29: enum OverscrollSource
{
I believe it's preferred to use "enum class". Then you can probably remove
OVERSCROLL prefix from values (just NONE, TOUCHPAD, TOUCHSCREEN).

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
File content/browser/web_contents/aura/gesture_nav_simple.cc (right):

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
content/browser/web_contents/aura/gesture_nav_simple.cc:391:
GetOverscrollConfig(OVERSCROLL_CONFIG_HORIZ_THRESHOLD_START_TOUCHSCREEN);
Can you use the new source value to get the correct threshold?

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right):

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
content/browser/web_contents/aura/overscroll_navigation_overlay.cc:66:
UMA_HISTOGRAM_ENUMERATION("Overscroll.Cancelled3",
Is there a "Cancelled2" or you just want to name this similar to "Started3" and
"Navigated3"?

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
File content/browser/web_contents/aura/overscroll_window_animation.h (right):

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
content/browser/web_contents/aura/overscroll_window_animation.h:10: #include
"base/gtest_prod_util.h"
Is this needed?

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
File content/browser/web_contents/aura/uma_navigation_type.h (right):

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
content/browser/web_contents/aura/uma_navigation_type.h:9: enum
UmaNavigationType {
"enum class" here, too.

[mfomitchev, 2017-02-23 02:53:27]

Patchset #2 (id:20001) has been deleted

[mfomitchev, 2017-02-23 02:59:43]

https://codereview.chromium.org/2698673006/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/overscroll_controller.cc (right):

https://codereview.chromium.org/2698673006/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/overscroll_controller.cc:371: if (overscroll_mode_
== OVERSCROLL_NONE)
On 2017/02/21 20:47:58, mohsen wrote:
> nit: Curly braces for multi-line block.

Done.

https://codereview.chromium.org/2698673006/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/overscroll_controller.cc:373: new_mode,
is_touchpad ? OVERSCROLL_TOUCHPAD : OVERSCROLL_TOUCHSCREEN);
On 2017/02/21 20:47:58, mohsen wrote:
> Can the new_mode be NONE? If yes, can the source be non-NONE?

Yes. The contract only enforces that source is non-NONE when mode is non-NONE.
It says nothing about source when mode is NONE. FWIW, I wish we could always
supply valid source, but it is a little tricky and there are some edge cases. It
doesn't seem worth the effort.

https://codereview.chromium.org/2698673006/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/overscroll_controller.h (right):

https://codereview.chromium.org/2698673006/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/overscroll_controller.h:29: enum OverscrollSource
{
On 2017/02/21 20:47:58, mohsen wrote:
> I believe it's preferred to use "enum class". Then you can probably remove
> OVERSCROLL prefix from values (just NONE, TOUCHPAD, TOUCHSCREEN).

Done.

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
File content/browser/web_contents/aura/gesture_nav_simple.cc (right):

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
content/browser/web_contents/aura/gesture_nav_simple.cc:391:
GetOverscrollConfig(OVERSCROLL_CONFIG_HORIZ_THRESHOLD_START_TOUCHSCREEN);
On 2017/02/21 20:47:58, mohsen wrote:
> Can you use the new source value to get the correct threshold?

Done.

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right):

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
content/browser/web_contents/aura/overscroll_navigation_overlay.cc:66:
UMA_HISTOGRAM_ENUMERATION("Overscroll.Cancelled3",
On 2017/02/21 20:47:58, mohsen wrote:
> Is there a "Cancelled2" or you just want to name this similar to "Started3"
and
> "Navigated3"?

Yes - it seems less confusing this way.

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
File content/browser/web_contents/aura/overscroll_window_animation.h (right):

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
content/browser/web_contents/aura/overscroll_window_animation.h:10: #include
"base/gtest_prod_util.h"
On 2017/02/21 20:47:58, mohsen wrote:
> Is this needed?

Done.

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
File content/browser/web_contents/aura/uma_navigation_type.h (right):

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
content/browser/web_contents/aura/uma_navigation_type.h:9: enum
UmaNavigationType {
On 2017/02/21 20:47:58, mohsen wrote:
> "enum class" here, too.

If we use enum class here, we have to use static_cast<int> everywhere in
UMA_HISTOGRAM_ENUMERATIONs. Given that this enum is mostly for
UMA_HISTOGRAM_ENUMERATIONs, it seems it's better to use old-style enums with
implicit conversion here. Otherwise the code readability suffers.

[mfomitchev, 2017-02-23 02:59:50]

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

[commit-bot: I haz the power, 2017-02-23 03:00:26]

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-02-23 04:26:51]

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

[commit-bot: I haz the power, 2017-02-23 04:26:52]

Dry run: This issue passed the CQ dry run.

[mohsen, 2017-02-23 19:19:10]

Thanks. LGTM

https://codereview.chromium.org/2698673006/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/overscroll_controller.cc (right):

https://codereview.chromium.org/2698673006/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/overscroll_controller.cc:373: new_mode,
is_touchpad ? OVERSCROLL_TOUCHPAD : OVERSCROLL_TOUCHSCREEN);
On 2017/02/23 02:59:42, mfomitchev wrote:
> On 2017/02/21 20:47:58, mohsen wrote:
> > Can the new_mode be NONE? If yes, can the source be non-NONE?
> 
> Yes. The contract only enforces that source is non-NONE when mode is non-NONE.
> It says nothing about source when mode is NONE. FWIW, I wish we could always
> supply valid source, but it is a little tricky and there are some edge cases.
It
> doesn't seem worth the effort.

Acknowledged.

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right):

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
content/browser/web_contents/aura/overscroll_navigation_overlay.cc:66:
UMA_HISTOGRAM_ENUMERATION("Overscroll.Cancelled3",
On 2017/02/23 02:59:42, mfomitchev wrote:
> On 2017/02/21 20:47:58, mohsen wrote:
> > Is there a "Cancelled2" or you just want to name this similar to "Started3"
> and
> > "Navigated3"?
> 
> Yes - it seems less confusing this way.

Acknowledged.

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
File content/browser/web_contents/aura/uma_navigation_type.h (right):

https://codereview.chromium.org/2698673006/diff/1/content/browser/web_content...
content/browser/web_contents/aura/uma_navigation_type.h:9: enum
UmaNavigationType {
On 2017/02/23 02:59:42, mfomitchev wrote:
> On 2017/02/21 20:47:58, mohsen wrote:
> > "enum class" here, too.
> 
> If we use enum class here, we have to use static_cast<int> everywhere in
> UMA_HISTOGRAM_ENUMERATIONs. Given that this enum is mostly for
> UMA_HISTOGRAM_ENUMERATIONs, it seems it's better to use old-style enums with
> implicit conversion here. Otherwise the code readability suffers.

Acknowledged.

https://codereview.chromium.org/2698673006/diff/60001/content/browser/web_con...
File content/browser/web_contents/aura/gesture_nav_simple.cc (right):

https://codereview.chromium.org/2698673006/diff/60001/content/browser/web_con...
content/browser/web_contents/aura/gesture_nav_simple.cc:394:
OVERSCROLL_CONFIG_HORIZ_THRESHOLD_START_TOUCHSCREEN);
nit: I would put conditional ternary operator inside GetOverscrollConfig() to
avoid typing GetOverscrollConfig() twice!

[mfomitchev, 2017-02-24 22:43:36]

mfomitchev@chromium.org changed reviewers:
+ asvitkine@chromium.org, sadrul@chromium.org

[mfomitchev, 2017-02-24 22:43:36]

asvitkine@chromium.org: Please review changes in 
tools/metrics

sadrul@chromium.org: Please review changes in 
content/browser

[mfomitchev, 2017-02-24 22:45:02]

mfomitchev@chromium.org changed reviewers:
+ holte@google.com
- asvitkine@chromium.org

[mfomitchev, 2017-02-24 22:45:03]

-asvitkine since he has marked himself slow to review

+holte for changes in tools/metrics

[mfomitchev, 2017-02-24 22:50:17]

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

[commit-bot: I haz the power, 2017-02-24 22:51:12]

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-02-25 00:18:30]

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

[commit-bot: I haz the power, 2017-02-25 00:18:31]

Dry run: This issue passed the CQ dry run.

[Steven Holte, 2017-02-25 01:33:27]

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

[Steven Holte, 2017-02-25 01:33:27]

https://codereview.chromium.org/2698673006/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2698673006/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:43409: -<histogram
name="Overscroll.Navigated" enum="OverscrollMode">
Don't delete the old histograms.

[mfomitchev, 2017-02-25 03:40:12]

https://codereview.chromium.org/2698673006/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2698673006/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:43409: -<histogram
name="Overscroll.Navigated" enum="OverscrollMode">
On 2017/02/25 01:33:27, Steven Holte wrote:
> Don't delete the old histograms.

Did you note that the histograms I am removing have been deprecated since Chrome
44? It's been almost two years, I thought it would be okay to get rid of them? I
guess it may be best to do it in a different CL either way, especially because
the diff looks confusing.

[Steven Holte, 2017-02-27 20:19:18]

https://codereview.chromium.org/2698673006/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2698673006/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:43409: -<histogram
name="Overscroll.Navigated" enum="OverscrollMode">
On 2017/02/25 03:40:11, mfomitchev wrote:
> On 2017/02/25 01:33:27, Steven Holte wrote:
> > Don't delete the old histograms.
> 
> Did you note that the histograms I am removing have been deprecated since
Chrome
> 44? It's been almost two years, I thought it would be okay to get rid of them?
I
> guess it may be best to do it in a different CL either way, especially because
> the diff looks confusing.

I don't think we have any guidelines for when obsolete histograms are safe to
remove, but once we finally add some better handling for them we'll probably
just be removing all of them from this file, so I think it's simplest just to
leave them in for now.

[mfomitchev, 2017-02-27 20:28:57]

On 2017/02/27 20:19:18, Steven Holte wrote:
>
https://codereview.chromium.org/2698673006/diff/80001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (left):
> 
>
https://codereview.chromium.org/2698673006/diff/80001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:43409: -<histogram
> name="Overscroll.Navigated" enum="OverscrollMode">
> On 2017/02/25 03:40:11, mfomitchev wrote:
> > On 2017/02/25 01:33:27, Steven Holte wrote:
> > > Don't delete the old histograms.
> > 
> > Did you note that the histograms I am removing have been deprecated since
> Chrome
> > 44? It's been almost two years, I thought it would be okay to get rid of
them?
> I
> > guess it may be best to do it in a different CL either way, especially
because
> > the diff looks confusing.
> 
> I don't think we have any guidelines for when obsolete histograms are safe to
> remove, but once we finally add some better handling for them we'll probably
> just be removing all of them from this file, so I think it's simplest just to
> leave them in for now.

Ok, SG, thanks.
The latest patch doesn't remove any of the histograms. Can you please take
another look?

[Steven Holte, 2017-02-27 23:21:21]

thanks, lgtm

[sadrul, 2017-03-01 21:18:35]

lgtm

https://codereview.chromium.org/2698673006/diff/100001/content/browser/render...
File content/browser/renderer_host/overscroll_controller.cc (right):

https://codereview.chromium.org/2698673006/diff/100001/content/browser/render...
content/browser/renderer_host/overscroll_controller.cc:419: if (overscroll_mode_
== mode)
Can this DCHECK that when mode == NONE, source is also NONE?

[mfomitchev, 2017-03-01 23:45:28]

https://codereview.chromium.org/2698673006/diff/100001/content/browser/render...
File content/browser/renderer_host/overscroll_controller.cc (right):

https://codereview.chromium.org/2698673006/diff/100001/content/browser/render...
content/browser/renderer_host/overscroll_controller.cc:419: if (overscroll_mode_
== mode)
On 2017/03/01 21:18:35, sadrul wrote:
> Can this DCHECK that when mode == NONE, source is also NONE?

I've added the DCHECK after the if. It is possible that NONE mode will be set
with non-NONE source from ProcessOverscroll, but only if the previous mode is
also NONE.

[mfomitchev, 2017-03-01 23:46:39]

+creis@ for content/browser/BUILD.gn

[mfomitchev, 2017-03-01 23:47:13]

mfomitchev@chromium.org changed reviewers:
+ creis@chromium.org

[mfomitchev, 2017-03-01 23:47:14]

+creis@ for content/browser/BUILD.gn

[mfomitchev, 2017-03-01 23:47:50]

Description was changed from

==========
Add User Actions and adding more details to UMA metrics for overscroll
navigation

One of the goals is to get more detailed information on gesture navigation
usage. Another is to run a Finch experiment comparing the current gesture
navigation UI agains the new MD gesture navigation UI implemented in
https://codereview.chromium.org/2656463002/

UMA changes: instead of simply logging navigation direction, we now also log the
source (touch screen vs. trackpad) for each interaction.

User Actions: four new actions added:
 - Overscroll_Navigated.Back
 - Overscroll_Navigated.Forward
 - Overscroll_Cancelled.Back
 - Overscroll_Cancelled.Forward

BUG=668296
==========

to

==========
Add User Actions and adding more details to UMA metrics for overscroll
navigation

One of the goals is to get more detailed information on gesture navigation
usage. Another is to run a Finch experiment comparing the current gesture
navigation UI agains the new MD gesture navigation UI implemented in
https://codereview.chromium.org/2656463002/

UMA changes: instead of simply logging navigation direction, we now also log the
source (touch screen vs. trackpad) for each interaction.

User Actions: four new actions added:
 - Overscroll_Navigated.Back
 - Overscroll_Navigated.Forward
 - Overscroll_Cancelled.Back
 - Overscroll_Cancelled.Forward

BUG=668296
==========

[mfomitchev, 2017-03-01 23:47:50]

mfomitchev@chromium.org changed reviewers:
- creis@chromium.org

[mfomitchev, 2017-03-01 23:50:43]

mfomitchev@chromium.org changed reviewers:
+ dgozman@chromium.org

[mfomitchev, 2017-03-01 23:50:44]

-creis as he has marked himself slow
+dgozman@ for content/browser/BUILD.gn

[dgozman, 2017-03-02 00:01:46]

content/browser/BUILD.gn lgtm

[mfomitchev, 2017-03-02 03:34:28]

The CQ bit was checked by mfomitchev@chromium.org

[mfomitchev, 2017-03-02 03:34:28]

The patchset sent to the CQ was uploaded after l-g-t-m from mohsen@chromium.org,
sadrul@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2698673006/#ps140001
(title: "Trailing period")

[commit-bot: I haz the power, 2017-03-02 03:35:59]

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-03-02 07:27:09]

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

[commit-bot: I haz the power, 2017-03-02 07:27:11]

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

[mfomitchev, 2017-03-02 18:52:04]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2017-03-02 18:52:33]

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-03-02 19:40:12]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488480724060500,
"parent_rev": "30ee1086df7c132ecafae15534946181b03b315c", "commit_rev":
"09f0d64a9f882c2957469e72a05d7ad85bc35eff"}

[commit-bot: I haz the power, 2017-03-02 19:40:49]

Description was changed from

==========
Add User Actions and adding more details to UMA metrics for overscroll
navigation

One of the goals is to get more detailed information on gesture navigation
usage. Another is to run a Finch experiment comparing the current gesture
navigation UI agains the new MD gesture navigation UI implemented in
https://codereview.chromium.org/2656463002/

UMA changes: instead of simply logging navigation direction, we now also log the
source (touch screen vs. trackpad) for each interaction.

User Actions: four new actions added:
 - Overscroll_Navigated.Back
 - Overscroll_Navigated.Forward
 - Overscroll_Cancelled.Back
 - Overscroll_Cancelled.Forward

BUG=668296
==========

to

==========
Add User Actions and adding more details to UMA metrics for overscroll
navigation

One of the goals is to get more detailed information on gesture navigation
usage. Another is to run a Finch experiment comparing the current gesture
navigation UI agains the new MD gesture navigation UI implemented in
https://codereview.chromium.org/2656463002/

UMA changes: instead of simply logging navigation direction, we now also log the
source (touch screen vs. trackpad) for each interaction.

User Actions: four new actions added:
 - Overscroll_Navigated.Back
 - Overscroll_Navigated.Forward
 - Overscroll_Cancelled.Back
 - Overscroll_Cancelled.Forward

BUG=668296

Review-Url: https://codereview.chromium.org/2698673006
Cr-Commit-Position: refs/heads/master@{#454342}
Committed:
https://chromium.googlesource.com/chromium/src/+/09f0d64a9f882c2957469e72a05d...
==========

[commit-bot: I haz the power, 2017-03-02 19:40:51]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/09f0d64a9f882c2957469e72a05d...