Add two instrumentation tests for Web Notifications on Android.

Add two instrumentation tests for Web Notifications on Android.

Introduce two tests for validating that Web Notifications on Android have
the expected contents, appearance and behavior, and that notifications
which have an icon supplied get this icon fetched and included in the
notification as a Bitmap.

BUG=462251
Committed: https://crrev.com/2eff2ee0af365c531916807f7e075ca1bbf4f95f
Cr-Commit-Position: refs/heads/master@{#318319}

[Peter Beverloo, 2015-02-26 16:46:36]

peter@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org

[Peter Beverloo, 2015-02-26 16:46:37]

+mvanouwerkerk for review
+miguelg for OWNERS

[Michael van Ouwerkerk, 2015-02-26 17:24:24]

Just nits really. Nice tests. lgtm

https://codereview.chromium.org/960923004/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java
(right):

https://codereview.chromium.org/960923004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java:16:
public void cancel(int id);
nit: Java interface methods are public by default, you can omit it here and
below

https://codereview.chromium.org/960923004/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java
(right):

https://codereview.chromium.org/960923004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:78:
public static void overrideNotificationManager(NotificationManagerProxy
notificationManager) {
nit: I think this should be called overrideNotificationManagerForTesting

https://codereview.chromium.org/960923004/diff/1/chrome/android/javatests/src...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/MockNotificationManagerProxy.java
(right):

https://codereview.chromium.org/960923004/diff/1/chrome/android/javatests/src...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/MockNotificationManagerProxy.java:18:
private SparseArray<Notification> mNotifications;
nit: final

https://codereview.chromium.org/960923004/diff/1/chrome/android/javatests/src...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/MockNotificationManagerProxy.java:19:
private Map<String, Integer> mTags;
nit: final

https://codereview.chromium.org/960923004/diff/1/chrome/android/javatests/src...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/MockNotificationManagerProxy.java:23:
mNotifications = new SparseArray<Notification>();
nit: just new SparseArray<>();

https://codereview.chromium.org/960923004/diff/1/chrome/android/javatests/src...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/MockNotificationManagerProxy.java:24:
mTags = new HashMap<String, Integer>();
nit: just new HashMap<>();

https://codereview.chromium.org/960923004/diff/1/chrome/android/javatests/src...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java
(right):

https://codereview.chromium.org/960923004/diff/1/chrome/android/javatests/src...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java:36:
private void setNotificationContentSettingForCurrentOrigin(final ContentSetting
setting) {
nit: this looks like it can be static

https://codereview.chromium.org/960923004/diff/1/chrome/android/javatests/src...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java:42:
PushNotificationInfo pushNotificationInfo = new PushNotificationInfo(origin,
"");
Hm, does "" mean "*" for embedder here? Maybe extract a constant or add a
comment.

https://codereview.chromium.org/960923004/diff/1/chrome/test/data/notificatio...
File chrome/test/data/notifications/android_test.html (right):

https://codereview.chromium.org/960923004/diff/1/chrome/test/data/notificatio...
chrome/test/data/notifications/android_test.html:13: options = options || {};
Is it not ok to pass undefined?

https://codereview.chromium.org/960923004/diff/1/chrome/test/data/notificatio...
File chrome/test/data/notifications/android_test_worker.js (right):

https://codereview.chromium.org/960923004/diff/1/chrome/test/data/notificatio...
chrome/test/data/notifications/android_test_worker.js:12:
addEventListener('notificationclick', function(event) {
This doesn't do anything. Why is it included?

[Michael van Ouwerkerk, 2015-02-26 17:25:13]

mvanouwerkerk@chromium.org changed reviewers:
+ miguelg@chromium.org

[Michael van Ouwerkerk, 2015-02-26 17:25:14]

Miguel: I think Peter meant to add you as an owner reviewer.

[Peter Beverloo, 2015-02-26 18:22:39]

Thanks for the review!

https://codereview.chromium.org/960923004/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java
(right):

https://codereview.chromium.org/960923004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java:16:
public void cancel(int id);
On 2015/02/26 17:24:23, Michael van Ouwerkerk wrote:
> nit: Java interface methods are public by default, you can omit it here and
> below

Yup, but I like them because they make this explicit. Default visibility is
different per programming language.

https://codereview.chromium.org/960923004/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java
(right):

https://codereview.chromium.org/960923004/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:78:
public static void overrideNotificationManager(NotificationManagerProxy
notificationManager) {
On 2015/02/26 17:24:23, Michael van Ouwerkerk wrote:
> nit: I think this should be called overrideNotificationManagerForTesting

Done.

https://codereview.chromium.org/960923004/diff/1/chrome/android/javatests/src...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/MockNotificationManagerProxy.java
(right):

https://codereview.chromium.org/960923004/diff/1/chrome/android/javatests/src...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/MockNotificationManagerProxy.java:18:
private SparseArray<Notification> mNotifications;
On 2015/02/26 17:24:23, Michael van Ouwerkerk wrote:
> nit: final

Done.

https://codereview.chromium.org/960923004/diff/1/chrome/android/javatests/src...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/MockNotificationManagerProxy.java:19:
private Map<String, Integer> mTags;
On 2015/02/26 17:24:23, Michael van Ouwerkerk wrote:
> nit: final

Done.

https://codereview.chromium.org/960923004/diff/1/chrome/android/javatests/src...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/MockNotificationManagerProxy.java:23:
mNotifications = new SparseArray<Notification>();
On 2015/02/26 17:24:23, Michael van Ouwerkerk wrote:
> nit: just new SparseArray<>();

Done.

https://codereview.chromium.org/960923004/diff/1/chrome/android/javatests/src...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/MockNotificationManagerProxy.java:24:
mTags = new HashMap<String, Integer>();
On 2015/02/26 17:24:23, Michael van Ouwerkerk wrote:
> nit: just new HashMap<>();

Done.

https://codereview.chromium.org/960923004/diff/1/chrome/android/javatests/src...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java
(right):

https://codereview.chromium.org/960923004/diff/1/chrome/android/javatests/src...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java:36:
private void setNotificationContentSettingForCurrentOrigin(final ContentSetting
setting) {
On 2015/02/26 17:24:23, Michael van Ouwerkerk wrote:
> nit: this looks like it can be static

Making this static requires me to wrap it, which doesn't help readability.
Static or not static doesn't really matter here, so I'd prefer to optimize for
readability - do you feel strongly?

https://codereview.chromium.org/960923004/diff/1/chrome/android/javatests/src...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java:42:
PushNotificationInfo pushNotificationInfo = new PushNotificationInfo(origin,
"");
On 2015/02/26 17:24:23, Michael van Ouwerkerk wrote:
> Hm, does "" mean "*" for embedder here? Maybe extract a constant or add a
> comment.

Added a comment.

https://codereview.chromium.org/960923004/diff/1/chrome/test/data/notificatio...
File chrome/test/data/notifications/android_test.html (right):

https://codereview.chromium.org/960923004/diff/1/chrome/test/data/notificatio...
chrome/test/data/notifications/android_test.html:13: options = options || {};
On 2015/02/26 17:24:23, Michael van Ouwerkerk wrote:
> Is it not ok to pass undefined?

It is. Removed.

https://codereview.chromium.org/960923004/diff/1/chrome/test/data/notificatio...
File chrome/test/data/notifications/android_test_worker.js (right):

https://codereview.chromium.org/960923004/diff/1/chrome/test/data/notificatio...
chrome/test/data/notifications/android_test_worker.js:12:
addEventListener('notificationclick', function(event) {
On 2015/02/26 17:24:23, Michael van Ouwerkerk wrote:
> This doesn't do anything. Why is it included?

Took it out. Future tests will need it.

[Michael van Ouwerkerk, 2015-02-26 19:12:51]

still looks good to me

https://codereview.chromium.org/960923004/diff/1/chrome/android/javatests/src...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java
(right):

https://codereview.chromium.org/960923004/diff/1/chrome/android/javatests/src...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java:36:
private void setNotificationContentSettingForCurrentOrigin(final ContentSetting
setting) {
On 2015/02/26 18:22:38, Peter Beverloo wrote:
> On 2015/02/26 17:24:23, Michael van Ouwerkerk wrote:
> > nit: this looks like it can be static
> 
> Making this static requires me to wrap it, which doesn't help readability.
> Static or not static doesn't really matter here, so I'd prefer to optimize for
> readability - do you feel strongly?

Since this is a test, I don't feel strongly. In other contexts it might matter
more in terms of readability.

[Miguel Garcia, 2015-02-26 19:20:18]

lgtm

This is really cool stuff Peter.

https://chromiumcodereview.appspot.com/960923004/diff/20001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java
(right):

https://chromiumcodereview.appspot.com/960923004/diff/20001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java:16:
public void cancel(int id);
I also find labeling the methods of an interface public strange (what would they
be given it is an interface?). However it seems it is used inconsistently anyway
so ok.

https://chromiumcodereview.appspot.com/960923004/diff/20001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java
(right):

https://chromiumcodereview.appspot.com/960923004/diff/20001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:43:
private static NotificationManagerProxy sNotificationManagerOverride;
don't we usually call this kind of objects delegates?

https://chromiumcodereview.appspot.com/960923004/diff/20001/chrome/android/ja...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/MockNotificationManagerProxy.java
(right):

https://chromiumcodereview.appspot.com/960923004/diff/20001/chrome/android/ja...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/MockNotificationManagerProxy.java:19:
private final Map<String, Integer> mTags;
Nit: can you mention in a comment that this a tag -> notification ID map?

https://chromiumcodereview.appspot.com/960923004/diff/20001/chrome/android/ja...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/MockNotificationManagerProxy.java:29:
* Returns the notifications which are currently being managed by the
notification manager.
nit: can you use javadoc style? i.e @return and so?

https://chromiumcodereview.appspot.com/960923004/diff/20001/chrome/android/ja...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java
(right):

https://chromiumcodereview.appspot.com/960923004/diff/20001/chrome/android/ja...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java:96:
NotificationUIManager.overrideNotificationManagerForTesting(mMockNotificationManager);
I think it would make sense to add a tearDown where you do
NotificationUIManager.overrideNotificationManagerForTesting(null); since you are
supposed to clean up the mock yourself

https://chromiumcodereview.appspot.com/960923004/diff/20001/chrome/android/ja...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/OWNERS
(right):

https://chromiumcodereview.appspot.com/960923004/diff/20001/chrome/android/ja...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/OWNERS:1:
peter@chromium.org
would it be ok to add Michael as an owner too? It's always good to have more
than one just in case and since he is reviewing the directory...

[Peter Beverloo, 2015-02-26 19:34:17]

Thanks for the reviews!

https://codereview.chromium.org/960923004/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java
(right):

https://codereview.chromium.org/960923004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java:16:
public void cancel(int id);
On 2015/02/26 19:20:18, Miguel Garcia wrote:
> I also find labeling the methods of an interface public strange (what would
they
> be given it is an interface?). However it seems it is used inconsistently
anyway
> so ok.

OK. I'll trust your opinions about it so removed the "public" keywords.

https://codereview.chromium.org/960923004/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java
(right):

https://codereview.chromium.org/960923004/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:43:
private static NotificationManagerProxy sNotificationManagerOverride;
On 2015/02/26 19:20:18, Miguel Garcia wrote:
> don't we usually call this kind of objects delegates?

It's not a delegate, it's a proxy. The NotificationManagerProxy follows the
interface of the NotificationManager of the Android framework. The caller
shouldn't have to care.

https://codereview.chromium.org/960923004/diff/20001/chrome/android/javatests...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/MockNotificationManagerProxy.java
(right):

https://codereview.chromium.org/960923004/diff/20001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/MockNotificationManagerProxy.java:19:
private final Map<String, Integer> mTags;
On 2015/02/26 19:20:18, Miguel Garcia wrote:
> Nit: can you mention in a comment that this a tag -> notification ID map?

Done.

https://codereview.chromium.org/960923004/diff/20001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/MockNotificationManagerProxy.java:29:
* Returns the notifications which are currently being managed by the
notification manager.
On 2015/02/26 19:20:18, Miguel Garcia wrote:
> nit: can you use javadoc style? i.e @return and so?

We seem to be inconsistent - I prefer this. Nonetheless, I've changed it to
JavaDoc style. Out of interest, do we generate docs somewhere?

https://codereview.chromium.org/960923004/diff/20001/chrome/android/javatests...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java
(right):

https://codereview.chromium.org/960923004/diff/20001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java:96:
NotificationUIManager.overrideNotificationManagerForTesting(mMockNotificationManager);
On 2015/02/26 19:20:18, Miguel Garcia wrote:
> I think it would make sense to add a tearDown where you do
> NotificationUIManager.overrideNotificationManagerForTesting(null); since you
are
> supposed to clean up the mock yourself

aaaah :-). Done.

https://codereview.chromium.org/960923004/diff/20001/chrome/android/javatests...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/OWNERS
(right):

https://codereview.chromium.org/960923004/diff/20001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/OWNERS:1:
peter@chromium.org
On 2015/02/26 19:20:18, Miguel Garcia wrote:
> would it be ok to add Michael as an owner too? It's always good to have more
> than one just in case and since he is reviewing the directory...

Done.

[Peter Beverloo, 2015-02-26 20:42:00]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-02-26 20:42:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvanouwerkerk@chromium.org, miguelg@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/960923004/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-02-26 20:42:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/960923004/40001

[commit-bot: I haz the power, 2015-02-26 21:39:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-26 21:42:17]

Patchset 3 (id:??) landed as
https://crrev.com/2eff2ee0af365c531916807f7e075ca1bbf4f95f
Cr-Commit-Position: refs/heads/master@{#318319}