Change Physical Web notification display behavior

Change Physical Web notification display behavior

Currently, we only display a notification when we go from 0 to 1
displayable URLs.  The reason for this is that we don't want to
needlessly displayed a notification if the user recently swiped it
away.  However, in a URL dense environment (e.g. a mall filled with
beacons), a user may not ever go from seeing 0 to 1 displayable URLs,
since they will always be around multiple.  A single swipe away will
then leave them never seeing another notification.

This change displays a notification if we go from seeing 0 to 1
displayable URLs *or* if we see a displayable URL that does not exist
in our cache (i.e. a URL that has not been encountered for 24 hours.)

This behavior could be further refined, but this is a positive step
toward finding a balance between being there when desired and not
needlessly renotifying if a user has swiped away a notification.

BUG=596668
Committed: https://crrev.com/44ac07c8316385250002371bec549fc6621826ec
Cr-Commit-Position: refs/heads/master@{#396060}

[cco3, 2016-05-11 23:30:03]

cco3@chromium.org changed reviewers:
+ mattreynolds@chromium.org

[mattreynolds, 2016-05-11 23:45:11]

https://codereview.chromium.org/1969983002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java
(right):

https://codereview.chromium.org/1969983002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:152:
if (!PhysicalWeb.isOnboarding(mContext) &&
!mResolvedUrls.contains(urlInfo.getUrl())) {
If we save the isOnboarding state here we can avoid checking it again below.

https://codereview.chromium.org/1969983002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:416:
private void recordNotificationUpdate() {
Unused?

[cco3, 2016-05-12 00:11:50]

https://codereview.chromium.org/1969983002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java
(right):

https://codereview.chromium.org/1969983002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:152:
if (!PhysicalWeb.isOnboarding(mContext) &&
!mResolvedUrls.contains(urlInfo.getUrl())) {
On 2016/05/11 23:45:11, mattreynolds wrote:
> If we save the isOnboarding state here we can avoid checking it again below.

Done.

https://codereview.chromium.org/1969983002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:416:
private void recordNotificationUpdate() {
On 2016/05/11 23:45:11, mattreynolds wrote:
> Unused?

Done.

[mattreynolds, 2016-05-12 00:13:47]

lgtm

[cco3, 2016-05-12 00:15:33]

cco3@chromium.org changed reviewers:
+ nyquist@chromium.org

[nyquist, 2016-05-12 20:15:54]

nyquist@chromium.org changed reviewers:
+ tedchoc@chromium.org

[nyquist, 2016-05-12 20:15:55]

tedchoc: From a UX perspective, are we OK with the change described in the CL
description and in the bug?

https://codereview.chromium.org/1969983002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java
(right):

https://codereview.chromium.org/1969983002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:170:
boolean isOnboarding = !PhysicalWeb.isOnboarding(mContext);
This variable name is very confusing based on the ! in the right hand side.
Also, is it even used?

https://codereview.chromium.org/1969983002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/UrlManagerTest.java
(right):

https://codereview.chromium.org/1969983002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/UrlManagerTest.java:203:
// Make sure that a notification was shown.
I think thas a typo and you mean: was not shown

[Ted C, 2016-05-12 21:19:40]

On 2016/05/12 20:15:55, nyquist wrote:
> tedchoc: From a UX perspective, are we OK with the change described in the CL
> description and in the bug?

While I definitely understand the reasoning of the change, the potential
impact to Chrome as a whole is somewhat alarming.

Anytime you are going to change your notification policy, I think it should
be done with more scrutiny.  The potential for user annoyance seems roughly
equal to the potential for user satisfaction.  In a dense beacon area, this
could result in never being able to dismiss the beacon notifications.

And sadly, disabling notifications for all of chrome is roughly the same
(in terms of ease) as disabling notifications for just the physical web.

I think this should be discussed with the larger Chrome on Android team before
we go ahead and land this.

> 
>
https://codereview.chromium.org/1969983002/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java
> (right):
> 
>
https://codereview.chromium.org/1969983002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:170:
> boolean isOnboarding = !PhysicalWeb.isOnboarding(mContext);
> This variable name is very confusing based on the ! in the right hand side.
> Also, is it even used?
> 
>
https://codereview.chromium.org/1969983002/diff/40001/chrome/android/javatest...
> File
>
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/UrlManagerTest.java
> (right):
> 
>
https://codereview.chromium.org/1969983002/diff/40001/chrome/android/javatest...
>
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/UrlManagerTest.java:203:
> // Make sure that a notification was shown.
> I think thas a typo and you mean: was not shown

[scottj, 2016-05-12 22:41:47]

I don't think this change is quite as dramatic as you fear. It's important to
know how limited our notification scheme is:

1) The scan will only occur on screen wake up.
This means there will no additional notifications coming into the device as
you're walking around. The same is true while it is in your pocket: no
notifications will be generated while it is asleep.

2) Notifications are MIN priority.
Even when a notification does fire it will be soundless/buzzless so the user
will have no idea.

This means that the number of increased notifications will be lower than stated
above and when they do, the user shouldn't even notice. The whole point of our
notification approach is that they only really have value to the user when they
proactively open the notification tray, usually in response to a Physical Web
logo on a poster/device.

The reason we're doing this is precisely because people in beacon dense
environments were dismissing all notifications at once and then wondering why
they couldn't see the vending machine/poster etc. Our current approach is TOO
conservative making it impossible to see devices that you are standing right in
front of. We totally appreciate that we don't want to create additional or
irritating notifications, but that why we've taken the steps set out above to
make sure that isn't the case.

[scottj, 2016-05-13 18:25:08]

I just sent off an email to Clank-Leads to make sure they are apprised. We
should be able to continue.

[mattreynolds, 2016-05-24 01:19:05]

lgtm

[cco3, 2016-05-24 16:24:54]

nyquist@, whenever you are available to take a look at this, this is ready.

[nyquist, 2016-05-25 22:29:49]

lgtm

https://codereview.chromium.org/1969983002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java
(right):

https://codereview.chromium.org/1969983002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:406:
UrlInfo curUrlInfo = mUrlInfoMap.get(urlInfo.getUrl());
Nit: s/\bcur/current/g ?

[cco3, 2016-05-25 22:55:06]

https://codereview.chromium.org/1969983002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java
(right):

https://codereview.chromium.org/1969983002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:406:
UrlInfo curUrlInfo = mUrlInfoMap.get(urlInfo.getUrl());
On 2016/05/25 22:29:49, nyquist wrote:
> Nit: s/\bcur/current/g ?

Done.

[cco3, 2016-05-25 22:55:11]

The CQ bit was checked by cco3@chromium.org

[cco3, 2016-05-25 22:55:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, mattreynolds@chromium.org
Link to the patchset: https://codereview.chromium.org/1969983002/#ps120001
(title: "Spell out "current"")

[commit-bot: I haz the power, 2016-05-25 22:56:01]

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

[commit-bot: I haz the power, 2016-05-26 00:55:51]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-26 00:57:13]

Description was changed from

==========
Change Physical Web notification display behavior

Currently, we only display a notification when we go from 0 to 1
displayable URLs.  The reason for this is that we don't want to
needlessly displayed a notification if the user recently swiped it
away.  However, in a URL dense environment (e.g. a mall filled with
beacons), a user may not ever go from seeing 0 to 1 displayable URLs,
since they will always be around multiple.  A single swipe away will
then leave them never seeing another notification.

This change displays a notification if we go from seeing 0 to 1
displayable URLs *or* if we see a displayable URL that does not exist
in our cache (i.e. a URL that has not been encountered for 24 hours.)

This behavior could be further refined, but this is a positive step
toward finding a balance between being there when desired and not
needlessly renotifying if a user has swiped away a notification.

BUG=596668
==========

to

==========
Change Physical Web notification display behavior

Currently, we only display a notification when we go from 0 to 1
displayable URLs.  The reason for this is that we don't want to
needlessly displayed a notification if the user recently swiped it
away.  However, in a URL dense environment (e.g. a mall filled with
beacons), a user may not ever go from seeing 0 to 1 displayable URLs,
since they will always be around multiple.  A single swipe away will
then leave them never seeing another notification.

This change displays a notification if we go from seeing 0 to 1
displayable URLs *or* if we see a displayable URL that does not exist
in our cache (i.e. a URL that has not been encountered for 24 hours.)

This behavior could be further refined, but this is a positive step
toward finding a balance between being there when desired and not
needlessly renotifying if a user has swiped away a notification.

BUG=596668

Committed: https://crrev.com/44ac07c8316385250002371bec549fc6621826ec
Cr-Commit-Position: refs/heads/master@{#396060}
==========

[commit-bot: I haz the power, 2016-05-26 00:57:14]

Patchset 7 (id:??) landed as
https://crrev.com/44ac07c8316385250002371bec549fc6621826ec
Cr-Commit-Position: refs/heads/master@{#396060}