Introduce Queue-Based Notification Snackbars

Introduce Queue-Based Notification Snackbars

Notification snackbars are low-priority snackbars that are stored in a
queue, contrary to tranditional "action" snackbars that ought to be
shown immediately and are stored in a stack.

When a queue snackbar is showing, a stack snackbar can always kick in
and override the snackbar; however when a stack snackbar is showing, a
queue snackbar will wait till the stack is cleared.

Queue snackbars are not dismissed in batch; instead, they will be shown
one by one, in FIFO order.

BUG=579347
Committed: https://crrev.com/d117eb1c195ee27a75c12bde155438a13955ec0c
Cr-Commit-Position: refs/heads/master@{#371899}

[Ian Wen, 2016-01-25 21:23:32]

ianwen@chromium.org changed reviewers:
+ newt@chromium.org

[newt (away), 2016-01-26 21:26:45]

Thanks for this change, Ian!

There are a few structural issues that I can think can be improved -- see the
list of ideas below. Here are the main issues I noticed:

 - Stack/Queue logic is leaking outside of SnackbarContainer: mHideRunnable and
showSnackbar() both have extensive knowledge of the internals of
SnackbarContainer

 - Calls to SnackbarController.onDismissNoAction() are scattered about. It's
hard to reason about whether we're accidentally missing or duplicating calls to
onDismissNoAction().

 - Calling dismissAllSnackbars() after the last snackbar has already been
dismissed is confusing but currently necessary, due to dismissAllSnackbars's
dual role (calling onDismissNoAction() *and* updating the popup state). This is
a pre-existing condition that I think we should fix now.

Here are some ideas for structuring this code that I think will keep things
cleaner:

 - Snackbar updates (showing, hiding, clicking, timing out, etc) should consist
of two phases:
    1. Update the SnackbarContainer to reflect the new state
    2. Update the popup to reflect SnackbarContainer.peek()

 - All changes to the popup state (and ViewTreeObserver) should be centralized,
and isolated from the changes to SnackbarContainer. This method could be called
"updatePopup()". This method would show the popup, change the popup text, or
hide the popup, depending on the value of SnackbarContainer.peek().

 - Keep all knowledge of the stack/queue craziness inside of SnackbarContainer.
SnackbarManager shouldn't ever need to check isOnStack(). It might be helpful to
add methods to SnackbarContainer for removing the current snackbar (and possibly
calling onAction()) and for removing all snackbars (and possible calling
onDismissNoAction())

 - Put all the calls to onDismissNoAction() in one place, perhaps inside
SnackbarContainer.

https://codereview.chromium.org/1635753002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java
(right):

https://codereview.chromium.org/1635753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:81:
private SnackbarContainer mContainer = new SnackbarContainer();
How about s/mContainer/mSnackbars/

https://codereview.chromium.org/1635753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:305:
private static class SnackbarContainer {
I don't think "Container" is the right word here. How about "SnackbarQueue" or
"SnackbarList"?

https://codereview.chromium.org/1635753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:318:
* @return The next snackbar to display.
Is this really "the next snackbar to display"? Isn't this just the currently
displayed snackbar (just like peek())?

Perhaps this method should be renamed "removeCurrentSnackbar()".

https://codereview.chromium.org/1635753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:332:
public Snackbar peek() {
Simpler:

    return !mStack.isEmpty() ? mStack.peek() : mQueue.peek();

Also, naming this "currentSnackbar()" might make the code clearer.

https://codereview.chromium.org/1635753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:353:
public boolean removeWithData(SnackbarController controller) {
call this "removeSnackbars" or "removeMatchingSnackbars"?

https://codereview.chromium.org/1635753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:380:
private static boolean compareData(Object a, Object b) {
If only we could use Objects.equals()... but it's not available before KitKat.

Also, how about naming this "objectsAreEqual()" so that it's clear what the
return value means without having to read the code.

[Ian Wen, 2016-01-27 01:37:12]

Thanks for the advice. All addressed. PTAL

https://codereview.chromium.org/1635753002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java
(right):

https://codereview.chromium.org/1635753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:81:
private SnackbarContainer mContainer = new SnackbarContainer();
On 2016/01/26 21:26:45, newt wrote:
> How about s/mContainer/mSnackbars/

Done.

https://codereview.chromium.org/1635753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:305:
private static class SnackbarContainer {
On 2016/01/26 21:26:45, newt wrote:
> I don't think "Container" is the right word here. How about "SnackbarQueue" or
> "SnackbarList"?

Queue or List is a specific data structure in Java... How about
SnackbarCollection?

https://codereview.chromium.org/1635753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:318:
* @return The next snackbar to display.
On 2016/01/26 21:26:45, newt wrote:
> Is this really "the next snackbar to display"? Isn't this just the currently
> displayed snackbar (just like peek())?
> 
> Perhaps this method should be renamed "removeCurrentSnackbar()".

Done.

https://codereview.chromium.org/1635753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:332:
public Snackbar peek() {
On 2016/01/26 21:26:45, newt wrote:
> Simpler:
> 
>     return !mStack.isEmpty() ? mStack.peek() : mQueue.peek();
> 
> Also, naming this "currentSnackbar()" might make the code clearer.

Done.

https://codereview.chromium.org/1635753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:353:
public boolean removeWithData(SnackbarController controller) {
On 2016/01/26 21:26:45, newt wrote:
> call this "removeSnackbars" or "removeMatchingSnackbars"?

Done.

https://codereview.chromium.org/1635753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:380:
private static boolean compareData(Object a, Object b) {
On 2016/01/26 21:26:45, newt wrote:
> If only we could use Objects.equals()... but it's not available before KitKat.
> 
> Also, how about naming this "objectsAreEqual()" so that it's clear what the
> return value means without having to read the code.

Done.

[newt (away), 2016-01-27 02:13:41]

Thanks, Ian. I think this is a solid improvement :)

https://codereview.chromium.org/1635753002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java
(right):

https://codereview.chromium.org/1635753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:305:
private static class SnackbarContainer {
On 2016/01/27 01:37:12, Ian Wen wrote:
> On 2016/01/26 21:26:45, newt wrote:
> > I don't think "Container" is the right word here. How about "SnackbarQueue"
or
> > "SnackbarList"?
> 
> Queue or List is a specific data structure in Java... How about
> SnackbarCollection?

Sounds good. "Queue" can have alternate meanings (e.g. priority queue), so I
thought it could maybe be stretched here. But "collection" is clear and probably
less confusing.

https://codereview.chromium.org/1635753002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/snackbar/Snackbar.java
(right):

https://codereview.chromium.org/1635753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/Snackbar.java:166:
boolean isOnStack() {
This class shouldn't both with details about stacks and queues; it should just
know about snackbar types. So, how about calling this method "isTypeAction()" or
just "getType()"

https://codereview.chromium.org/1635753002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java
(right):

https://codereview.chromium.org/1635753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:85:
mSnackbars.jumpToQueue();
Instead of calling jumpToQueue(), I'd call something like
mSnackbars().removeCurrentDueToTimeout().

https://codereview.chromium.org/1635753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:166:
if (mPopup == null) return false;
or just "return mPopup != null && mPopup.isShowing()"

https://codereview.chromium.org/1635753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:198:
* If {@link #mSnackbars} is empty, dismisses the popup; otherwise it updates the
popup
"Updates the snackbar popup window to reflect the value of
mSnackbars.currentSnackbar(), which may be null. This may show, change, or hide
the popup."

https://codereview.chromium.org/1635753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:211:
boolean refreshTimeout = true;
or you could call this "popupChanged". That's a more accurate name (since this
also controls announcing for accessibility) and makes the comment below ("Only
refresh timeout when the popup's content has changed") unnecessary.

https://codereview.chromium.org/1635753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:278:
if (getCurrent() != null && !getCurrent().isOnStack()) {
this subtle behavior is worthy of an explanatory comment

https://codereview.chromium.org/1635753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:287:
public void removeCurrent(boolean isAction) {
I'd explain what "isAction" means

https://codereview.chromium.org/1635753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:288:
Snackbar current = null;
simpler:

    Snackbar current = !mStack.isEmpty() ? mStack.pop() : mQueue.poll();

https://codereview.chromium.org/1635753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:323:
public void jumpToQueue() {
This method should be internal -- SnackbarManager shouldn't know about the Queue
vs Stack snackbars.

https://codereview.chromium.org/1635753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:324:
Snackbar current = getCurrent();
nit: remove these two lines. This function could be

    removeCurrent(false);
    Snackbar current;
    while (...) { ... }

https://codereview.chromium.org/1635753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:333:
boolean isFound = false;
s/isFound/snackbarRemoved/

https://codereview.chromium.org/1635753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:346:
boolean isFound = false;
s/isFound/snackbarRemoved/

https://codereview.chromium.org/1635753002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarPopupWindow.java
(right):

https://codereview.chromium.org/1635753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarPopupWindow.java:65:
/**
newline before this

[Ian Wen, 2016-01-27 18:14:07]

https://chromiumcodereview.appspot.com/1635753002/diff/20001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/snackbar/Snackbar.java
(right):

https://chromiumcodereview.appspot.com/1635753002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/Snackbar.java:166:
boolean isOnStack() {
On 2016/01/27 02:13:40, newt wrote:
> This class shouldn't both with details about stacks and queues; it should just
> know about snackbar types. So, how about calling this method "isTypeAction()"
or
> just "getType()"

Done.

https://chromiumcodereview.appspot.com/1635753002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java
(right):

https://chromiumcodereview.appspot.com/1635753002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:85:
mSnackbars.jumpToQueue();
On 2016/01/27 02:13:41, newt wrote:
> Instead of calling jumpToQueue(), I'd call something like
> mSnackbars().removeCurrentDueToTimeout().

Done.

https://chromiumcodereview.appspot.com/1635753002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:166:
if (mPopup == null) return false;
On 2016/01/27 02:13:40, newt wrote:
> or just "return mPopup != null && mPopup.isShowing()"

Done.

https://chromiumcodereview.appspot.com/1635753002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:198:
* If {@link #mSnackbars} is empty, dismisses the popup; otherwise it updates the
popup
On 2016/01/27 02:13:40, newt wrote:
> "Updates the snackbar popup window to reflect the value of
> mSnackbars.currentSnackbar(), which may be null. This may show, change, or
hide
> the popup."

Done.

https://chromiumcodereview.appspot.com/1635753002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:211:
boolean refreshTimeout = true;
On 2016/01/27 02:13:40, newt wrote:
> or you could call this "popupChanged". That's a more accurate name (since this
> also controls announcing for accessibility) and makes the comment below ("Only
> refresh timeout when the popup's content has changed") unnecessary.

Done.

https://chromiumcodereview.appspot.com/1635753002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:278:
if (getCurrent() != null && !getCurrent().isOnStack()) {
On 2016/01/27 02:13:40, newt wrote:
> this subtle behavior is worthy of an explanatory comment

Done.

https://chromiumcodereview.appspot.com/1635753002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:287:
public void removeCurrent(boolean isAction) {
On 2016/01/27 02:13:40, newt wrote:
> I'd explain what "isAction" means

Done.

https://chromiumcodereview.appspot.com/1635753002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:288:
Snackbar current = null;
On 2016/01/27 02:13:40, newt wrote:
> simpler:
> 
>     Snackbar current = !mStack.isEmpty() ? mStack.pop() : mQueue.poll();

Done.

https://chromiumcodereview.appspot.com/1635753002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:323:
public void jumpToQueue() {
On 2016/01/27 02:13:40, newt wrote:
> This method should be internal -- SnackbarManager shouldn't know about the
Queue
> vs Stack snackbars.

Renamed it to removeCurrentDueToTimeout().

https://chromiumcodereview.appspot.com/1635753002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:324:
Snackbar current = getCurrent();
On 2016/01/27 02:13:41, newt wrote:
> nit: remove these two lines. This function could be
> 
>     removeCurrent(false);
>     Snackbar current;
>     while (...) { ... }
>     

Done.

https://chromiumcodereview.appspot.com/1635753002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:333:
boolean isFound = false;
On 2016/01/27 02:13:40, newt wrote:
> s/isFound/snackbarRemoved/

Done.

https://chromiumcodereview.appspot.com/1635753002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarManager.java:346:
boolean isFound = false;
On 2016/01/27 02:13:40, newt wrote:
> s/isFound/snackbarRemoved/

Done.

https://chromiumcodereview.appspot.com/1635753002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarPopupWindow.java
(right):

https://chromiumcodereview.appspot.com/1635753002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarPopupWindow.java:65:
/**
On 2016/01/27 02:13:41, newt wrote:
> newline before this

Done.

[newt (away), 2016-01-27 18:39:13]

lgtm

[Ian Wen, 2016-01-27 19:07:16]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2016-01-27 19:07:17]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1635753002/#ps60001
(title: "rebase")

[Ian Wen, 2016-01-27 19:09:29]

The CQ bit was unchecked by ianwen@chromium.org

[Ian Wen, 2016-01-27 19:14:23]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2016-01-27 19:14:24]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1635753002/#ps40002
(title: "make it compile")

[commit-bot: I haz the power, 2016-01-27 19:14:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1635753002/40002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1635753002/40002

[commit-bot: I haz the power, 2016-01-27 21:25:23]

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

[commit-bot: I haz the power, 2016-01-27 21:25:24]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ian Wen, 2016-01-27 21:45:06]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2016-01-27 21:45:06]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1635753002/#ps90001
(title: "add VisibleForTesting to a method")

[commit-bot: I haz the power, 2016-01-27 21:45:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1635753002/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1635753002/90001

[commit-bot: I haz the power, 2016-01-27 22:37:50]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2016-01-27 22:39:07]

Description was changed from

==========
Introduce Queue-Based Notification Snackbars

Notification snackbars are low-priority snackbars that are stored in a
queue, contrary to tranditional "action" snackbars that ought to be
shown immediately and are stored in a stack.

When a queue snackbar is showing, a stack snackbar can always kick in
and override the snackbar; however when a stack snackbar is showing, a
queue snackbar will wait till the stack is cleared.

Queue snackbars are not dismissed in batch; instead, they will be shown
one by one, in FIFO order.

BUG=579347
==========

to

==========
Introduce Queue-Based Notification Snackbars

Notification snackbars are low-priority snackbars that are stored in a
queue, contrary to tranditional "action" snackbars that ought to be
shown immediately and are stored in a stack.

When a queue snackbar is showing, a stack snackbar can always kick in
and override the snackbar; however when a stack snackbar is showing, a
queue snackbar will wait till the stack is cleared.

Queue snackbars are not dismissed in batch; instead, they will be shown
one by one, in FIFO order.

BUG=579347

Committed: https://crrev.com/d117eb1c195ee27a75c12bde155438a13955ec0c
Cr-Commit-Position: refs/heads/master@{#371899}
==========

[commit-bot: I haz the power, 2016-01-27 22:39:07]

Patchset 6 (id:??) landed as
https://crrev.com/d117eb1c195ee27a75c12bde155438a13955ec0c
Cr-Commit-Position: refs/heads/master@{#371899}