Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2862893002:
📰 Add visibility change triggers for bottom sheet content (Closed)
Description[Suggestions] Add visibility change triggers for bottom sheet content
Add SheetContentStateChangeObserver, which emits events specialised
for the visibility of a BottomSheetContent, and use it to record
user actions for the Suggestions bottom sheet
BUG=693593, 717704
Review-Url: https://codereview.chromium.org/2862893002
Cr-Commit-Position: refs/heads/master@{#473699}
Committed: https://chromium.googlesource.com/chromium/src/+/2401662bf23c6bb0a9e666324cc9d72158d9cbbb
Patch Set 1 #Patch Set 2 : cleanup, support app switching #
Total comments: 15
Patch Set 3 : cleanup and address comments #Patch Set 4 : include new file #Patch Set 5 : #
Total comments: 2
Patch Set 6 : rebase, address comment, mark actions not user triggered #Patch Set 7 : rebase, remove not_user_triggered #Patch Set 8 : [Suggestions] Add visibility change triggers for bottm sheet content #Patch Set 9 : Fix compilation post merge #
Messages
Total messages: 40 (28 generated)
Description was changed from ========== [Suggestions] Add visibility change triggers for bottm sheet content Add SheetContentStateChangeObserver, which emits events specialised for the visibility of a BottomSheetContent, and use it to record user actions for the Suggestions bottom sheet BUG=693593 ========== to ========== [Suggestions] Add visibility change triggers for bottm sheet content Add SheetContentStateChangeObserver, which emits events specialised for the visibility of a BottomSheetContent, and use it to record user actions for the Suggestions bottom sheet BUG=693593 ==========
Description was changed from ========== [Suggestions] Add visibility change triggers for bottm sheet content Add SheetContentStateChangeObserver, which emits events specialised for the visibility of a BottomSheetContent, and use it to record user actions for the Suggestions bottom sheet BUG=693593 ========== to ========== [Suggestions] Add visibility change triggers for bottom sheet content Add SheetContentStateChangeObserver, which emits events specialised for the visibility of a BottomSheetContent, and use it to record user actions for the Suggestions bottom sheet BUG=693593 ==========
dgn@chromium.org changed reviewers: + bauerb@chromium.org, mdjones@chromium.org
PTAL https://codereview.chromium.org/2862893002/diff/20001/base/metrics/user_metri... File base/metrics/user_metrics.cc (right): https://codereview.chromium.org/2862893002/diff/20001/base/metrics/user_metri... base/metrics/user_metrics.cc:37: LOG(INFO) << "RecordAction: " << action; will be removed before landing
https://codereview.chromium.org/2862893002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/SheetContentStateChangeObserver.java (right): https://codereview.chromium.org/2862893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/SheetContentStateChangeObserver.java:44: public abstract void onContentStateChanged(int sheetState); @BottomSheet.SheetState? https://codereview.chromium.org/2862893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/SheetContentStateChangeObserver.java:68: if (newState == BottomSheet.SHEET_STATE_SCROLLING) return; // Ignore intermediate states. Don't you already have a seemingly more accurate check in onStateChange()? https://codereview.chromium.org/2862893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/SheetContentStateChangeObserver.java:79: mBottomSheet.getCurrentSheetContent(); What's this for? Ensuring that the sheet content is created? It seems that would already happen in line 76 (in order for |newVisibility| to be true, both conditions need to be true and therefore get evaluated). https://codereview.chromium.org/2862893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/SheetContentStateChangeObserver.java:82: if (newVisibility == mCurrentVisibility && mCurrentSheetState == newSheetState) return; The first part of this check is already in line 84, and you could just move the second part to line 93.
Description was changed from ========== [Suggestions] Add visibility change triggers for bottom sheet content Add SheetContentStateChangeObserver, which emits events specialised for the visibility of a BottomSheetContent, and use it to record user actions for the Suggestions bottom sheet BUG=693593 ========== to ========== [Suggestions] Add visibility change triggers for bottom sheet content Add SheetContentStateChangeObserver, which emits events specialised for the visibility of a BottomSheetContent, and use it to record user actions for the Suggestions bottom sheet BUG=693593,717704 ==========
https://codereview.chromium.org/2862893002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java (right): https://codereview.chromium.org/2862893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:361: /** Returns whether the bottom sheet is in a stable state, not transitioning or animating. */ Full javadoc please. https://codereview.chromium.org/2862893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:368: case SHEET_STATE_NONE: SHEET_STATE_NONE is a private member that is used for internal state management. I'd rather this not be handled here as it implies external knowledge of the variable. Instead, I'd have an assert at the top to make sure the input is not that. https://codereview.chromium.org/2862893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:592: ApplicationStatus.registerStateListenerForActivity( I'd rather see this logic attached to the SuggestionsBottomSheetContent (BottomSheetContentController has access to ChromeActivity). My reasoning is that this is an event on the activity and not the sheet; this seems like an unnecessary layer for the single use it has.
The CQ bit was checked by dgn@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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by dgn@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...
dgn@chromium.org changed reviewers: + asvitkine@chromium.org
PTAL https://codereview.chromium.org/2862893002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java (right): https://codereview.chromium.org/2862893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:361: /** Returns whether the bottom sheet is in a stable state, not transitioning or animating. */ On 2017/05/05 15:52:00, mdjones wrote: > Full javadoc please. Done. https://codereview.chromium.org/2862893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:368: case SHEET_STATE_NONE: On 2017/05/05 15:52:00, mdjones wrote: > SHEET_STATE_NONE is a private member that is used for internal state management. > I'd rather this not be handled here as it implies external knowledge of the > variable. Instead, I'd have an assert at the top to make sure the input is not > that. Added an assert here, as otherwise the linter complains about unhandled enum value. https://codereview.chromium.org/2862893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:592: ApplicationStatus.registerStateListenerForActivity( On 2017/05/05 15:52:00, mdjones wrote: > I'd rather see this logic attached to the SuggestionsBottomSheetContent > (BottomSheetContentController has access to ChromeActivity). My reasoning is > that this is an event on the activity and not the sheet; this seems like an > unnecessary layer for the single use it has. Done. https://codereview.chromium.org/2862893002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/SheetContentStateChangeObserver.java (right): https://codereview.chromium.org/2862893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/SheetContentStateChangeObserver.java:44: public abstract void onContentStateChanged(int sheetState); On 2017/05/05 13:39:54, Bernhard Bauer wrote: > @BottomSheet.SheetState? Done. https://codereview.chromium.org/2862893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/SheetContentStateChangeObserver.java:68: if (newState == BottomSheet.SHEET_STATE_SCROLLING) return; // Ignore intermediate states. On 2017/05/05 13:39:55, Bernhard Bauer wrote: > Don't you already have a seemingly more accurate check in onStateChange()? Done. https://codereview.chromium.org/2862893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/SheetContentStateChangeObserver.java:79: mBottomSheet.getCurrentSheetContent(); On 2017/05/05 13:39:55, Bernhard Bauer wrote: > What's this for? Ensuring that the sheet content is created? It seems that would > already happen in line 76 (in order for |newVisibility| to be true, both > conditions need to be true and therefore get evaluated). oops ignore, that has nothing to do here. https://codereview.chromium.org/2862893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/SheetContentStateChangeObserver.java:82: if (newVisibility == mCurrentVisibility && mCurrentSheetState == newSheetState) return; On 2017/05/05 13:39:55, Bernhard Bauer wrote: > The first part of this check is already in line 84, and you could just move the > second part to line 93. Hum that's the kind of stuff that makes me think I should not present this as a general purpose class. I would still want to keep the whole check for the onContentStateChanged detection, because even if the current state didn't change, if the visibility did I still want the notification.
lgtm % nit https://codereview.chromium.org/2862893002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java (right): https://codereview.chromium.org/2862893002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:365: assert false; // Should never be tested, internal only value. nit: make "assert false;" the default. This is what we do for other switch statements. So: case SHEET_STATE_NONE: default: assert false;
lgtm
https://codereview.chromium.org/2862893002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java (right): https://codereview.chromium.org/2862893002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:365: assert false; // Should never be tested, internal only value. On 2017/05/09 17:20:15, mdjones wrote: > nit: make "assert false;" the default. This is what we do for other switch > statements. So: > > case SHEET_STATE_NONE: > default: > assert false; Done.
lgtm
The CQ bit was checked by dgn@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 dgn@chromium.org
The CQ bit was checked by dgn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, mdjones@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2862893002/#ps120001 (title: "rebase, remove not_user_triggered")
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
Failed to apply patch for
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java:
While running git apply --index -3 -p1;
error: patch failed:
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java:30
Falling back to three-way merge...
Applied patch to
'chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java'
with conflicts.
U
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java
Patch:
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java
Index:
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java
diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java
b/chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java
index
e51070fd02aabad2f6e48401c8b93bed4e159e3b..8e8380ffc3b8373e845a1d8665e2117446d0d1bf
100644
---
a/chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java
+++
b/chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java
@@ -30,8 +30,6 @@
import org.chromium.chrome.browser.widget.FadingShadowView;
import org.chromium.chrome.browser.widget.bottomsheet.BottomSheet;
import
org.chromium.chrome.browser.widget.bottomsheet.BottomSheetContentController;
-import org.chromium.chrome.browser.widget.bottomsheet.BottomSheetObserver;
-import org.chromium.chrome.browser.widget.bottomsheet.EmptyBottomSheetObserver;
import org.chromium.chrome.browser.widget.displaystyle.UiConfig;
/**
@@ -47,8 +45,7 @@
private final ContextMenuManager mContextMenuManager;
private final SuggestionsUiDelegateImpl mSuggestionsUiDelegate;
private final TileGroup.Delegate mTileGroupDelegate;
- private final BottomSheet mBottomSheet;
- private final BottomSheetObserver mBottomSheetObserver;
+ private final SuggestionsSheetVisibilityChangeObserver
mBottomSheetObserver;
public SuggestionsBottomSheetContent(final ChromeActivity activity, final
BottomSheet sheet,
TabModelSelector tabModelSelector, SnackbarManager snackbarManager)
{
@@ -87,25 +84,36 @@ public void onDestroy() {
mContextMenuManager, mTileGroupDelegate);
mRecyclerView.init(uiConfig, mContextMenuManager, adapter);
- mBottomSheetObserver = new EmptyBottomSheetObserver() {
+ mBottomSheetObserver = new
SuggestionsSheetVisibilityChangeObserver(this, activity) {
@Override
public void onSheetOpened() {
mRecyclerView.scrollToPosition(0);
- prepareSuggestionsForReveal(adapter);
-
+ adapter.refreshSuggestions();
+ mSuggestionsUiDelegate.getEventReporter().onSurfaceOpened();
mRecyclerView.getScrollEventReporter().reset();
+
+ super.onSheetOpened();
+ }
+
+ @Override
+ public void onContentShown() {
+ SuggestionsMetrics.recordSurfaceVisible();
}
@Override
- public void onSheetClosed() {
+ public void onContentHidden() {
SuggestionsMetrics.recordSurfaceHidden();
}
+ @Override
+ public void onContentStateChanged(@BottomSheet.SheetState int
contentState) {
+ if (contentState == BottomSheet.SHEET_STATE_HALF) {
+ SuggestionsMetrics.recordSurfaceHalfVisible();
+ } else if (contentState == BottomSheet.SHEET_STATE_FULL) {
+ SuggestionsMetrics.recordSurfaceFullyVisible();
+ }
+ }
};
- mBottomSheet = activity.getBottomSheet();
- mBottomSheet.addObserver(mBottomSheetObserver);
-
- if (mBottomSheet.isSheetOpen()) prepareSuggestionsForReveal(adapter);
mShadowView = (FadingShadowView) mView.findViewById(R.id.shadow);
mShadowView.init(
@@ -157,7 +165,7 @@ public int getVerticalScrollOffset() {
@Override
public void destroy() {
- mBottomSheet.removeObserver(mBottomSheetObserver);
+ mBottomSheetObserver.onDestroy();
mSuggestionsUiDelegate.onDestroy();
mTileGroupDelegate.destroy();
}
@@ -167,13 +175,6 @@ public int getType() {
return BottomSheetContentController.TYPE_SUGGESTIONS;
}
- /** Called when the UI is revlealed, prepares the list of suggestions. */
- private void prepareSuggestionsForReveal(NewTabPageAdapter adapter) {
- adapter.refreshSuggestions();
- mSuggestionsUiDelegate.getEventReporter().onSurfaceOpened();
- SuggestionsMetrics.recordSurfaceVisible();
- }
-
public static void setSuggestionsSourceForTesting(SuggestionsSource
suggestionsSource) {
sSuggestionsSourceForTesting = suggestionsSource;
}
The CQ bit was checked by dgn@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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by dgn@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 dgn@chromium.org
The CQ bit was checked by dgn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, mdjones@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2862893002/#ps160001 (title: "Fix compilation post merge")
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": 1495483326258910,
"parent_rev": "b4388f67d576b77e139f834695c461e625560779", "commit_rev":
"2401662bf23c6bb0a9e666324cc9d72158d9cbbb"}
Message was sent while issue was closed.
Description was changed from ========== [Suggestions] Add visibility change triggers for bottom sheet content Add SheetContentStateChangeObserver, which emits events specialised for the visibility of a BottomSheetContent, and use it to record user actions for the Suggestions bottom sheet BUG=693593,717704 ========== to ========== [Suggestions] Add visibility change triggers for bottom sheet content Add SheetContentStateChangeObserver, which emits events specialised for the visibility of a BottomSheetContent, and use it to record user actions for the Suggestions bottom sheet BUG=693593,717704 Review-Url: https://codereview.chromium.org/2862893002 Cr-Commit-Position: refs/heads/master@{#473699} Committed: https://chromium.googlesource.com/chromium/src/+/2401662bf23c6bb0a9e666324cc9... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/2401662bf23c6bb0a9e666324cc9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
