Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2331093002:
UMA stats for stylus usage (Closed)
|
Created:
4 years, 3 months ago by xiaoyinh(OOO Sep 11-29) Modified:
4 years, 2 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, davemoore+watch_chromium.org, kalyank, oshima+watch_chromium.org, sadrul, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUMA stats for stylus usage
Adding the following uma:
Number of “down” events received by
(1) default note-taking app
(2) web browser
(3) ARC++ app
(4) regular chrome app
(5) Others, everything except browser and apps
Also refactor stylus_metrics_recorder to derive from PointerWatcher
for better practice.
BUG=630070
Committed: https://crrev.com/1e2779e1871c89d04eb03cfe271cc4267be8cadb
Cr-Commit-Position: refs/heads/master@{#422161}
Patch Set 1 #
Total comments: 34
Patch Set 2 : Incorporate comments. #Patch Set 3 : fix enum value in histograms.xml #
Total comments: 23
Patch Set 4 : Incorporate comments from xiyuan@ and mpearson@ #
Total comments: 23
Patch Set 5 : Incorporate comments from sky@ #Patch Set 6 : rebase #
Total comments: 4
Patch Set 7 : Incorporate comments from sky@ and add unit tests #Patch Set 8 : Changed ash/BUILD.gn #
Total comments: 37
Patch Set 9 : Incorporate comments. #Patch Set 10 : rebase #
Total comments: 15
Patch Set 11 : run PointerMetricsRecorderTest in all platforms #
Total comments: 4
Patch Set 12 : Incorporate comments. #Patch Set 13 : nit and rebase #Messages
Total messages: 87 (49 generated)
xiaoyinh@chromium.org changed reviewers: + jdufault@chromium.org, mpearson@chromium.org, sky@chromium.org, xiyuan@chromium.org
On 2016/09/12 18:35:32, xiaoyinh wrote: Can you please indicate which files the reviewers should look at?
https://codereview.chromium.org/2331093002/diff/1/chrome/browser/chromeos/not... File chrome/browser/chromeos/note_taking_app_utils.cc (right): https://codereview.chromium.org/2331093002/diff/1/chrome/browser/chromeos/not... chrome/browser/chromeos/note_taking_app_utils.cc:91: for (const char* id : kExtensionIds) { Line 46-54 above seems suggesting that the app ids could be set via command line. Do we need to consider that as well? https://codereview.chromium.org/2331093002/diff/1/chrome/browser/chromeos/not... File chrome/browser/chromeos/note_taking_app_utils.h (right): https://codereview.chromium.org/2331093002/diff/1/chrome/browser/chromeos/not... chrome/browser/chromeos/note_taking_app_utils.h:30: bool IsCurrentAppNoteTakingApp(extensions::AppWindow* app_window); nit: IsCurrentAppNoteTakingApp -> IsNoteTakingApp CurrentApp sounds redundant. https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... File chrome/browser/ui/ash/metrics/stylus_metrics_recorder.cc (right): https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.cc:120: if (window->GetProperty(aura::client::kStylusWindowTypeBucket) == nit: save the result of window->GetProperty(aura::client::kStylusWindowTypeBucket) in a local var since it is used more than once. https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... File chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h (right): https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h:51: enum StylusWindowType { Are those enums used elsewhere? If not, let's move them into cc file. https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h:73: DownEventDestination GetDownEventDestination(views::Widget* target); RecordUMA, IsEventOnShelf and GetDownEventDestination are impl details and can all be moved into an anonymous namespace in cc file. https://codereview.chromium.org/2331093002/diff/1/ui/aura/client/aura_constan... File ui/aura/client/aura_constants.h (right): https://codereview.chromium.org/2331093002/diff/1/ui/aura/client/aura_constan... ui/aura/client/aura_constants.h:74: AURA_EXPORT extern const aura::WindowProperty<int>* const This does not feel like need to be here. Can we move this somewhere else, e.g. in stylus_metrics_recorder.h ?
On 2016/09/12 20:11:26, sky wrote: > On 2016/09/12 18:35:32, xiaoyinh wrote: > > Can you please indicate which files the reviewers should look at? Thanks for your suggestion. Could you take a look at the following? chrome/browser/ui/ ash/ ui/aura/ xiyuan@ and jdufault@, could you take a look at chrome/browser/chromeos/ mpearson@, could you take a look at tools/metrics/histograms/histograms.xml
On 2016/09/12 20:15:48, xiyuan wrote: > https://codereview.chromium.org/2331093002/diff/1/chrome/browser/chromeos/not... > File chrome/browser/chromeos/note_taking_app_utils.cc (right): > > https://codereview.chromium.org/2331093002/diff/1/chrome/browser/chromeos/not... > chrome/browser/chromeos/note_taking_app_utils.cc:91: for (const char* id : > kExtensionIds) { > Line 46-54 above seems suggesting that the app ids could be set via command > line. Do we need to consider that as well? > > https://codereview.chromium.org/2331093002/diff/1/chrome/browser/chromeos/not... > File chrome/browser/chromeos/note_taking_app_utils.h (right): > > https://codereview.chromium.org/2331093002/diff/1/chrome/browser/chromeos/not... > chrome/browser/chromeos/note_taking_app_utils.h:30: bool > IsCurrentAppNoteTakingApp(extensions::AppWindow* app_window); > nit: IsCurrentAppNoteTakingApp -> IsNoteTakingApp > > CurrentApp sounds redundant. > > https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... > File chrome/browser/ui/ash/metrics/stylus_metrics_recorder.cc (right): > > https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... > chrome/browser/ui/ash/metrics/stylus_metrics_recorder.cc:120: if > (window->GetProperty(aura::client::kStylusWindowTypeBucket) == > nit: save the result of > window->GetProperty(aura::client::kStylusWindowTypeBucket) in a local var since > it is used more than once. > > https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... > File chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h (right): > > https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... > chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h:51: enum > StylusWindowType { > Are those enums used elsewhere? If not, let's move them into cc file. > > https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... > chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h:73: DownEventDestination > GetDownEventDestination(views::Widget* target); > RecordUMA, IsEventOnShelf and GetDownEventDestination are impl details and can > all be moved into an anonymous namespace in cc file. > > https://codereview.chromium.org/2331093002/diff/1/ui/aura/client/aura_constan... > File ui/aura/client/aura_constants.h (right): > > https://codereview.chromium.org/2331093002/diff/1/ui/aura/client/aura_constan... > ui/aura/client/aura_constants.h:74: AURA_EXPORT extern const > aura::WindowProperty<int>* const > This does not feel like need to be here. Can we move this somewhere else, e.g. > in stylus_metrics_recorder.h ? Thanks for your comments! I will incorporate them soon.
Can you better clarify what you mean by system ui? There are a bunch of things that might be considered system ui that you aren't getting. For example, overview mode, or alt-tab mode, or wallpaper picker, file system... https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right): https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:881: stylus_metrics_recorder_.reset(new chromeos::StylusMetricsRecorder()); Use MakeUnique (search for examples in code if not familiar with it). https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:886: #if defined(OS_CHROMEOS) Please document why this needs to be explicitly destroyed. https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h (right): https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h:24: #include "chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h" Only include this if on chromeos (to match your ifdef below). https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... File chrome/browser/ui/ash/metrics/stylus_metrics_recorder.cc (right): https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.cc:80: if (!target) { In general we don't use {} around single line conditionals. https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.cc:85: int container_id = ash::wm::GetContainerForWindow(window)->GetShellWindowId(); Make sure you null check GetContainerForWindow and deal. https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.cc:94: if (container_id == ash::kShellWindowId_MenuContainer) This hits *all* menus. I don't think you want that. https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... File chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h (right): https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h:9: #include "ui/aura/client/aura_constants.h" Can this include be moved to .cc? https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h:24: enum DownEventFormFactor { Please use enum class. https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h:32: enum DownEventSource { Move enums that are only used in the .cc into the .cc.
As sky@ says, it'd be good to clarify the system UI category, as well as others. You may want to hold off on improving the histograms.xml file until after you settle the rest of the code with your reviewers. I'll send these initial comments in case it helps. --mark https://codereview.chromium.org/2331093002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2331093002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:75114: + <int value="0" label="Others"/> Does this include regular Chrome apps? Perhaps that should be a separate category and not "others"? If Chrome apps isn't included here (perhaps in its websites?), then what does this include? You need to be clear. https://codereview.chromium.org/2331093002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:75115: + <int value="1" label="System UI"/> Is this basically everything besides renderers and apps? e.g., ChromeOS menus? https://codereview.chromium.org/2331093002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:75116: + <int value="2" label="Website"/> Do you mean content area of a renderer? https://codereview.chromium.org/2331093002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:75117: + <int value="3" label="ARC++ app"/> Do you mean specifically an Android app on Chrome or any app that's running on an ARC++ device?
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Sorry about the confusion. System UI initially means that everything except browser/apps that has an effect. Eg interacting with shelf, system menu, launcher would all count, but tapping randomly on the desktop wouldn't. Since it could be a lot and will keep increasing, I've confirmed with PM to lump others + system UI together. The categories looks like this now: 1.others: Others, everything except browser and apps 2.browser: down events inside the browser frame. 3.chrome_app: Regular chrome app, except default note-taking app 4.arc++ app: ARC++ app(andriod app on Chrome) 5.default note taking app All the above comments has been incorporated as well. Could you take a look and let me know? Thanks! https://codereview.chromium.org/2331093002/diff/1/chrome/browser/chromeos/not... File chrome/browser/chromeos/note_taking_app_utils.cc (right): https://codereview.chromium.org/2331093002/diff/1/chrome/browser/chromeos/not... chrome/browser/chromeos/note_taking_app_utils.cc:91: for (const char* id : kExtensionIds) { On 2016/09/12 20:15:47, xiyuan wrote: > Line 46-54 above seems suggesting that the app ids could be set via command > line. Do we need to consider that as well? Oh, I missed this case. Thanks for pointing that out. https://codereview.chromium.org/2331093002/diff/1/chrome/browser/chromeos/not... File chrome/browser/chromeos/note_taking_app_utils.h (right): https://codereview.chromium.org/2331093002/diff/1/chrome/browser/chromeos/not... chrome/browser/chromeos/note_taking_app_utils.h:30: bool IsCurrentAppNoteTakingApp(extensions::AppWindow* app_window); On 2016/09/12 20:15:47, xiyuan wrote: > nit: IsCurrentAppNoteTakingApp -> IsNoteTakingApp > > CurrentApp sounds redundant. Done. https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right): https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:881: stylus_metrics_recorder_.reset(new chromeos::StylusMetricsRecorder()); On 2016/09/13 00:03:25, sky wrote: > Use MakeUnique (search for examples in code if not familiar with it). Done. https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:886: #if defined(OS_CHROMEOS) On 2016/09/13 00:03:25, sky wrote: > Please document why this needs to be explicitly destroyed. Done. https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h (right): https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h:24: #include "chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h" On 2016/09/13 00:03:25, sky wrote: > Only include this if on chromeos (to match your ifdef below). Done. https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... File chrome/browser/ui/ash/metrics/stylus_metrics_recorder.cc (right): https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.cc:80: if (!target) { On 2016/09/13 00:03:26, sky wrote: > In general we don't use {} around single line conditionals. Done. https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.cc:85: int container_id = ash::wm::GetContainerForWindow(window)->GetShellWindowId(); On 2016/09/13 00:03:26, sky wrote: > Make sure you null check GetContainerForWindow and deal. Done. https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.cc:94: if (container_id == ash::kShellWindowId_MenuContainer) On 2016/09/13 00:03:26, sky wrote: > This hits *all* menus. I don't think you want that. Deleted. Thanks. https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.cc:120: if (window->GetProperty(aura::client::kStylusWindowTypeBucket) == On 2016/09/12 20:15:47, xiyuan wrote: > nit: save the result of > window->GetProperty(aura::client::kStylusWindowTypeBucket) in a local var since > it is used more than once. Done. https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... File chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h (right): https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h:9: #include "ui/aura/client/aura_constants.h" On 2016/09/13 00:03:26, sky wrote: > Can this include be moved to .cc? I moved the window property key out of aura::client, so this include is removed. Thanks! https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h:24: enum DownEventFormFactor { On 2016/09/13 00:03:26, sky wrote: > Please use enum class. Thanks! I've made StylusWindowType as enum class. Other 3 are used in UMA HISTGRAM. And UMA seems to only accept int as samples. ../chrome/browser/ui/ash/metrics/stylus_metrics_recorder.cc:147:68: error: cannot initialize a parameter of type 'Sample' (aka 'int') with an lvalue of type 'chromeos::DownEventDestination' UMA_HISTOGRAM_ENUMERATION("Event.DownEventCount.PerDestination", dest, https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h:32: enum DownEventSource { On 2016/09/13 00:03:26, sky wrote: > Move enums that are only used in the .cc into the .cc. Done. https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h:51: enum StylusWindowType { On 2016/09/12 20:15:47, xiyuan wrote: > Are those enums used elsewhere? If not, let's move them into cc file. Yes, they are being used when set the window property in chrome_native_app_window_views_aura_ash.cc and browser_frame.cc https://codereview.chromium.org/2331093002/diff/1/chrome/browser/ui/ash/metri... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h:73: DownEventDestination GetDownEventDestination(views::Widget* target); On 2016/09/12 20:15:47, xiyuan wrote: > RecordUMA, IsEventOnShelf and GetDownEventDestination are impl details and can > all be moved into an anonymous namespace in cc file. Done. https://codereview.chromium.org/2331093002/diff/1/ui/aura/client/aura_constan... File ui/aura/client/aura_constants.h (right): https://codereview.chromium.org/2331093002/diff/1/ui/aura/client/aura_constan... ui/aura/client/aura_constants.h:74: AURA_EXPORT extern const aura::WindowProperty<int>* const On 2016/09/12 20:15:47, xiyuan wrote: > This does not feel like need to be here. Can we move this somewhere else, e.g. > in stylus_metrics_recorder.h ? Done.
https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/note_taking_app_utils.cc (right): https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/note_taking_app_utils.cc:62: ids = GetAppIds(); nit: Move the declaration in L58 down here, i.e. std::vector<std::string> ids = GetAppIds(); https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right): https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:888: // required. So explicityly delete it before the shell instance becomes invalid. nit: wrong indent. Comment should use the same indent as code. https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:889: #if defined(OS_CHROMEOS) nit: move this above the comment to keep it closer to code. https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h (right): https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h:24: #include "chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h" forgot to remove? https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h:29: #include "chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h" nit: Since StylusMetricsRecorder is held in a std::unique_ptr, we can forward declare it and move the include to cc file. https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/ash/m... File chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h (right): https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h:20: namespace chromeos { This file is in the ash section in browser/ui/BUILD.gn. If we want to make this chromeos only, maybe we should rename the file to stylus_metrics_recorder_chromeos.h/cc. Otherwise, maybe we should put it in namespace ash. https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc (right): https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:22: #include "chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h" Depending on whether we want to make StylusMetricsRecorder chromeos only, this and the SetProperty call below might need to be put into #if defined(OS_CHROMEOS). https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_frame.cc (right): https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame.cc:16: #include "chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h" ditto. Need to decide whether this is chormeos-only and if yes, need to be wrapped under #if defined(OS_CHROMEOS).
https://codereview.chromium.org/2331093002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2331093002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12795: + <summary>Counts the number of down events received per destination.</summary> nit: omit "Counts" https://codereview.chromium.org/2331093002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12795: + <summary>Counts the number of down events received per destination.</summary> Add clarification that this only emitted for events that are handled / have an effect. (As an example to test the clarify of what you add, consider whether it answer the question: What about tapping on a disabled button?) https://codereview.chromium.org/2331093002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75556: + <int value="3" label="ARC++ app(andriod app on Chrome)"/> nit: space before (
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/note_taking_app_utils.cc (right): https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/note_taking_app_utils.cc:62: ids = GetAppIds(); On 2016/09/16 20:07:53, xiyuan wrote: > nit: Move the declaration in L58 down here, > > i.e. std::vector<std::string> ids = GetAppIds(); Done. https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right): https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:888: // required. So explicityly delete it before the shell instance becomes invalid. On 2016/09/16 20:07:54, xiyuan wrote: > nit: wrong indent. Comment should use the same indent as code. Done. https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:889: #if defined(OS_CHROMEOS) On 2016/09/16 20:07:54, xiyuan wrote: > nit: move this above the comment to keep it closer to code. Done. https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h (right): https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h:24: #include "chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h" On 2016/09/16 20:07:54, xiyuan wrote: > forgot to remove? Oops...Removed. https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h:29: #include "chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h" On 2016/09/16 20:07:54, xiyuan wrote: > nit: Since StylusMetricsRecorder is held in a std::unique_ptr, we can forward > declare it and move the include to cc file. Done. https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/ash/m... File chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h (right): https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h:20: namespace chromeos { On 2016/09/16 20:07:54, xiyuan wrote: > This file is in the ash section in browser/ui/BUILD.gn. If we want to make this > chromeos only, maybe we should rename the file to > stylus_metrics_recorder_chromeos.h/cc. Otherwise, maybe we should put it in > namespace ash. Renamed as stylus_metrics_recorder_chromeos.h/cc. Thanks! https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc (right): https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:22: #include "chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h" On 2016/09/16 20:07:54, xiyuan wrote: > Depending on whether we want to make StylusMetricsRecorder chromeos only, this > and the SetProperty call below might need to be put into #if > defined(OS_CHROMEOS). This file seems to be used in chromeos only, do I still need the if defined(OS_CHROMEOS)? https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_frame.cc (right): https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame.cc:16: #include "chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h" On 2016/09/16 20:07:54, xiyuan wrote: > ditto. Need to decide whether this is chormeos-only and if yes, need to be > wrapped under #if defined(OS_CHROMEOS). Wrapped under OS_CHROMEOS https://codereview.chromium.org/2331093002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2331093002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12795: + <summary>Counts the number of down events received per destination.</summary> On 2016/09/16 20:27:02, Mark P wrote: > nit: omit "Counts" Done. https://codereview.chromium.org/2331093002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12795: + <summary>Counts the number of down events received per destination.</summary> On 2016/09/16 20:27:02, Mark P wrote: > Add clarification that this only emitted for events that are handled / have an > effect. > (As an example to test the clarify of what you add, consider whether it answer > the question: What about tapping on a disabled button?) Actually it will also count events that has no effect. As long as the event target belongs to the category we are interested in. Tapping on a disabled button inside the browser frame will be treated as down events on browser window. I've added this information in the summary. Hope it can make things more clear. Thanks https://codereview.chromium.org/2331093002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75556: + <int value="3" label="ARC++ app(andriod app on Chrome)"/> On 2016/09/16 20:27:02, Mark P wrote: > nit: space before ( Done.
lgtm https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc (right): https://codereview.chromium.org/2331093002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:22: #include "chrome/browser/ui/ash/metrics/stylus_metrics_recorder.h" On 2016/09/16 22:34:25, xiaoyinh wrote: > On 2016/09/16 20:07:54, xiyuan wrote: > > Depending on whether we want to make StylusMetricsRecorder chromeos only, this > > and the SetProperty call below might need to be put into #if > > defined(OS_CHROMEOS). > > This file seems to be used in chromeos only, do I still need the if > defined(OS_CHROMEOS)? Right. This file is chromeos only per c/b/ui/BUILD.gn. So this is okay.
histograms.xml lgtm with a correction below https://codereview.chromium.org/2331093002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2331093002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12797: + targeted to each destination will be counted, include thoes who doesn't have two typos and wrong verb tenses. include thoes who doesn't -> including those that don't
P.S. the changelist description is out-of-date. You may want to revise it.
Description was changed from ========== UMA stats for stylus usage Adding the following uma: Number of “down” events received by (1) default note-taking app (2) website, (3) ARC++ app (4) system UI (5) others Also refactor stylus_metrics_recorder to derive from PointerWatcher for better practice. BUG=630070 ========== to ========== UMA stats for stylus usage Adding the following uma: Number of “down” events received by (1) default note-taking app (2) web browser (3) ARC++ app (4) regular chrome app (5) Others, everything except browser and apps Also refactor stylus_metrics_recorder to derive from PointerWatcher for better practice. BUG=630070 ==========
https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/note_taking_app_utils.cc (right): https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/note_taking_app_utils.cc:60: std::vector<std::string> ids = GetAppIds(); Move into for loop (see comment below). https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/note_taking_app_utils.cc:97: std::vector<std::string> ids = GetAppIds(); move into for loop, e.g. for (const auto& id : GetAppIds()) https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/ash/m... File chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.cc (right): https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.cc:34: enum DownEventFormFactor { enum class, and avoid duplicating the name, e.g. DownEventFormFactor::CLAM_SHELL. https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.cc:62: DownEventDestination dest = DOWN_EVENT_DESTINATION_OTHERS; dest is never changed. The code would be cleaner if you returned DOWN_EVENT_DESTINATION_OTHERS rather than having dest. https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.cc:68: if (!window) This shouldn't be possible. DCHECK? https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.cc:84: else no else (see chromium style guide for details) https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/ash/m... File chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.h (right): https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.h:23: WINDOW_TYPE_OTHERS = 0, StylusWindowType::WINDOW_TYPE is a bit redundant, StylusWindowType::OTHER and StylusWindowType::APP is more succinct. https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.h:33: // An metrics recorder that record stylus related metrics. An->A record->records https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc (right): https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:138: chromeos::StylusWindowType::WINDOW_TYPE_APP); This is also used for panels, are you sure you want those lumped in with apps? https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_frame.cc (right): https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame.cc:106: chromeos::StylusWindowType::WINDOW_TYPE_BROWSER_WINDOW); This also includes things like popups. Are you sure you want them lumped with BROWSER_WINDOWS? Also, move this to BrowserNonClientFrameViewAsh as that is the only place that really cares about it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sky@, thank you for your comments. I'm not familiar with window code and might have made some mistakes. Could you take another look and let me know. https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/note_taking_app_utils.cc (right): https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/note_taking_app_utils.cc:60: std::vector<std::string> ids = GetAppIds(); On 2016/09/16 23:20:16, sky wrote: > Move into for loop (see comment below). Done. https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/note_taking_app_utils.cc:97: std::vector<std::string> ids = GetAppIds(); On 2016/09/16 23:20:16, sky wrote: > move into for loop, e.g. for (const auto& id : GetAppIds()) Done. https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/ash/m... File chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.cc (right): https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.cc:34: enum DownEventFormFactor { On 2016/09/16 23:20:16, sky wrote: > enum class, and avoid duplicating the name, e.g. > DownEventFormFactor::CLAM_SHELL. Done. https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.cc:62: DownEventDestination dest = DOWN_EVENT_DESTINATION_OTHERS; On 2016/09/16 23:20:16, sky wrote: > dest is never changed. The code would be cleaner if you returned > DOWN_EVENT_DESTINATION_OTHERS rather than having dest. Done. https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.cc:68: if (!window) On 2016/09/16 23:20:16, sky wrote: > This shouldn't be possible. DCHECK? Done. https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.cc:84: else On 2016/09/16 23:20:16, sky wrote: > no else (see chromium style guide for details) Done. https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/ash/m... File chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.h (right): https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.h:23: WINDOW_TYPE_OTHERS = 0, On 2016/09/16 23:20:16, sky wrote: > StylusWindowType::WINDOW_TYPE is a bit redundant, StylusWindowType::OTHER and > StylusWindowType::APP is more succinct. Done. https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.h:33: // An metrics recorder that record stylus related metrics. On 2016/09/16 23:20:16, sky wrote: > An->A > record->records Thanks. Changed. https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc (right): https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:138: chromeos::StylusWindowType::WINDOW_TYPE_APP); On 2016/09/16 23:20:17, sky wrote: > This is also used for panels, are you sure you want those lumped in with apps? Thanks for pointing this out. I've move it to ChromeNativeAppWindowViews::InitializeDefaultWindow. https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_frame.cc (right): https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame.cc:106: chromeos::StylusWindowType::WINDOW_TYPE_BROWSER_WINDOW); On 2016/09/16 23:20:17, sky wrote: > This also includes things like popups. Are you sure you want them lumped with > BROWSER_WINDOWS? Also, move this to BrowserNonClientFrameViewAsh as that is the > only place that really cares about it. Moved to BrowserNonClientFrameViewAsh::Init() I feel that popup windows could also be counted as down events on browser window because it happens during the web browsing. Let me know if you think otherwise. If we are going to rule out the popup, could I use widget type TYPE_POPUP to check it? https://codereview.chromium.org/2331093002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2331093002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12797: + targeted to each destination will be counted, include thoes who doesn't have On 2016/09/16 22:51:06, Mark P wrote: > two typos and wrong verb tenses. > include thoes who doesn't > -> > including those that don't > Thanks! Done.
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
derat@chromium.org changed reviewers: + derat@chromium.org
https://codereview.chromium.org/2331093002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_app_utils.cc (right): https://codereview.chromium.org/2331093002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_app_utils.cc:95: for (const auto& id : GetAppIds()) { the changelist description describes us as measuring events in the "default note-taking app", but this looks like it's checking against all of the hardcoded IDs instead of just the one that we'd launch via LaunchNoteTakingAppForNewNote(). is that intentional? if not, it seems like it may be more appropriate to call GetApp() here and compare its ID (if it's non-null) against the window's ID. in particular, once we expose a pref to choose a different app, we'd probably only want to compare against that app here, right? (this all comes down to how this metric is going to be used, though, and the answer to that isn't clear to me from looking at the bug.) https://codereview.chromium.org/2331093002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.cc (right): https://codereview.chromium.org/2331093002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.cc:37: FORMFACTOR_COUNT, nit: FORM_FACTOR_COUNT (to match enum name)
https://codereview.chromium.org/2331093002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_app_utils.cc (right): https://codereview.chromium.org/2331093002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_app_utils.cc:95: for (const auto& id : GetAppIds()) { On 2016/09/19 22:08:47, Daniel Erat wrote: > the changelist description describes us as measuring events in the "default > note-taking app", but this looks like it's checking against all of the hardcoded > IDs instead of just the one that we'd launch via > LaunchNoteTakingAppForNewNote(). is that intentional? if not, it seems like it > may be more appropriate to call GetApp() here and compare its ID (if it's > non-null) against the window's ID. in particular, once we expose a pref to > choose a different app, we'd probably only want to compare against that app > here, right? > > (this all comes down to how this metric is going to be used, though, and the > answer to that isn't clear to me from looking at the bug.) Thank you for your comments. GetApp will return the first installed and enabled app, does that means we will have only one default note taking app in the system? I was a little confused with the list of extension ids and thought they may belong to different versions of the extension.
Your change has lots of similarities with DesktopTaskSwitchMetricRecorder. I'm wondering if we can get rid of DesktopTaskSwitchMetricRecorder or would it make sense to combine? https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_frame.cc (right): https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame.cc:106: chromeos::StylusWindowType::WINDOW_TYPE_BROWSER_WINDOW); On 2016/09/19 21:19:53, xiaoyinh wrote: > On 2016/09/16 23:20:17, sky wrote: > > This also includes things like popups. Are you sure you want them lumped with > > BROWSER_WINDOWS? Also, move this to BrowserNonClientFrameViewAsh as that is > the > > only place that really cares about it. > > Moved to BrowserNonClientFrameViewAsh::Init() > > I feel that popup windows could also be counted as down events on browser window > because it happens during the web browsing. Let me know if you think otherwise. > > If we are going to rule out the popup, could I use widget type TYPE_POPUP to > check it? I'm not familiar with what you're considering 'browser' window. I wanted to make sure you were aware this code is hit for popups too. If you want popups in that too, that's fine. If you want to rule out popups you would check the browser type. In fact the browser type may include apps. See Browser::is_app().
On 2016/09/19 23:36:53, sky wrote: > Your change has lots of similarities with DesktopTaskSwitchMetricRecorder. I'm > wondering if we can get rid of DesktopTaskSwitchMetricRecorder or would it make > sense to combine? As well as TaskSwitchMetricsRecorder. -Scott > > https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/views... > File chrome/browser/ui/views/frame/browser_frame.cc (right): > > https://codereview.chromium.org/2331093002/diff/60001/chrome/browser/ui/views... > chrome/browser/ui/views/frame/browser_frame.cc:106: > chromeos::StylusWindowType::WINDOW_TYPE_BROWSER_WINDOW); > On 2016/09/19 21:19:53, xiaoyinh wrote: > > On 2016/09/16 23:20:17, sky wrote: > > > This also includes things like popups. Are you sure you want them lumped > with > > > BROWSER_WINDOWS? Also, move this to BrowserNonClientFrameViewAsh as that is > > the > > > only place that really cares about it. > > > > Moved to BrowserNonClientFrameViewAsh::Init() > > > > I feel that popup windows could also be counted as down events on browser > window > > because it happens during the web browsing. Let me know if you think > otherwise. > > > > If we are going to rule out the popup, could I use widget type TYPE_POPUP to > > check it? > > I'm not familiar with what you're considering 'browser' window. I wanted to make > sure you were aware this code is hit for popups too. If you want popups in that > too, that's fine. If you want to rule out popups you would check the browser > type. In fact the browser type may include apps. See Browser::is_app().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xiaoyinh@chromium.org changed reviewers: + reveman@chromium.org
sky@, could you take a look at the new patch(patch 7)? reveman@, Could you take a look components/exo/shell_surface.cc Thank you! https://codereview.chromium.org/2331093002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.cc (right): https://codereview.chromium.org/2331093002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/metrics/stylus_metrics_recorder_chromeos.cc:37: FORMFACTOR_COUNT, On 2016/09/19 22:08:47, Daniel Erat wrote: > nit: FORM_FACTOR_COUNT (to match enum name) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/pointer_me... File ash/metrics/pointer_metrics_recorder.cc (right): https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/pointer_me... ash/metrics/pointer_metrics_recorder.cc:24: CLAM_SHELL = 0, nit: "CLAMSHELL" instead of "CLAM_SHELL" (since it's usually written as a single word; see e.g. https://en.wikipedia.org/wiki/Clamshell_(container) ) https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/pointer_me... File ash/metrics/pointer_metrics_recorder_unittest.cc (right): https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/pointer_me... ash/metrics/pointer_metrics_recorder_unittest.cc:83: // Verifies that down events from diffent input are recorded. nit: "different", s/input/inputs/ https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/pointer_me... ash/metrics/pointer_metrics_recorder_unittest.cc:88: const ui::PointerEvent unkown_event( nit: "unknown" https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/pointer_me... ash/metrics/pointer_metrics_recorder_unittest.cc:129: // Verifies that down events in different form factor are recorded. nit: s/factor/factors/ https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/pointer_me... ash/metrics/pointer_metrics_recorder_unittest.cc:153: // Verifies that down events dispatched to different destination are recorded. nit: s/destination/destinations/ https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/user_metri... File ash/metrics/user_metrics_recorder.cc (right): https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/user_metri... ash/metrics/user_metrics_recorder.cc:612: // To clean up stylus_metrics_recorder_ properly, a valid shell instance is nit: s/stylus/pointer/ https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/user_metri... ash/metrics/user_metrics_recorder.cc:613: // required. So explicityly delete it before the shell instance becomes nit: explicitly https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/user_metri... File ash/metrics/user_metrics_recorder.h (right): https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/user_metri... ash/metrics/user_metrics_recorder.h:86: // Metric recorder to track pointer down event. nit: s/event/events/ https://codereview.chromium.org/2331093002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_app_utils.cc (right): https://codereview.chromium.org/2331093002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_app_utils.cc:91: DCHECK(profile); DCHECK(app_window) too? https://codereview.chromium.org/2331093002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_app_utils.h (right): https://codereview.chromium.org/2331093002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_app_utils.h:29: // Returns true if current app window is note taking app window. nit: "Returns true if |app_window| belongs to the default note-taking app." https://codereview.chromium.org/2331093002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_app_utils.h:30: bool IsNoteTakingApp(extensions::AppWindow* app_window, Profile* profile); i'd rename this to IsNoteTakingAppWindow() to make it clearer what it does https://codereview.chromium.org/2331093002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right): https://codereview.chromium.org/2331093002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:150: ash::AppType appType = ash::AppType::CHROME_APP; s/appType/app_type/ https://codereview.chromium.org/2331093002/diff/140001/ui/aura/client/aura_co... File ui/aura/client/aura_constants.cc (right): https://codereview.chromium.org/2331093002/diff/140001/ui/aura/client/aura_co... ui/aura/client/aura_constants.cc:27: DEFINE_WINDOW_PROPERTY_KEY(int, kAppType, 0); can you use the enum here instead of 0? https://codereview.chromium.org/2331093002/diff/140001/ui/aura/client/aura_co... File ui/aura/client/aura_constants.h (right): https://codereview.chromium.org/2331093002/diff/140001/ui/aura/client/aura_co... ui/aura/client/aura_constants.h:30: // pointer metrics. please mention the enum name in this comment so later devs can know what's stored here without hunting down the code that sets it
https://codereview.chromium.org/2331093002/diff/140001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2331093002/diff/140001/components/exo/shell_s... components/exo/shell_surface.cc:949: static_cast<int>(ash::AppType::ARC_APP)); This is incorrect. All shell surfaces are not arc apps. You can use the kApplicationIdKey window property to detect if a window is from an arc app. It's set to org.chromium.arc.* for arc apps. However, this type of detection doesn't belong in exo code. Arc specific code should handle this. Maybe we can add it to components/arc or the arc launcher code.
https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/pointer_me... File ash/metrics/pointer_metrics_recorder.h (right): https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/pointer_me... ash/metrics/pointer_metrics_recorder.h:19: namespace ash { Please move this class to ash/common/metrics. Add a getter to WmWindow for the AppType that uses the property you added. Implement it for WmWindowAura and leave TODOs for WmWindowMus. https://codereview.chromium.org/2331093002/diff/140001/ash/shared/app_types.h File ash/shared/app_types.h (right): https://codereview.chromium.org/2331093002/diff/140001/ash/shared/app_types.h... ash/shared/app_types.h:17: DEFAULT_NOTE_TAKING_APP, Why the DEFAULT_ here instead of NOTE_TAKING_APP? https://codereview.chromium.org/2331093002/diff/140001/ash/shared/app_types.h... ash/shared/app_types.h:18: APP_COUNT, I would prefer you define a constant outside the enum for this. That way anyone wanting to handle a switch statement doesn't have to deal with APP_COUNT.
https://codereview.chromium.org/2331093002/diff/140001/ash/shared/app_types.h File ash/shared/app_types.h (right): https://codereview.chromium.org/2331093002/diff/140001/ash/shared/app_types.h... ash/shared/app_types.h:17: DEFAULT_NOTE_TAKING_APP, On 2016/09/27 22:08:59, sky wrote: > Why the DEFAULT_ here instead of NOTE_TAKING_APP? i'm fine with dropping the DEFAULT_ here. (as background, there will be a setting by which the user can select a note-taking app that will be launched in various places in the UI.)
Ah, ok, in that case DEFAULT_ makes sense. On Tue, Sep 27, 2016 at 4:34 PM, <derat@chromium.org> wrote: > > https://codereview.chromium.org/2331093002/diff/140001/ash/shared/app_types.h > File ash/shared/app_types.h (right): > > https://codereview.chromium.org/2331093002/diff/140001/ash/shared/app_types.h... > ash/shared/app_types.h:17: DEFAULT_NOTE_TAKING_APP, > On 2016/09/27 22:08:59, sky wrote: >> Why the DEFAULT_ here instead of NOTE_TAKING_APP? > > i'm fine with dropping the DEFAULT_ here. (as background, there will be > a setting by which the user can select a note-taking app that will be > launched in various places in the UI.) > > https://codereview.chromium.org/2331093002/ -- 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.
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/pointer_me... File ash/metrics/pointer_metrics_recorder.cc (right): https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/pointer_me... ash/metrics/pointer_metrics_recorder.cc:24: CLAM_SHELL = 0, On 2016/09/27 19:15:10, Daniel Erat wrote: > nit: "CLAMSHELL" instead of "CLAM_SHELL" (since it's usually written as a single > word; see e.g. https://en.wikipedia.org/wiki/Clamshell_(container) ) Done. https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/pointer_me... File ash/metrics/pointer_metrics_recorder.h (right): https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/pointer_me... ash/metrics/pointer_metrics_recorder.h:19: namespace ash { On 2016/09/27 22:08:59, sky wrote: > Please move this class to ash/common/metrics. Add a getter to WmWindow for the > AppType that uses the property you added. Implement it for WmWindowAura and > leave TODOs for WmWindowMus. Done. Also added SetAppType() in WmWindow so that ash/metrics/pointer_metrics_recorder_unittest.cc could use for testing. https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/pointer_me... File ash/metrics/pointer_metrics_recorder_unittest.cc (right): https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/pointer_me... ash/metrics/pointer_metrics_recorder_unittest.cc:83: // Verifies that down events from diffent input are recorded. On 2016/09/27 19:15:10, Daniel Erat wrote: > nit: "different", s/input/inputs/ Done. https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/pointer_me... ash/metrics/pointer_metrics_recorder_unittest.cc:88: const ui::PointerEvent unkown_event( On 2016/09/27 19:15:10, Daniel Erat wrote: > nit: "unknown" Done. https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/pointer_me... ash/metrics/pointer_metrics_recorder_unittest.cc:129: // Verifies that down events in different form factor are recorded. On 2016/09/27 19:15:10, Daniel Erat wrote: > nit: s/factor/factors/ Done. https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/pointer_me... ash/metrics/pointer_metrics_recorder_unittest.cc:153: // Verifies that down events dispatched to different destination are recorded. On 2016/09/27 19:15:10, Daniel Erat wrote: > nit: s/destination/destinations/ Done. https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/user_metri... File ash/metrics/user_metrics_recorder.cc (right): https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/user_metri... ash/metrics/user_metrics_recorder.cc:612: // To clean up stylus_metrics_recorder_ properly, a valid shell instance is On 2016/09/27 19:15:10, Daniel Erat wrote: > nit: s/stylus/pointer/ Done. https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/user_metri... ash/metrics/user_metrics_recorder.cc:613: // required. So explicityly delete it before the shell instance becomes On 2016/09/27 19:15:10, Daniel Erat wrote: > nit: explicitly Done. https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/user_metri... File ash/metrics/user_metrics_recorder.h (right): https://codereview.chromium.org/2331093002/diff/140001/ash/metrics/user_metri... ash/metrics/user_metrics_recorder.h:86: // Metric recorder to track pointer down event. On 2016/09/27 19:15:10, Daniel Erat wrote: > nit: s/event/events/ Done. https://codereview.chromium.org/2331093002/diff/140001/ash/shared/app_types.h File ash/shared/app_types.h (right): https://codereview.chromium.org/2331093002/diff/140001/ash/shared/app_types.h... ash/shared/app_types.h:17: DEFAULT_NOTE_TAKING_APP, On 2016/09/27 23:34:09, Daniel Erat wrote: > On 2016/09/27 22:08:59, sky wrote: > > Why the DEFAULT_ here instead of NOTE_TAKING_APP? > > i'm fine with dropping the DEFAULT_ here. (as background, there will be a > setting by which the user can select a note-taking app that will be launched in > various places in the UI.) Thanks for the explanation. https://codereview.chromium.org/2331093002/diff/140001/ash/shared/app_types.h... ash/shared/app_types.h:18: APP_COUNT, On 2016/09/27 22:08:59, sky wrote: > I would prefer you define a constant outside the enum for this. That way anyone > wanting to handle a switch statement doesn't have to deal with APP_COUNT. Done. https://codereview.chromium.org/2331093002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_app_utils.cc (right): https://codereview.chromium.org/2331093002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_app_utils.cc:91: DCHECK(profile); On 2016/09/27 19:15:10, Daniel Erat wrote: > DCHECK(app_window) too? Done. https://codereview.chromium.org/2331093002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/note_taking_app_utils.h (right): https://codereview.chromium.org/2331093002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_app_utils.h:29: // Returns true if current app window is note taking app window. On 2016/09/27 19:15:10, Daniel Erat wrote: > nit: "Returns true if |app_window| belongs to the default note-taking app." Done. https://codereview.chromium.org/2331093002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/note_taking_app_utils.h:30: bool IsNoteTakingApp(extensions::AppWindow* app_window, Profile* profile); On 2016/09/27 19:15:10, Daniel Erat wrote: > i'd rename this to IsNoteTakingAppWindow() to make it clearer what it does Done. https://codereview.chromium.org/2331093002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right): https://codereview.chromium.org/2331093002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:150: ash::AppType appType = ash::AppType::CHROME_APP; On 2016/09/27 19:15:10, Daniel Erat wrote: > s/appType/app_type/ Done. I've moved this logic to chrome_native_app_window_views_aura_ash.cc which could use ash dependencies. https://codereview.chromium.org/2331093002/diff/140001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2331093002/diff/140001/components/exo/shell_s... components/exo/shell_surface.cc:949: static_cast<int>(ash::AppType::ARC_APP)); On 2016/09/27 20:39:00, reveman wrote: > This is incorrect. All shell surfaces are not arc apps. You can use the > kApplicationIdKey window property to detect if a window is from an arc app. It's > set to org.chromium.arc.* for arc apps. However, this type of detection doesn't > belong in exo code. Arc specific code should handle this. Maybe we can add it to > components/arc or the arc launcher code. Thanks for pointing it out. I have moved it to arc_app_window_launcher_controller.cc https://codereview.chromium.org/2331093002/diff/140001/ui/aura/client/aura_co... File ui/aura/client/aura_constants.cc (right): https://codereview.chromium.org/2331093002/diff/140001/ui/aura/client/aura_co... ui/aura/client/aura_constants.cc:27: DEFINE_WINDOW_PROPERTY_KEY(int, kAppType, 0); On 2016/09/27 19:15:10, Daniel Erat wrote: > can you use the enum here instead of 0? Thanks for the suggestion. I tried including app_types.h here, then static_cast AppType::OTHERS, it seems to be working. But in general I'm not sure if it is a good practice to include ash headers in here. sky@, could you please advise? https://codereview.chromium.org/2331093002/diff/140001/ui/aura/client/aura_co... File ui/aura/client/aura_constants.h (right): https://codereview.chromium.org/2331093002/diff/140001/ui/aura/client/aura_co... ui/aura/client/aura_constants.h:30: // pointer metrics. On 2016/09/27 19:15:10, Daniel Erat wrote: > please mention the enum name in this comment so later devs can know what's > stored here without hunting down the code that sets it Done.
One minor question, otherwise nearly there. https://codereview.chromium.org/2331093002/diff/180001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2331093002/diff/180001/ash/BUILD.gn#newcode1619 ash/BUILD.gn:1619: "common/metrics/pointer_metrics_recorder_unittest.cc", What is chromeos specific about this test? If it really is chromeos specific I would rather you not list it above and instead include it in the chromeos section only (line 1596ish)
On 2016/09/29 21:25:21, sky wrote: > One minor question, otherwise nearly there. > > https://codereview.chromium.org/2331093002/diff/180001/ash/BUILD.gn > File ash/BUILD.gn (right): > > https://codereview.chromium.org/2331093002/diff/180001/ash/BUILD.gn#newcode1619 > ash/BUILD.gn:1619: "common/metrics/pointer_metrics_recorder_unittest.cc", > What is chromeos specific about this test? If it really is chromeos specific I > would rather you not list it above and instead include it in the chromeos > section only (line 1596ish) My understanding is that we only record pointer metrics for chromeos, so the test for that won't be needed other than chromeos. Plus, maximize_mode used in the test is chromeos specific.
On 2016/09/29 21:48:08, xiaoyinh wrote: > On 2016/09/29 21:25:21, sky wrote: > > One minor question, otherwise nearly there. > > > > https://codereview.chromium.org/2331093002/diff/180001/ash/BUILD.gn > > File ash/BUILD.gn (right): > > > > > https://codereview.chromium.org/2331093002/diff/180001/ash/BUILD.gn#newcode1619 > > ash/BUILD.gn:1619: "common/metrics/pointer_metrics_recorder_unittest.cc", > > What is chromeos specific about this test? If it really is chromeos specific I > > would rather you not list it above and instead include it in the chromeos > > section only (line 1596ish) > > My understanding is that we only record pointer metrics for chromeos, so the > test for that won't be needed other than chromeos. > Plus, maximize_mode used in the test is chromeos specific. Maximize mode is compiled on all platforms. It seems to me it's worth running this test everywhere. Does the test work everywhere?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thanks, i mostly just have nits now. feel free to submit based on scott's l-g-t-m after addressing https://codereview.chromium.org/2331093002/diff/180001/ash/common/metrics/poi... File ash/common/metrics/pointer_metrics_recorder.cc (right): https://codereview.chromium.org/2331093002/diff/180001/ash/common/metrics/poi... ash/common/metrics/pointer_metrics_recorder.cc:21: // and should be treated as append-only. in the interest of correctness, maybe change "should be treated as append-only" to "new values should be inserted immediately above FORM_FACTOR_COUNT" (ditto for DownEventSource) https://codereview.chromium.org/2331093002/diff/180001/ash/common/metrics/poi... ash/common/metrics/pointer_metrics_recorder.cc:75: if (!target) when is this case hit? if it's expected, would it be worthwhile to add a NO_WINDOW or similar value so that the different counts match? https://codereview.chromium.org/2331093002/diff/180001/ash/common/metrics/poi... ash/common/metrics/pointer_metrics_recorder.cc:100: if (event.type() != ui::ET_POINTER_DOWN) nit: maybe do: if (event.type() == ui::ET_POINTER_DOWN) RecordUMA(...); ? one fewer line :-P https://codereview.chromium.org/2331093002/diff/180001/ash/common/metrics/poi... File ash/common/metrics/pointer_metrics_recorder_unittest.cc (right): https://codereview.chromium.org/2331093002/diff/180001/ash/common/metrics/poi... ash/common/metrics/pointer_metrics_recorder_unittest.cc:43: std::unique_ptr<PointerMetricsRecorder> pointer_metrics_recorder_; nit: reorder this member and the previous one to match the order in which you initialize them? https://codereview.chromium.org/2331093002/diff/180001/ash/common/metrics/poi... ash/common/metrics/pointer_metrics_recorder_unittest.cc:60: histogram_tester_.reset(); do you actually need to explicitly reset the tester, or does only the recorder need to be destroyed before ash is torn down? https://codereview.chromium.org/2331093002/diff/180001/ash/common/metrics/poi... ash/common/metrics/pointer_metrics_recorder_unittest.cc:164: DCHECK(window); nit: probably fine to do a CHECK here; this is test code https://codereview.chromium.org/2331093002/diff/180001/ash/metrics/user_metri... File ash/metrics/user_metrics_recorder.cc (right): https://codereview.chromium.org/2331093002/diff/180001/ash/metrics/user_metri... ash/metrics/user_metrics_recorder.cc:613: // required. So explicitly delete it before the shell instance becomes total nit: change ". So" to ", so" https://codereview.chromium.org/2331093002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2331093002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:76748: + <int value="3" label="ARC++ app (android app on Chrome)"/> nit: i think we usually use "ARC" instead of "ARC++" in the code
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sky@, PointerMetricsRecorderTest passed in all platforms. See Patch 11. Also incorporated comments from derat@. Thanks a lot for all your comments! https://codereview.chromium.org/2331093002/diff/180001/ash/common/metrics/poi... File ash/common/metrics/pointer_metrics_recorder.cc (right): https://codereview.chromium.org/2331093002/diff/180001/ash/common/metrics/poi... ash/common/metrics/pointer_metrics_recorder.cc:21: // and should be treated as append-only. On 2016/09/29 22:58:46, Daniel Erat wrote: > in the interest of correctness, maybe change "should be treated as append-only" > to "new values should be inserted immediately above FORM_FACTOR_COUNT" (ditto > for DownEventSource) Done. https://codereview.chromium.org/2331093002/diff/180001/ash/common/metrics/poi... ash/common/metrics/pointer_metrics_recorder.cc:75: if (!target) On 2016/09/29 22:58:46, Daniel Erat wrote: > when is this case hit? if it's expected, would it be worthwhile to add a > NO_WINDOW or similar value so that the different counts match? Based on the comments in PointerWatcher, it feels like the target will be null if it is on mus? I'm going to categorize it as OTHERS, let me know if you think otherwise. https://codereview.chromium.org/2331093002/diff/180001/ash/common/metrics/poi... ash/common/metrics/pointer_metrics_recorder.cc:100: if (event.type() != ui::ET_POINTER_DOWN) On 2016/09/29 22:58:46, Daniel Erat wrote: > nit: maybe do: > > if (event.type() == ui::ET_POINTER_DOWN) > RecordUMA(...); > > ? one fewer line :-P Done. https://codereview.chromium.org/2331093002/diff/180001/ash/common/metrics/poi... File ash/common/metrics/pointer_metrics_recorder_unittest.cc (right): https://codereview.chromium.org/2331093002/diff/180001/ash/common/metrics/poi... ash/common/metrics/pointer_metrics_recorder_unittest.cc:43: std::unique_ptr<PointerMetricsRecorder> pointer_metrics_recorder_; On 2016/09/29 22:58:46, Daniel Erat wrote: > nit: reorder this member and the previous one to match the order in which you > initialize them? Done. https://codereview.chromium.org/2331093002/diff/180001/ash/common/metrics/poi... ash/common/metrics/pointer_metrics_recorder_unittest.cc:60: histogram_tester_.reset(); On 2016/09/29 22:58:46, Daniel Erat wrote: > do you actually need to explicitly reset the tester, or does only the recorder > need to be destroyed before ash is torn down? Oh, thanks. Only recorder here needs to be destroyed explicitly. https://codereview.chromium.org/2331093002/diff/180001/ash/common/metrics/poi... ash/common/metrics/pointer_metrics_recorder_unittest.cc:164: DCHECK(window); On 2016/09/29 22:58:46, Daniel Erat wrote: > nit: probably fine to do a CHECK here; this is test code Done. https://codereview.chromium.org/2331093002/diff/180001/ash/metrics/user_metri... File ash/metrics/user_metrics_recorder.cc (right): https://codereview.chromium.org/2331093002/diff/180001/ash/metrics/user_metri... ash/metrics/user_metrics_recorder.cc:613: // required. So explicitly delete it before the shell instance becomes On 2016/09/29 22:58:46, Daniel Erat wrote: > total nit: change ". So" to ", so" Done. https://codereview.chromium.org/2331093002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2331093002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:76748: + <int value="3" label="ARC++ app (android app on Chrome)"/> On 2016/09/29 22:58:46, Daniel Erat wrote: > nit: i think we usually use "ARC" instead of "ARC++" in the code Done.
bruthig@chromium.org changed reviewers: + bruthig@chromium.org
lgtm https://codereview.chromium.org/2331093002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2331093002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:13045: + <owner>xiaoyinh@chromium.org</owner> nit: Might be a good idea to put a PM here as well.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
LGTM
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
https://codereview.chromium.org/2331093002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2331093002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:13045: + <owner>xiaoyinh@chromium.org</owner> On 2016/09/30 00:30:25, bruthig wrote: > nit: Might be a good idea to put a PM here as well. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xiaoyinh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, mpearson@chromium.org, sky@chromium.org, bruthig@chromium.org Link to the patchset: https://codereview.chromium.org/2331093002/#ps240001 (title: "nit and rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== UMA stats for stylus usage Adding the following uma: Number of “down” events received by (1) default note-taking app (2) web browser (3) ARC++ app (4) regular chrome app (5) Others, everything except browser and apps Also refactor stylus_metrics_recorder to derive from PointerWatcher for better practice. BUG=630070 ========== to ========== UMA stats for stylus usage Adding the following uma: Number of “down” events received by (1) default note-taking app (2) web browser (3) ARC++ app (4) regular chrome app (5) Others, everything except browser and apps Also refactor stylus_metrics_recorder to derive from PointerWatcher for better practice. BUG=630070 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== UMA stats for stylus usage Adding the following uma: Number of “down” events received by (1) default note-taking app (2) web browser (3) ARC++ app (4) regular chrome app (5) Others, everything except browser and apps Also refactor stylus_metrics_recorder to derive from PointerWatcher for better practice. BUG=630070 ========== to ========== UMA stats for stylus usage Adding the following uma: Number of “down” events received by (1) default note-taking app (2) web browser (3) ARC++ app (4) regular chrome app (5) Others, everything except browser and apps Also refactor stylus_metrics_recorder to derive from PointerWatcher for better practice. BUG=630070 Committed: https://crrev.com/1e2779e1871c89d04eb03cfe271cc4267be8cadb Cr-Commit-Position: refs/heads/master@{#422161} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/1e2779e1871c89d04eb03cfe271cc4267be8cadb Cr-Commit-Position: refs/heads/master@{#422161} |
