[Home] Record some user actions for the Chrome Home BottomSheet

[Home] Record some user actions for the Chrome Home BottomSheet

Record user actions for:
 * Android.ChromeHome.Closed
 * Android.ChromeHome.FullState
 * Android.ChromeHome.HalfState
 * Android.ChromeHome.Opened
 * Android.ChromeHome.ShowBookmarks
 * Android.ChromeHome.ShowDownloads
 * Android.ChromeHome.ShowHistory
 * Android.ChromeHome.ShowSuggestions

BUG=700589
Review-Url: https://codereview.chromium.org/2754313002
Cr-Commit-Position: refs/heads/master@{#458229}
Committed: https://chromium.googlesource.com/chromium/src/+/09e0b1fd3049d3cb92f20cd443cb02f6dab1221c

[Theresa, 2017-03-17 15:59:38]

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

[commit-bot: I haz the power, 2017-03-17 15:59:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2017-03-17 16:04:49]

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

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

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2017-03-17 16:07:11]

twellington@chromium.org changed reviewers:
+ mdjones@chromium.org

[Theresa, 2017-03-17 16:07:12]

ptal

https://codereview.chromium.org/2754313002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2754313002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:130:
public void onSheetContentChanged(BottomSheetContent newContent) {}
I'm considering making a new class variable for the BottomSheetObserver that
extends EmptyBottomSheetObserver to avoid the need for these empty methods.
Thoughts? Should I do the same for BottomToolbarPhone?

https://codereview.chromium.org/2754313002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java
(right):

https://codereview.chromium.org/2754313002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java:57:
RecordUserAction.record("Android.ChromeHome.Show" +
mLastContent.getMetricsName());
The upside to this approach is that it's simple. The downside to this approach
is that our presubmit scripts can't check for missing metrics due to the string
concatenation. Another approach could be:

if (newContent instanceof SuggestionsBottomSheetContent) {
   RecordUserAction.record("Android.ChromeHome.ShowSuggestions");
} else if (...) {
...
}


Or using an emun value or type returned by the BottomSheetContent to determine
which string to print.

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

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

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

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  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...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Theresa, 2017-03-17 16:34:46]

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

[commit-bot: I haz the power, 2017-03-17 16:35:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mdjones, 2017-03-17 18:17:27]

https://codereview.chromium.org/2754313002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2754313002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:130:
public void onSheetContentChanged(BottomSheetContent newContent) {}
On 2017/03/17 16:07:12, Theresa wrote:
> I'm considering making a new class variable for the BottomSheetObserver that
> extends EmptyBottomSheetObserver to avoid the need for these empty methods.
> Thoughts? Should I do the same for BottomToolbarPhone?

Yeah, I think that interface is sufficiently large to support doing this.

https://codereview.chromium.org/2754313002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java
(right):

https://codereview.chromium.org/2754313002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java:34:
if (!mIsSheetOpen) return;
Are the onSheetOpened and onSheetClosed events not 1:1?

https://codereview.chromium.org/2754313002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java:57:
RecordUserAction.record("Android.ChromeHome.Show" +
mLastContent.getMetricsName());
On 2017/03/17 16:07:12, Theresa wrote:
> The upside to this approach is that it's simple. The downside to this approach
> is that our presubmit scripts can't check for missing metrics due to the
string
> concatenation. Another approach could be:
> 
> if (newContent instanceof SuggestionsBottomSheetContent) {
>    RecordUserAction.record("Android.ChromeHome.ShowSuggestions");
> } else if (...) {
> ...
> }
> 
> 
> Or using an emun value or type returned by the BottomSheetContent to determine
> which string to print.

Though I like the current approach, I'm inclined to make this easier for other
developers. This would also be more difficult for code search. I'm not opposed
to having verbose metrics code in this class as it's already dedicated to
metrics.

https://codereview.chromium.org/2754313002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
(right):

https://codereview.chromium.org/2754313002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:754:
o.onSheetStateChanged(mCurrentState);
Admittedly the sheet state concept in this class isn't very robust. Where this
is doesn't necessarily mean the sheet is in the provided state (specifically if
"animate" is true). We should probably trigger this event in onAnimationEnd in
createSettleAnimation and conditionally block this one.

https://codereview.chromium.org/2754313002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetObserver.java
(right):

https://codereview.chromium.org/2754313002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetObserver.java:57:
void onSheetStateChanged(int newState);
@SheetState

[commit-bot: I haz the power, 2017-03-17 19:25:44]

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

[commit-bot: I haz the power, 2017-03-17 19:25:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Theresa, 2017-03-17 20:16:48]

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

[commit-bot: I haz the power, 2017-03-17 20:17:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2017-03-17 20:26:30]

https://codereview.chromium.org/2754313002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java
(right):

https://codereview.chromium.org/2754313002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java:34:
if (!mIsSheetOpen) return;
On 2017/03/17 18:17:26, mdjones wrote:
> Are the onSheetOpened and onSheetClosed events not 1:1?

onSheetClosed gets called when the activity is first initialized so this check
is needed to avoid logging an event then. I figured it was good to be
symmetrical and check in both observer methods. We could also update BottomSheet
to make sure onSheetClosed() isn't called if the sheet hasn't been opened yet.

https://codereview.chromium.org/2754313002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java:57:
RecordUserAction.record("Android.ChromeHome.Show" +
mLastContent.getMetricsName());
On 2017/03/17 18:17:26, mdjones wrote:
> On 2017/03/17 16:07:12, Theresa wrote:
> > The upside to this approach is that it's simple. The downside to this
approach
> > is that our presubmit scripts can't check for missing metrics due to the
> string
> > concatenation. Another approach could be:
> > 
> > if (newContent instanceof SuggestionsBottomSheetContent) {
> >    RecordUserAction.record("Android.ChromeHome.ShowSuggestions");
> > } else if (...) {
> > ...
> > }
> > 
> > 
> > Or using an emun value or type returned by the BottomSheetContent to
determine
> > which string to print.
> 
> Though I like the current approach, I'm inclined to make this easier for other
> developers. This would also be more difficult for code search. I'm not opposed
> to having verbose metrics code in this class as it's already dedicated to
> metrics.

Done.

https://codereview.chromium.org/2754313002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
(right):

https://codereview.chromium.org/2754313002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:754:
o.onSheetStateChanged(mCurrentState);
On 2017/03/17 18:17:26, mdjones wrote:
> Admittedly the sheet state concept in this class isn't very robust. Where this
> is doesn't necessarily mean the sheet is in the provided state (specifically
if
> "animate" is true). We should probably trigger this event in onAnimationEnd in
> createSettleAnimation and conditionally block this one.

As discussed offline, waiting until onAnimationEnd and not logging if the
animation is canceled may cause states to be skipped e.g. animation to go full
state gets interrupted and user finishes dragging up to full state. This can be
fixed if we represent the current state separately from the target state.

https://codereview.chromium.org/2754313002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetObserver.java
(right):

https://codereview.chromium.org/2754313002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetObserver.java:57:
void onSheetStateChanged(int newState);
On 2017/03/17 18:17:26, mdjones wrote:
> @SheetState

Done.

[commit-bot: I haz the power, 2017-03-17 21:15:49]

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

[commit-bot: I haz the power, 2017-03-17 21:15:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mdjones, 2017-03-17 21:53:32]

https://codereview.chromium.org/2754313002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java
(right):

https://codereview.chromium.org/2754313002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java:34:
if (!mIsSheetOpen) return;
On 2017/03/17 20:26:30, Theresa wrote:
> On 2017/03/17 18:17:26, mdjones wrote:
> > Are the onSheetOpened and onSheetClosed events not 1:1?
> 
> onSheetClosed gets called when the activity is first initialized so this check
> is needed to avoid logging an event then. I figured it was good to be
> symmetrical and check in both observer methods. We could also update
BottomSheet
> to make sure onSheetClosed() isn't called if the sheet hasn't been opened yet.

I think I'd rather have this check in the bottom sheet. It should be the
observed object's responsibility to trigger reliable events.

[Theresa, 2017-03-17 22:54:22]

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

[commit-bot: I haz the power, 2017-03-17 22:55:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2017-03-17 22:55:25]

https://codereview.chromium.org/2754313002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java
(right):

https://codereview.chromium.org/2754313002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java:34:
if (!mIsSheetOpen) return;
On 2017/03/17 21:53:32, mdjones wrote:
> On 2017/03/17 20:26:30, Theresa wrote:
> > On 2017/03/17 18:17:26, mdjones wrote:
> > > Are the onSheetOpened and onSheetClosed events not 1:1?
> > 
> > onSheetClosed gets called when the activity is first initialized so this
check
> > is needed to avoid logging an event then. I figured it was good to be
> > symmetrical and check in both observer methods. We could also update
> BottomSheet
> > to make sure onSheetClosed() isn't called if the sheet hasn't been opened
yet.
> 
> I think I'd rather have this check in the bottom sheet. It should be the
> observed object's responsibility to trigger reliable events.

Done.

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java
(right):

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java:50:
if (mLastContent == null  || !mIsSheetOpen) {
nit: remove the extra space that crept in.

[mdjones, 2017-03-17 23:07:16]

lgtm!

[commit-bot: I haz the power, 2017-03-18 02:34:46]

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

[commit-bot: I haz the power, 2017-03-18 02:34:47]

Dry run: This issue passed the CQ dry run.

[Theresa, 2017-03-20 15:22:52]

twellington@chromium.org changed reviewers:
+ dfalcantara@chromium.org, holte@chromium.org

[Theresa, 2017-03-20 15:22:53]

+dfalcantara@ for general OWNERS
+holte@ for actions.xml

[Steven Holte, 2017-03-20 19:34:56]

lgtm

[gone, 2017-03-20 20:29:32]

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkSheetContent.java
(right):

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkSheetContent.java:58:
public int getType() {
Does it work if you try to annotate these with @ContentType?

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
(right):

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:519:

nit: too many newlines

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:761:
if (!stateChanged) return;
Why is this check so far down?  Seems like you would want to avoid doing
anything if the state hasn't changed.

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java
(right):

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java:21:
sheet.addObserver(this);
This should be left to the BottomSheet to do.  It's weird having it just be a
dangly new BottomSheetMetrics(sheet) from the calling location.

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java:63:
}
else assert false;

[Theresa, 2017-03-20 20:59:12]

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkSheetContent.java
(right):

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkSheetContent.java:58:
public int getType() {
On 2017/03/20 20:29:31, dfalcantara (load balance plz) wrote:
> Does it work if you try to annotate these with @ContentType?

I added an annotation to the superclass method. Will that suffice or do you want
to see it on every subclass method?

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
(right):

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:519:

On 2017/03/20 20:29:31, dfalcantara (load balance plz) wrote:
> nit: too many newlines

Done.

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:761:
if (!stateChanged) return;
On 2017/03/20 20:29:31, dfalcantara (load balance plz) wrote:
> Why is this check so far down?  Seems like you would want to avoid doing
> anything if the state hasn't changed.

The sheet doesn't end up in the correct place if this returns earlier. I think
the change Matt has in-flight will help correct that:
https://codereview.chromium.org/2759823002/

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java
(right):

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java:21:
sheet.addObserver(this);
On 2017/03/20 20:29:32, dfalcantara (load balance plz) wrote:
> This should be left to the BottomSheet to do.  It's weird having it just be a
> dangly new BottomSheetMetrics(sheet) from the calling location.

Done.

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java:50:
if (mLastContent == null  || !mIsSheetOpen) {
On 2017/03/17 22:55:25, Theresa wrote:
> nit: remove the extra space that crept in.

Done.

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java:63:
}
On 2017/03/20 20:29:32, dfalcantara (load balance plz) wrote:
> else assert false;

Done.

[Theresa, 2017-03-20 21:01:14]

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

[commit-bot: I haz the power, 2017-03-20 21:01:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gone, 2017-03-20 21:04:52]

lgtm

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkSheetContent.java
(right):

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkSheetContent.java:58:
public int getType() {
On 2017/03/20 20:59:12, Theresa wrote:
> On 2017/03/20 20:29:31, dfalcantara (load balance plz) wrote:
> > Does it work if you try to annotate these with @ContentType?
> 
> I added an annotation to the superclass method. Will that suffice or do you
want
> to see it on every subclass method?

super's probably cool.  Hard to gauge where it should go, given that this thing
is Overriding something.  I'd ask if Android Studio complains, but I'm not sure
anyone's really using that at the moment.

https://codereview.chromium.org/2754313002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
(right):

https://codereview.chromium.org/2754313002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:286:
addObserver(metrics);
maybe just make it addObserver(new BottomSheetMetrics())?

[Theresa, 2017-03-20 21:22:48]

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

[Theresa, 2017-03-20 21:22:53]

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkSheetContent.java
(right):

https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkSheetContent.java:58:
public int getType() {
On 2017/03/20 21:04:52, dfalcantara (load balance plz) wrote:
> On 2017/03/20 20:59:12, Theresa wrote:
> > On 2017/03/20 20:29:31, dfalcantara (load balance plz) wrote:
> > > Does it work if you try to annotate these with @ContentType?
> > 
> > I added an annotation to the superclass method. Will that suffice or do you
> want
> > to see it on every subclass method?
> 
> super's probably cool.  Hard to gauge where it should go, given that this
thing
> is Overriding something.  I'd ask if Android Studio complains, but I'm not
sure
> anyone's really using that at the moment.

With the superclass annotation, Android Studio complains if the subclass
implementation returns an int rather than a ContentType. Eclipse doesn't seem to
care regardless of annotations.

https://codereview.chromium.org/2754313002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
(right):

https://codereview.chromium.org/2754313002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:286:
addObserver(metrics);
On 2017/03/20 21:04:52, dfalcantara (load balance plz) wrote:
> maybe just make it addObserver(new BottomSheetMetrics())?

Done.

[Theresa, 2017-03-20 21:23:27]

The CQ bit was unchecked by twellington@chromium.org

[Theresa, 2017-03-20 21:23:28]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2017-03-20 21:23:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, holte@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2754313002/#ps120001
(title: "[Home] Record some user actions for the Chrome Home BottomSheet")

[commit-bot: I haz the power, 2017-03-20 21:23:30]

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-03-20 23:18:47]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490045008056080,
"parent_rev": "de52d1630afa30d255f1e773d350c758056fd2ed", "commit_rev":
"09e0b1fd3049d3cb92f20cd443cb02f6dab1221c"}

[commit-bot: I haz the power, 2017-03-20 23:19:51]

Description was changed from

==========
[Home] Record some user actions for the Chrome Home BottomSheet

Record user actions for:
 * Android.ChromeHome.Closed
 * Android.ChromeHome.FullState
 * Android.ChromeHome.HalfState
 * Android.ChromeHome.Opened
 * Android.ChromeHome.ShowBookmarks
 * Android.ChromeHome.ShowDownloads
 * Android.ChromeHome.ShowHistory
 * Android.ChromeHome.ShowSuggestions

BUG=700589
==========

to

==========
[Home] Record some user actions for the Chrome Home BottomSheet

Record user actions for:
 * Android.ChromeHome.Closed
 * Android.ChromeHome.FullState
 * Android.ChromeHome.HalfState
 * Android.ChromeHome.Opened
 * Android.ChromeHome.ShowBookmarks
 * Android.ChromeHome.ShowDownloads
 * Android.ChromeHome.ShowHistory
 * Android.ChromeHome.ShowSuggestions

BUG=700589

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

[commit-bot: I haz the power, 2017-03-20 23:19:52]

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