[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
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
The CQ bit was unchecked by commit-bot@chromium.org
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_arm64_dbg_recipe/builds/229971) android_clang_dbg_recipe on ...
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
The CQ bit was unchecked by commit-bot@chromium.org
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_chromeos_rel_ng/builds/385983)
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
The CQ bit was unchecked by commit-bot@chromium.org
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_chromeos_ozone_rel_ng/builds/342660)
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
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
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.
https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkSheetContent.java File chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkSheetContent.java (right): https://codereview.chromium.org/2754313002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkSheetContent.java#newcode58 chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkSheetContent.java:58: public int getType() { Does it work if you ...
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
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by twellington@chromium.org
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
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/09e0b1fd3049d3cb92f20cd443cb02f6dab1221c
Issue 2754313002: [Home] Record some user actions for the Chrome Home BottomSheet
(Closed)
Created 1 year, 1 month ago by Theresa
Modified 1 year, 1 month ago
Reviewers: mdjones, gone, Steven Holte
Base URL:
Comments: 29