Metrics logging for user engagement of the Reload and Feedback buttons

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/+/1b59498f081885d9f82539f478653669b2c5c5a6

[PL, 2017-05-09 22:38:23]

peterlaurens@chromium.org changed reviewers:
+ eugenebut@chromium.org

[PL, 2017-05-09 22:38:23]

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!

[Eugene But (OOO till 7-30), 2017-05-09 22:45:58]

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.

[PL, 2017-05-09 22:47:54]

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

[PL, 2017-05-09 22:57:20]

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.

[Sidney San Martín, 2017-05-12 03:03:20]

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!

[PL, 2017-05-16 23:18:49]

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

[Eugene But (OOO till 7-30), 2017-05-16 23:49:33]

+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

[Sidney San Martín, 2017-05-19 20:54:06]

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

[PL, 2017-05-23 23:06:38]

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!

[PL, 2017-05-23 23:07:18]

The CQ bit was checked by peterlaurens@chromium.org

[commit-bot: I haz the power, 2017-05-23 23:08:13]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 23:08:16]

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

[commit-bot: I haz the power, 2017-05-23 23:08:17]

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.

[PL, 2017-05-23 23:09:02]

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

[commit-bot: I haz the power, 2017-05-23 23:09:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 23:46:19]

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

[commit-bot: I haz the power, 2017-05-23 23:46:19]

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

[Sidney San Martín, 2017-05-24 14:28:26]

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.

[Sidney San Martín, 2017-05-24 14:30:41]

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

[PL, 2017-05-25 15:59:55]

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

[commit-bot: I haz the power, 2017-05-25 16:00:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 16:04:36]

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

[commit-bot: I haz the power, 2017-05-25 16:04:37]

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

[PL, 2017-05-25 22:27:51]

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

[commit-bot: I haz the power, 2017-05-25 22:28:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 22:56:37]

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

[commit-bot: I haz the power, 2017-05-25 22:56:40]

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

[PL, 2017-05-25 23:07:06]

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

[commit-bot: I haz the power, 2017-05-25 23:08:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 03:55:14]

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

[commit-bot: I haz the power, 2017-05-26 03:55:16]

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

[PL, 2017-05-26 17:15:50]

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

[commit-bot: I haz the power, 2017-05-26 17:16:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 18:08:35]

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

[commit-bot: I haz the power, 2017-05-26 18:08:36]

Dry run: This issue passed the CQ dry run.

[PL, 2017-05-26 18:36:10]

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

[Eugene But (OOO till 7-30), 2017-05-26 18:38:16]

lgtm

[Sidney San Martín, 2017-05-26 18:40:35]

lgtm

[PL, 2017-05-26 18:43:06]

The CQ bit was checked by peterlaurens@chromium.org

[commit-bot: I haz the power, 2017-05-26 18:43:55]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 18:52:15]

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

[commit-bot: I haz the power, 2017-05-26 18:52:16]

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

[PL, 2017-05-26 20:29:32]

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.

[PL, 2017-05-26 20:42:44]

peterlaurens@chromium.org changed reviewers:
+ sky@chromium.org

[PL, 2017-05-26 20:42:44]

+ 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

[sky, 2017-05-26 23:07:31]

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.

[sky, 2017-05-26 23:10:42]

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

[PL, 2017-06-07 17:38:18]

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!

[PL, 2017-06-07 21:27:56]

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

[commit-bot: I haz the power, 2017-06-07 21:28:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 21:46:29]

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

[commit-bot: I haz the power, 2017-06-07 21:46:32]

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

[PL, 2017-06-07 22:00:09]

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

[commit-bot: I haz the power, 2017-06-07 22:00:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 22:15:40]

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

[commit-bot: I haz the power, 2017-06-07 22:15:41]

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

[PL, 2017-06-07 22:58:52]

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

[commit-bot: I haz the power, 2017-06-07 22:59:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[PL, 2017-06-08 00:28:39]

peterlaurens@chromium.org changed reviewers:
+ sdefresne@chromium.org

[PL, 2017-06-08 00:28:40]

+ 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!

[commit-bot: I haz the power, 2017-06-08 00:30:18]

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

[commit-bot: I haz the power, 2017-06-08 00:30:19]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2017-06-09 14:47:41]

sdefresne@chromium.org changed reviewers:
+ mark@chromium.org

[sdefresne, 2017-06-09 14:47:42]

lgtm

But, I'm wondering whether we should put this in //components/crash/core maybe.
+mark: WDYT?

[Mark Mentovai, 2017-06-09 14:55:21]

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.

[PL, 2017-06-09 16:09:01]

The CQ bit was checked by peterlaurens@chromium.org

[PL, 2017-06-09 16:09:02]

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

[commit-bot: I haz the power, 2017-06-09 16:09:29]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 16:18:27]

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

[commit-bot: I haz the power, 2017-06-09 16:18:28]

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

[PL, 2017-06-09 17:09:53]

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

[sky, 2017-06-09 18:18:23]

LGTM

[PL, 2017-06-09 20:56:43]

The CQ bit was checked by peterlaurens@chromium.org

[commit-bot: I haz the power, 2017-06-09 20:57:01]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 21:56:52]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1497041803236430,
"parent_rev": "30661e1351e5de95f097bdf40854337200a56d7c", "commit_rev":
"1b59498f081885d9f82539f478653669b2c5c5a6"}

[commit-bot: I haz the power, 2017-06-09 21:57:04]

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

[commit-bot: I haz the power, 2017-06-09 21:57:06]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/1b59498f081885d9f82539f47865...