|
|
DescriptionMetrics logging for Sad Tab Reload and Feedback buttons.
BUG=719781
Review-Url: https://codereview.chromium.org/2869223002
Cr-Commit-Position: refs/heads/master@{#478431}
Committed: https://chromium.googlesource.com/chromium/src/+/1b59498f081885d9f82539f478653669b2c5c5a6
Patch Set 1 #
Total comments: 3
Patch Set 2 : Using shared file #
Total comments: 2
Patch Set 3 : Migrating Desktop to use shared symbols, explicitly setting enum values" #
Total comments: 1
Patch Set 4 : Experimental change to compile metrics types for Java #Patch Set 5 : Attempt to get Java generating sad tab enum #Patch Set 6 : Attempt 2 to make Java enum work #
Total comments: 1
Patch Set 7 : Tweak to DEPS file for ui_metrics #Patch Set 8 : Adding to DEPS file for Desktop build #Patch Set 9 : Fix Mac build #
Messages
Total messages: 78 (47 generated)
peterlaurens@chromium.org changed reviewers: + eugenebut@chromium.org
This is a change that will add logging for user engagement of the new reload and feedback buttons on the Sad Tab. It matches the implementation used on desktop. One question about the best place to put shared enums/keys is inline. Thanks! - PL https://codereview.chromium.org/2869223002/diff/1/ios/chrome/browser/ui/sad_t... File ios/chrome/browser/ui/sad_tab/sad_tab_view.mm (right): https://codereview.chromium.org/2869223002/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:123: typedef NS_ENUM(NSInteger, SadTabViewEvent) { This enumeration copies one reporting this metric for Desktop in sad_tab.cc. I'd like to make both implementations use the same enumeration if possible. If you agree, what would be the way to do this?: a. File a bug for Desktop team to make their enum (and keys) public. Then adopt? b. Place our own C++ enum somewhere accessible to both platforms and file a bug for Desktop to adopt? If so, where? c. Something else. Thanks!
Could you please update CL description to have one line summary. https://codereview.chromium.org/2869223002/diff/1/ios/chrome/browser/ui/sad_t... File ios/chrome/browser/ui/sad_tab/sad_tab_view.mm (right): https://codereview.chromium.org/2869223002/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:123: typedef NS_ENUM(NSInteger, SadTabViewEvent) { On 2017/05/09 22:38:23, PL wrote: > This enumeration copies one reporting this metric for Desktop in sad_tab.cc. > I'd like to make both implementations use the same enumeration if possible. > If you agree, what would be the way to do this?: > a. File a bug for Desktop team to make their enum (and keys) public. Then adopt? > b. Place our own C++ enum somewhere accessible to both platforms and file a bug > for Desktop to adopt? If so, where? > c. Something else. > > Thanks! I think it is C :). We should find a common place for this enum (perhaps in components layer) and make changes for Desktop as well. isherman@ may help you with finding a good place for the enum.
Description was changed from ========== Metrics logging for user engagement of the Reload and Feedback buttons in Sad Tab. BUG=719781 ========== to ========== Metrics logging for Sad Tab Reload and Feedback buttons. BUG=719781 ==========
https://codereview.chromium.org/2869223002/diff/1/ios/chrome/browser/ui/sad_t... File ios/chrome/browser/ui/sad_tab/sad_tab_view.mm (right): https://codereview.chromium.org/2869223002/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:123: typedef NS_ENUM(NSInteger, SadTabViewEvent) { On 2017/05/09 22:45:58, Eugene But wrote: > On 2017/05/09 22:38:23, PL wrote: > > This enumeration copies one reporting this metric for Desktop in sad_tab.cc. > > I'd like to make both implementations use the same enumeration if possible. > > If you agree, what would be the way to do this?: > > a. File a bug for Desktop team to make their enum (and keys) public. Then > adopt? > > b. Place our own C++ enum somewhere accessible to both platforms and file a > bug > > for Desktop to adopt? If so, where? > > c. Something else. > > > > Thanks! > I think it is C :). We should find a common place for this enum (perhaps in > components layer) and make changes for Desktop as well. isherman@ may help you > with finding a good place for the enum. Thanks! Reached out to isherman@ and sdy@ for discussion.
On 2017/05/09 22:57:20, PL wrote: > https://codereview.chromium.org/2869223002/diff/1/ios/chrome/browser/ui/sad_t... > File ios/chrome/browser/ui/sad_tab/sad_tab_view.mm (right): > > https://codereview.chromium.org/2869223002/diff/1/ios/chrome/browser/ui/sad_t... > ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:123: typedef NS_ENUM(NSInteger, > SadTabViewEvent) { > On 2017/05/09 22:45:58, Eugene But wrote: > > On 2017/05/09 22:38:23, PL wrote: > > > This enumeration copies one reporting this metric for Desktop in sad_tab.cc. > > > I'd like to make both implementations use the same enumeration if possible. > > > If you agree, what would be the way to do this?: > > > a. File a bug for Desktop team to make their enum (and keys) public. Then > > adopt? > > > b. Place our own C++ enum somewhere accessible to both platforms and file a > > bug > > > for Desktop to adopt? If so, where? > > > c. Something else. > > > > > > Thanks! > > I think it is C :). We should find a common place for this enum (perhaps in > > components layer) and make changes for Desktop as well. isherman@ may help you > > with finding a good place for the enum. > > Thanks! Reached out to isherman@ and sdy@ for discussion. I suspect that the best place for this enum is a file in ui/base/ — see ui/base/page_transition_types.h for something similar. Let me know what you think!
Thanks Eugene, Sidney! This change now uses a shared header file, which should be adoptable by Desktop? Let me know how this looks to you in terms of naming of the shared file, location, and implementation (especially I wasn't sure on whether to use the ui namespace for the header?). PTAL! Thanks! - PL
+Nico for ui OWNERS Peter, do you want to make changes for Mac as well to demonstrate your intentions? https://codereview.chromium.org/2869223002/diff/20001/ui/base/ui_metrics_types.h File ui/base/ui_metrics_types.h (right): https://codereview.chromium.org/2869223002/diff/20001/ui/base/ui_metrics_type... ui/base/ui_metrics_types.h:14: SAD_TAB_EVENT_BUTTON_CLICKED, nit: It is better to initialize every enum item, because they are meant to be stable and always correctly map to xml
On 2017/05/16 23:49:33, Eugene But wrote: > +Nico for ui OWNERS > > Peter, do you want to make changes for Mac as well to demonstrate your > intentions? +1: I think that changing it in both places is a good idea. If you don't have a Mac build set up, you can upload a patch and use the trybots to make sure it compiles and passes tests (like I do when I make changes that touch non-Mac code). > > https://codereview.chromium.org/2869223002/diff/20001/ui/base/ui_metrics_types.h > File ui/base/ui_metrics_types.h (right): > > https://codereview.chromium.org/2869223002/diff/20001/ui/base/ui_metrics_type... > ui/base/ui_metrics_types.h:14: SAD_TAB_EVENT_BUTTON_CLICKED, > nit: It is better to initialize every enum item, because they are meant to be > stable and always correctly map to xml
Thanks! I will see if I can get this Mac version building here, before opening this up to review again. https://codereview.chromium.org/2869223002/diff/20001/ui/base/ui_metrics_types.h File ui/base/ui_metrics_types.h (right): https://codereview.chromium.org/2869223002/diff/20001/ui/base/ui_metrics_type... ui/base/ui_metrics_types.h:14: SAD_TAB_EVENT_BUTTON_CLICKED, On 2017/05/16 23:49:33, Eugene But wrote: > nit: It is better to initialize every enum item, because they are meant to be > stable and always correctly map to xml Done!
The CQ bit was checked by peterlaurens@chromium.org
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by peterlaurens@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2869223002/diff/40001/ui/base/ui_metrics_types.h File ui/base/ui_metrics_types.h (right): https://codereview.chromium.org/2869223002/diff/40001/ui/base/ui_metrics_type... ui/base/ui_metrics_types.h:12: SAD_TAB_EVENT_DISPLAYED = 0, Is it necessary to prefix these with SAD_TAB_EVENT_ since they're in a scoped enum? Maybe there's a reason, just checking.
By the way, just in case you didn't know, there's a "dry run" button here that lets you run a commit against all the bots without actually trying to commit it :).
The CQ bit was checked by peterlaurens@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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by peterlaurens@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by peterlaurens@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by peterlaurens@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: This issue passed the CQ dry run.
Thanks for your patience Sidney, Eugene! I think this now generates the correct Java enums for Android and all tryjobs are passing. PTAL! Thanks, - Peter
lgtm
lgtm
The CQ bit was checked by peterlaurens@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/05/26 18:52:16, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Missing LGTM from an OWNER for these files: chrome/BUILD.gn chrome/browser/ui/sad_tab.cc ui/base/ui_metrics_types.h Will add reviewers.
peterlaurens@chromium.org changed reviewers: + sky@chromium.org
+ Sky@, Hello! Sky@ could you review the use of these files, as they need an owner's approval, thank you for your help! chrome/BUILD.gn chrome/browser/ui/sad_tab.cc ui/base/ui_metrics_types.h (... the last one has a strange owner's file which I'm not sure I understand, it doesn't appear to have owners listed in the normal fashion). Thank you! - PL
https://codereview.chromium.org/2869223002/diff/100001/ui/base/ui_metrics_typ... File ui/base/ui_metrics_types.h (right): https://codereview.chromium.org/2869223002/diff/100001/ui/base/ui_metrics_typ... ui/base/ui_metrics_types.h:8: namespace ui { Sorry for the bike shed. The ui directory is intended for generic code used in building uis. From the readme: "This directory contains UI frameworks used to build various user interface features. This directory it not intended to contain UI features (such as a keyboard)." (I realize ui/ is a bit of a mess now, hopefully that won't last). components was created as a place to share code between chrome and ios. Perhaps that code should live there.
On 2017/05/26 23:07:31, sky wrote: > https://codereview.chromium.org/2869223002/diff/100001/ui/base/ui_metrics_typ... > File ui/base/ui_metrics_types.h (right): > > https://codereview.chromium.org/2869223002/diff/100001/ui/base/ui_metrics_typ... > ui/base/ui_metrics_types.h:8: namespace ui { > Sorry for the bike shed. > > The ui directory is intended for generic code used in building uis. From the > readme: "This directory contains UI frameworks used to build various user > interface features. This directory it not intended to contain UI features (such > as a keyboard)." (I realize ui/ is a bit of a mess now, hopefully that won't > last). > > components was created as a place to share code between chrome and ios. Perhaps > that code should live there. Do you think there will be a bunch of files such as this that need to be moved? If so, perhaps the directory name should reflect that, e.g. components/ui_metrics
On 2017/05/26 23:10:42, sky wrote: > On 2017/05/26 23:07:31, sky wrote: > > > https://codereview.chromium.org/2869223002/diff/100001/ui/base/ui_metrics_typ... > > File ui/base/ui_metrics_types.h (right): > > > > > https://codereview.chromium.org/2869223002/diff/100001/ui/base/ui_metrics_typ... > > ui/base/ui_metrics_types.h:8: namespace ui { > > Sorry for the bike shed. > > > > The ui directory is intended for generic code used in building uis. From the > > readme: "This directory contains UI frameworks used to build various user > > interface features. This directory it not intended to contain UI features > (such > > as a keyboard)." (I realize ui/ is a bit of a mess now, hopefully that won't > > last). > > > > components was created as a place to share code between chrome and ios. > Perhaps > > that code should live there. > > Do you think there will be a bunch of files such as this that need to be moved? > If so, perhaps the directory name should reflect that, e.g. > components/ui_metrics I can imagine that we will share more metrics in the future... I'll move this to components/ui_metrics and put up another revision, thanks!
The CQ bit was checked by peterlaurens@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: 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 peterlaurens@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_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by peterlaurens@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...
peterlaurens@chromium.org changed reviewers: + sdefresne@chromium.org
+ sdefresne as an owner of some of the new areas this change affects. Sylvain, to bring you up to speed: This is a change which allows for the sharing of a simple enum and strings for metrics collection across iOS, Android, and Desktop. sky@ suggested that //ui/base/ was not the correct place for the header file, and that this should instead be in //components/... somewhere, so this CL now has the header in //components/ui_metrics/. Your guidance is much appreciated! PTAL!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sdefresne@chromium.org changed reviewers: + mark@chromium.org
lgtm But, I'm wondering whether we should put this in //components/crash/core maybe. +mark: WDYT?
LGTM. I suppose it could. Doesn’t have to, though. Let’s leave it here and if we find that we need to reach it from more places, we can component-ize it.
The CQ bit was checked by peterlaurens@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdy@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2869223002/#ps160001 (title: "Fix Mac build")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/06/09 16:18:28, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Thanks all, I think I need sky@ to review the changes to chrome/DEPS chrome/browser/ui PTAL! - Peter
LGTM
The CQ bit was checked by peterlaurens@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 160001, "attempt_start_ts": 1497041803236430, "parent_rev": "30661e1351e5de95f097bdf40854337200a56d7c", "commit_rev": "1b59498f081885d9f82539f478653669b2c5c5a6"}
Message was sent while issue was closed.
Description was changed from ========== Metrics logging for Sad Tab Reload and Feedback buttons. BUG=719781 ========== to ========== Metrics logging for Sad Tab Reload and Feedback buttons. BUG=719781 Review-Url: https://codereview.chromium.org/2869223002 Cr-Commit-Position: refs/heads/master@{#478431} Committed: https://chromium.googlesource.com/chromium/src/+/1b59498f081885d9f82539f47865... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/1b59498f081885d9f82539f47865... |