New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 764391 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug

Blocking:
issue 731128
issue 762549



Sign in to add a comment

Provide reasons for Chrome Home's bottom sheet changing state

Project Member Reported by mdjones@chromium.org, Sep 12 2017

Issue description

As more content becomes available in the bottom sheet, the logic to determine which should show is becoming more complicated. To simplify things, calls to setSheetState should provide a reason that will be propagated through the BottomSheetObservers to classes that are interested in the information.
 
Blocking: 731128

Comment 2 by dgn@chromium.org, Sep 12 2017

Blocking: 762549
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0c860bfecebfc1395fb8a8f6e28f3ea7c2f921b1

commit 0c860bfecebfc1395fb8a8f6e28f3ea7c2f921b1
Author: Matthew Jones <mdjones@chromium.org>
Date: Tue Sep 12 20:46:22 2017

[Home] Add state change reason to BottomSheet#setSheetState

This change introduces the concept of 'state change reason' to the
bottom sheet. Once setSheetState is called with a reason, that
reason persists through all the events until the sheet reaches its
final state or setSheetState is called again. This change allows
users of the API to make more nuanced decisions based on what
caused the bottom sheet to, for example, open.

The first change to use this API is the placeholder content in the
content controller. If the sheet is opened because of the omnibox
focusing, the placeholder will be shown instead of suggestions.
Previously this was done by immediately replacing the suggestions
content when the omnibox gained focus.

BUG= 764391 

Change-Id: I4fb129037b9fc8981ad5793fff5589524300b5a2
Reviewed-on: https://chromium-review.googlesource.com/656078
Commit-Queue: Matthew Jones <mdjones@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501389}
[modify] https://crrev.com/0c860bfecebfc1395fb8a8f6e28f3ea7c2f921b1/chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsSheetVisibilityChangeObserver.java
[modify] https://crrev.com/0c860bfecebfc1395fb8a8f6e28f3ea7c2f921b1/chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java
[modify] https://crrev.com/0c860bfecebfc1395fb8a8f6e28f3ea7c2f921b1/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
[modify] https://crrev.com/0c860bfecebfc1395fb8a8f6e28f3ea7c2f921b1/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
[modify] https://crrev.com/0c860bfecebfc1395fb8a8f6e28f3ea7c2f921b1/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java
[modify] https://crrev.com/0c860bfecebfc1395fb8a8f6e28f3ea7c2f921b1/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetObserver.java
[modify] https://crrev.com/0c860bfecebfc1395fb8a8f6e28f3ea7c2f921b1/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/EmptyBottomSheetObserver.java
[modify] https://crrev.com/0c860bfecebfc1395fb8a8f6e28f3ea7c2f921b1/chrome/test/android/javatests/src/org/chromium/chrome/test/BottomSheetTestRule.java

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bb9e8226df6785c358201bbe584e64a356e976b3

commit bb9e8226df6785c358201bbe584e64a356e976b3
Author: Matthew Jones <mdjones@chromium.org>
Date: Wed Sep 13 22:46:15 2017

[Home] Refactor sheet open logging to use StateChangeReason

This change removes the explicit logging for the bottom sheet open
reason in favor of the reason that can now be provided. The close
reason will be converted in a separate change.

BUG= 764391 

Change-Id: I4ebc3e5b893900f85d42a113a6a7bf19461b10f7
Reviewed-on: https://chromium-review.googlesource.com/657926
Commit-Queue: Matthew Jones <mdjones@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501777}
[modify] https://crrev.com/bb9e8226df6785c358201bbe584e64a356e976b3/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
[modify] https://crrev.com/bb9e8226df6785c358201bbe584e64a356e976b3/chrome/android/java/src/org/chromium/chrome/browser/ntp/NativePageFactory.java
[modify] https://crrev.com/bb9e8226df6785c358201bbe584e64a356e976b3/chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java
[modify] https://crrev.com/bb9e8226df6785c358201bbe584e64a356e976b3/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
[modify] https://crrev.com/bb9e8226df6785c358201bbe584e64a356e976b3/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java
[modify] https://crrev.com/bb9e8226df6785c358201bbe584e64a356e976b3/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetNewTabController.java

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8080665661ba4c95866cc9baae1faa91df72966d

commit 8080665661ba4c95866cc9baae1faa91df72966d
Author: Matthew Jones <mdjones@chromium.org>
Date: Thu Sep 14 16:01:49 2017

[Home] Add StateChangeReason as param to BottomSheetObserver.onSheetClosed

This change adds a parameter to onSheetClosed that allows users of the
method to know the reason the sheet was closed. A result of this is
the ability to remove some of the infrastructure around the bottom
sheet's metrics; the reason no longer needs to be explicitly set.

BUG= 764391 

Change-Id: I07ee183bd3acec256791338e3c1bda441f0fe46c
Reviewed-on: https://chromium-review.googlesource.com/661099
Commit-Queue: Matthew Jones <mdjones@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501961}
[modify] https://crrev.com/8080665661ba4c95866cc9baae1faa91df72966d/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
[modify] https://crrev.com/8080665661ba4c95866cc9baae1faa91df72966d/chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetContent.java
[modify] https://crrev.com/8080665661ba4c95866cc9baae1faa91df72966d/chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsSheetVisibilityChangeObserver.java
[modify] https://crrev.com/8080665661ba4c95866cc9baae1faa91df72966d/chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java
[modify] https://crrev.com/8080665661ba4c95866cc9baae1faa91df72966d/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
[modify] https://crrev.com/8080665661ba4c95866cc9baae1faa91df72966d/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
[modify] https://crrev.com/8080665661ba4c95866cc9baae1faa91df72966d/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetMetrics.java
[modify] https://crrev.com/8080665661ba4c95866cc9baae1faa91df72966d/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetNewTabController.java
[modify] https://crrev.com/8080665661ba4c95866cc9baae1faa91df72966d/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetObserver.java
[modify] https://crrev.com/8080665661ba4c95866cc9baae1faa91df72966d/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/EmptyBottomSheetObserver.java
[modify] https://crrev.com/8080665661ba4c95866cc9baae1faa91df72966d/chrome/test/android/javatests/src/org/chromium/chrome/test/BottomSheetTestRule.java

Labels: Hotlist-Chrome-Home

Comment 7 by k...@chromium.org, Sep 21 2017

Labels: Fine-Pri-3.0
Status: Fixed (was: Assigned)

Sign in to add a comment