New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 826476 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jul 19
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocking:
issue 678705



Sign in to add a comment

mash: Remove ash/touch/touch_uma.h usage in chrome/browser/ui/views/touch_uma

Project Member Reported by jamescook@chromium.org, Mar 27 2018

Issue description

ash/metrics/gesture_action_type.h could move to //ash/public/cpp, then the histogram calls could be inlined in chrome.

 
Labels: -Proj-Mustash-Mash
Owner: rcui@chromium.org
Status: Assigned (was: Untriaged)
Here's a relatively simple first bug. The goal is to eliminate access into //ash from code in //chrome. Both sides can record their own UMA metrics via UMA_HISTOGRAM_ENUMERATION().

Ideally we would separate out browser metrics from ash metrics into separate values. If we can't do that, and we just need to share some constants, the constants can go into a header in //ash/public/cpp. (This works because today the ash process and chrome process are the same binary, hence built at same revision, hence the constants are always the same in both processes. Also, UMA values are not allowed to change.)

Alternately, if you can figure out that no one uses these metrics, you could delete them.

Comment 3 by rcui@chromium.org, May 19 2018

Status: Started (was: Assigned)
Will take a look, thanks

Comment 4 by rcui@chromium.org, Jun 8 2018

Even after moving the header file to //ash/public/cpp it seems weird to me that chrome would be recording metrics for ash.  In this case the metric is "Ash.GestureTarget", and will be sent at https://cs.chromium.org/chromium/src/chrome/browser/ui/views/touch_uma/touch_uma_ash.cc?rcl=0a6839983e0efa1d846dc99d5291e59553b5abff&l=29 once the call is inlined.  The touch handler for views seems very chromeos-specific - the generic touch_uma.cc implementation does nothing.  Will dig deeper and see if we really do need tab gesture counts in Ash.GestureTarget.
Yeah, maybe it could be split into two separate metrics, one for ash touch things, and one for chrome touch things.

Or maybe one set of metrics could just be eliminated.

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/50441c1a60811e371a208b3c3841172655fbcf03

commit 50441c1a60811e371a208b3c3841172655fbcf03
Author: Ryan Cui <rcui@chromium.org>
Date: Fri Jun 29 04:26:41 2018

Touch metric refactoring

Adds new Event.Touch.GestureTarget histogram for browser UI elements,
and stops including browser events in Ash.GestureTarget.

Removes header and runtime dependency between browser and ash for
gesture metric handling.

BUG:  826476 
Change-Id: I87e8de2c4cae6f5e30939726917f5f811cb61ba8
Reviewed-on: https://chromium-review.googlesource.com/1116250
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Ryan Cui <rcui@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571387}
[modify] https://crrev.com/50441c1a60811e371a208b3c3841172655fbcf03/ash/touch/touch_uma.cc
[modify] https://crrev.com/50441c1a60811e371a208b3c3841172655fbcf03/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/50441c1a60811e371a208b3c3841172655fbcf03/chrome/browser/ui/views/frame/browser_root_view.cc
[modify] https://crrev.com/50441c1a60811e371a208b3c3841172655fbcf03/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/50441c1a60811e371a208b3c3841172655fbcf03/chrome/browser/ui/views/tabs/tab_strip.cc
[modify] https://crrev.com/50441c1a60811e371a208b3c3841172655fbcf03/chrome/browser/ui/views/touch_uma/touch_uma.cc
[modify] https://crrev.com/50441c1a60811e371a208b3c3841172655fbcf03/chrome/browser/ui/views/touch_uma/touch_uma.h
[delete] https://crrev.com/3ac19b43a9c276b0169dd263f7cd33f42785c21b/chrome/browser/ui/views/touch_uma/touch_uma_ash.cc
[modify] https://crrev.com/50441c1a60811e371a208b3c3841172655fbcf03/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/50441c1a60811e371a208b3c3841172655fbcf03/tools/metrics/histograms/histograms.xml

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/73ad4656d2b1104e08e8f97cf680a07070ec98bb

commit 73ad4656d2b1104e08e8f97cf680a07070ec98bb
Author: Ryan Cui <rcui@chromium.org>
Date: Fri Jul 13 04:00:01 2018

Deprecate unused touch metrics

Bug:  826476 
Change-Id: Id24d14cd5dc0ac31fac4e865f71047aa75c8a786
Reviewed-on: https://chromium-review.googlesource.com/1130819
Commit-Queue: Ryan Cui <rcui@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574837}
[modify] https://crrev.com/73ad4656d2b1104e08e8f97cf680a07070ec98bb/ash/touch/touch_uma.cc
[modify] https://crrev.com/73ad4656d2b1104e08e8f97cf680a07070ec98bb/tools/metrics/histograms/histograms.xml

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eb5b84e304cd9a05b8595774e287c0c38769807b

commit eb5b84e304cd9a05b8595774e287c0c38769807b
Author: Ryan Cui <rcui@chromium.org>
Date: Thu Jul 19 01:36:02 2018

Enable pretarget handlers for touch events in mash

Bug:  826476 
Change-Id: I6b30a7b5c4c252896d557db14e14a54ad4d36ee5
Reviewed-on: https://chromium-review.googlesource.com/1130827
Commit-Queue: Ryan Cui <rcui@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576305}
[modify] https://crrev.com/eb5b84e304cd9a05b8595774e287c0c38769807b/ash/shell.cc

Status: Fixed (was: Started)

Sign in to add a comment