Add testing for subscription from service workers

Add testing for subscription from service workers

Testing covers subscription from workers both with and without keys, and in
situations where it triggers a read from a manifest or a pre-subscribed worker.

This also cleaned up the unregistration, so that any registrations are searched
for, instead of using a variable to store the registration.

BUG=596471
Committed: https://crrev.com/2a019912946921d445189c61af9c1ab5c3cef860
Cr-Commit-Position: refs/heads/master@{#384332}

[harkness, 2016-03-21 15:05:01]

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

[harkness, 2016-03-21 15:05:02]

Adding tests for subscription from service workers.

https://codereview.chromium.org/1816123002/diff/20001/chrome/test/data/push_m...
File chrome/test/data/push_messaging/push_test.js (right):

https://codereview.chromium.org/1816123002/diff/20001/chrome/test/data/push_m...
chrome/test/data/push_messaging/push_test.js:199:
navigator.serviceWorker.ready.then(function(swRegistration) {
Previously unsubscribe used a variable that was set to be the current
subscription. That was brittle to page reloads, so I replaced it with the
following code that looks up the subscription.

[Michael van Ouwerkerk, 2016-03-22 11:31:38]

Tests, nice!

https://codereview.chromium.org/1816123002/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/1816123002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:412:
IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, WorkerSubscribeSuccessful) {
Not all attempts in this test are successful. Maybe name it something like
SubscribeFromWorker?

https://codereview.chromium.org/1816123002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:433:
ASSERT_TRUE(RunScript("subscribePushWithoutKey()", &script_result));
For clarity it might help to rename subscribePushWithoutKey to
documentSubscribePushWithoutKey (and similar in other places if we have a worker
and a document version of the function).

https://codereview.chromium.org/1816123002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:441: // In this
case, the key will be read from the datastore.
I think no application server key has been provided yet at this point, only a
sender_id through the manifest. What is read from the datastore? Let's be more
explicit, and replace "key" with "application server key" in a few places, and
distinguish it from a sender id even though it is stored in the same place.

https://codereview.chromium.org/1816123002/diff/20001/chrome/test/data/push_m...
File chrome/test/data/push_messaging/push_test.js (right):

https://codereview.chromium.org/1816123002/diff/20001/chrome/test/data/push_m...
chrome/test/data/push_messaging/push_test.js:158: function
workerSubscribePermissionFail() {
This function appears to be unused.

https://codereview.chromium.org/1816123002/diff/20001/chrome/test/data/push_m...
chrome/test/data/push_messaging/push_test.js:159: // Attempt to subscribe with
invalid useVisibleOnly flag
Is |useVisibleOnly| an intentional misspelling then? Might want to highlight
that some more :-)

https://codereview.chromium.org/1816123002/diff/20001/chrome/test/data/push_m...
File chrome/test/data/push_messaging/service_worker.js (right):

https://codereview.chromium.org/1816123002/diff/20001/chrome/test/data/push_m...
chrome/test/data/push_messaging/service_worker.js:7: var applicationServerKey =
new Uint8Array([
To avoid duplication of this inscrutable thing you could move it to a shared
util file and use importScripts [1] to include it here, and a regular script tag
to include it in the test page.

[1]
https://developer.mozilla.org/en-US/docs/Web/API/WorkerGlobalScope/importScripts

[harkness, 2016-03-24 16:56:24]

Separating out the tests revealed two additional issues with the code. First,
the previous WebString conversion wasn't doing the right thing with some
strings, so I'm now using a latin1 conversion to just copy things byte by byte. 

Once that was fixed, it revealed two subtle differences between subscribing from
a document and a worker which I updated, one in permissions and one in writing
to the document store.

https://codereview.chromium.org/1816123002/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/1816123002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:412:
IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, WorkerSubscribeSuccessful) {
On 2016/03/22 11:31:37, Michael van Ouwerkerk wrote:
> Not all attempts in this test are successful. Maybe name it something like
> SubscribeFromWorker?

Done.

https://codereview.chromium.org/1816123002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:433:
ASSERT_TRUE(RunScript("subscribePushWithoutKey()", &script_result));
On 2016/03/22 11:31:37, Michael van Ouwerkerk wrote:
> For clarity it might help to rename subscribePushWithoutKey to
> documentSubscribePushWithoutKey (and similar in other places if we have a
worker
> and a document version of the function).

Done.

https://codereview.chromium.org/1816123002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:441: // In this
case, the key will be read from the datastore.
On 2016/03/22 11:31:37, Michael van Ouwerkerk wrote:
> I think no application server key has been provided yet at this point, only a
> sender_id through the manifest. What is read from the datastore? Let's be more
> explicit, and replace "key" with "application server key" in a few places, and
> distinguish it from a sender id even though it is stored in the same place.

Good point! In fact, this was hiding an actual code issue, which was also missed
in the unit tests. Tests have been expanded appropriately.

https://codereview.chromium.org/1816123002/diff/20001/chrome/test/data/push_m...
File chrome/test/data/push_messaging/push_test.js (right):

https://codereview.chromium.org/1816123002/diff/20001/chrome/test/data/push_m...
chrome/test/data/push_messaging/push_test.js:158: function
workerSubscribePermissionFail() {
On 2016/03/22 11:31:37, Michael van Ouwerkerk wrote:
> This function appears to be unused.

Removed. It was duplicate testing anyway.

https://codereview.chromium.org/1816123002/diff/20001/chrome/test/data/push_m...
chrome/test/data/push_messaging/push_test.js:159: // Attempt to subscribe with
invalid useVisibleOnly flag
On 2016/03/22 11:31:37, Michael van Ouwerkerk wrote:
> Is |useVisibleOnly| an intentional misspelling then? Might want to highlight
> that some more :-)

Acknowledged.

https://codereview.chromium.org/1816123002/diff/20001/chrome/test/data/push_m...
File chrome/test/data/push_messaging/service_worker.js (right):

https://codereview.chromium.org/1816123002/diff/20001/chrome/test/data/push_m...
chrome/test/data/push_messaging/service_worker.js:7: var applicationServerKey =
new Uint8Array([
On 2016/03/22 11:31:37, Michael van Ouwerkerk wrote:
> To avoid duplication of this inscrutable thing you could move it to a shared
> util file and use importScripts [1] to include it here, and a regular script
tag
> to include it in the test page.
> 
> [1]
>
https://developer.mozilla.org/en-US/docs/Web/API/WorkerGlobalScope/importScripts

Done.

[Michael van Ouwerkerk, 2016-03-30 08:51:56]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-30 08:52:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1816123002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1816123002/60001

[commit-bot: I haz the power, 2016-03-30 09:34:22]

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

[commit-bot: I haz the power, 2016-03-30 09:34:23]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Michael van Ouwerkerk, 2016-03-30 09:50:05]

https://codereview.chromium.org/1816123002/diff/60001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/1816123002/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:329:
EXPECT_EQ(reinterpret_cast<const char*>(kApplicationServerKey),
nit: a small helper function like "getTestApplicationServerKey" might aid
readability of this file.

https://codereview.chromium.org/1816123002/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:495: // In this
case, the key will be read from the datastore.
nit: be a bit more explicit by calling out this is the sender id: "the key
(sender id)"

https://codereview.chromium.org/1816123002/diff/60001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_permission_context.cc (left):

https://codereview.chromium.org/1816123002/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_permission_context.cc:57:
push_content_setting == CONTENT_SETTING_ASK) {
Does this change any of our current behavior?

https://codereview.chromium.org/1816123002/diff/60001/chrome/test/data/push_m...
File chrome/test/data/push_messaging/service_worker.js (right):

https://codereview.chromium.org/1816123002/diff/60001/chrome/test/data/push_m...
chrome/test/data/push_messaging/service_worker.js:50: } else if
(event.data.command == 'workerSubscribePermissionFail') {
This case appears to be unused?

https://codereview.chromium.org/1816123002/diff/60001/chrome/test/data/push_m...
File chrome/test/data/push_messaging/test.html (right):

https://codereview.chromium.org/1816123002/diff/60001/chrome/test/data/push_m...
chrome/test/data/push_messaging/test.html:6: <script
src="push_constants.js"></script>
You'll need this also in test_no_subscription_options.html to get all tests to
pass, like this failure:

https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64...

https://codereview.chromium.org/1816123002/diff/60001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/1816123002/diff/60001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:302:
service_worker_context_->StoreRegistrationUserData(
Yikes! Glad we got that one.

https://codereview.chromium.org/1816123002/diff/60001/content/child/push_mess...
File content/child/push_messaging/push_provider.cc (right):

https://codereview.chromium.org/1816123002/diff/60001/content/child/push_mess...
content/child/push_messaging/push_provider.cc:81: content_options.sender_info =
options.applicationServerKey.latin1();
Could you please document why this must be latin1 instead of utf8?

https://codereview.chromium.org/1816123002/diff/60001/content/renderer/push_m...
File content/renderer/push_messaging/push_messaging_dispatcher.cc (right):

https://codereview.chromium.org/1816123002/diff/60001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.cc:58:
content_options.sender_info = options.applicationServerKey.latin1();
Same here.

https://codereview.chromium.org/1816123002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/1816123002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:57: return
WebString::fromLatin1(input, length);
Or maybe only here. Maybe find a single best place for documenting why this
needs to be latin1 :-)

https://codereview.chromium.org/1816123002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp
(right):

https://codereview.chromium.org/1816123002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp:32: uint8_t
senderKey[kValidKeyLength];
This is unused now.

[harkness, 2016-03-30 10:51:49]

https://codereview.chromium.org/1816123002/diff/60001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/1816123002/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:329:
EXPECT_EQ(reinterpret_cast<const char*>(kApplicationServerKey),
On 2016/03/30 09:50:04, Michael van Ouwerkerk wrote:
> nit: a small helper function like "getTestApplicationServerKey" might aid
> readability of this file.

Done.

https://codereview.chromium.org/1816123002/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:495: // In this
case, the key will be read from the datastore.
On 2016/03/30 09:50:04, Michael van Ouwerkerk wrote:
> nit: be a bit more explicit by calling out this is the sender id: "the key
> (sender id)"

Done.

https://codereview.chromium.org/1816123002/diff/60001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_permission_context.cc (left):

https://codereview.chromium.org/1816123002/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_permission_context.cc:57:
push_content_setting == CONTENT_SETTING_ASK) {
On 2016/03/30 09:50:04, Michael van Ouwerkerk wrote:
> Does this change any of our current behavior?

This makes the current behavior for registering from a document consistent with
registering from a worker. This wouldn't have been hit before, because the
document registration would grant both permissions before a worker
re-registration queried it.

https://codereview.chromium.org/1816123002/diff/60001/chrome/test/data/push_m...
File chrome/test/data/push_messaging/service_worker.js (right):

https://codereview.chromium.org/1816123002/diff/60001/chrome/test/data/push_m...
chrome/test/data/push_messaging/service_worker.js:50: } else if
(event.data.command == 'workerSubscribePermissionFail') {
On 2016/03/30 09:50:04, Michael van Ouwerkerk wrote:
> This case appears to be unused?

Done.

https://codereview.chromium.org/1816123002/diff/60001/chrome/test/data/push_m...
File chrome/test/data/push_messaging/test.html (right):

https://codereview.chromium.org/1816123002/diff/60001/chrome/test/data/push_m...
chrome/test/data/push_messaging/test.html:6: <script
src="push_constants.js"></script>
On 2016/03/30 09:50:04, Michael van Ouwerkerk wrote:
> You'll need this also in test_no_subscription_options.html to get all tests to
> pass, like this failure:
> 
>
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64...

Done.

https://codereview.chromium.org/1816123002/diff/60001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/1816123002/diff/60001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:302:
service_worker_context_->StoreRegistrationUserData(
On 2016/03/30 09:50:04, Michael van Ouwerkerk wrote:
> Yikes! Glad we got that one.

yup!  (although ideally we'd completely remove the need for this, but it would
have required rewriting a lot to remove it.)

https://codereview.chromium.org/1816123002/diff/60001/content/child/push_mess...
File content/child/push_messaging/push_provider.cc (right):

https://codereview.chromium.org/1816123002/diff/60001/content/child/push_mess...
content/child/push_messaging/push_provider.cc:81: content_options.sender_info =
options.applicationServerKey.latin1();
On 2016/03/30 09:50:04, Michael van Ouwerkerk wrote:
> Could you please document why this must be latin1 instead of utf8?

Done.

https://codereview.chromium.org/1816123002/diff/60001/content/renderer/push_m...
File content/renderer/push_messaging/push_messaging_dispatcher.cc (right):

https://codereview.chromium.org/1816123002/diff/60001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.cc:58:
content_options.sender_info = options.applicationServerKey.latin1();
On 2016/03/30 09:50:04, Michael van Ouwerkerk wrote:
> Same here.

Done.

https://codereview.chromium.org/1816123002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/1816123002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManager.cpp:57: return
WebString::fromLatin1(input, length);
On 2016/03/30 09:50:04, Michael van Ouwerkerk wrote:
> Or maybe only here. Maybe find a single best place for documenting why this
> needs to be latin1 :-)

I documented it all three places. Documentation is good.

https://codereview.chromium.org/1816123002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp
(right):

https://codereview.chromium.org/1816123002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushManagerTest.cpp:32: uint8_t
senderKey[kValidKeyLength];
On 2016/03/30 09:50:04, Michael van Ouwerkerk wrote:
> This is unused now.

Done.

[Michael van Ouwerkerk, 2016-03-30 12:21:20]

lgtm

[Michael van Ouwerkerk, 2016-03-30 12:21:21]

lgtm

[harkness, 2016-03-30 13:03:58]

The CQ bit was checked by harkness@chromium.org

[commit-bot: I haz the power, 2016-03-30 13:04:06]

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

[commit-bot: I haz the power, 2016-03-30 13:53:56]

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

[commit-bot: I haz the power, 2016-03-30 13:53:57]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[harkness, 2016-03-30 17:12:38]

Fixed the mac notification failure and the push message permissions context
unittests.

[harkness, 2016-03-31 09:03:55]

The CQ bit was checked by harkness@chromium.org

[harkness, 2016-03-31 09:03:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/1816123002/#ps100001
(title: "Fix for mac and permissions tests")

[commit-bot: I haz the power, 2016-03-31 09:04:15]

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

[commit-bot: I haz the power, 2016-03-31 11:09:41]

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

[commit-bot: I haz the power, 2016-03-31 11:09:42]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[harkness, 2016-03-31 16:58:50]

The CQ bit was checked by harkness@chromium.org

[harkness, 2016-03-31 16:58:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/1816123002/#ps120001
(title: "Fix asan errors")

[commit-bot: I haz the power, 2016-03-31 16:59:21]

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

[commit-bot: I haz the power, 2016-03-31 18:15:36]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-31 18:17:42]

Description was changed from

==========
Add testing for subscription from service workers

Testing covers subscription from workers both with and without keys, and in
situations where it triggers a read from a manifest or a pre-subscribed worker.

This also cleaned up the unregistration, so that any registrations are searched
for, instead of using a variable to store the registration.

BUG=596471
==========

to

==========
Add testing for subscription from service workers

Testing covers subscription from workers both with and without keys, and in
situations where it triggers a read from a manifest or a pre-subscribed worker.

This also cleaned up the unregistration, so that any registrations are searched
for, instead of using a variable to store the registration.

BUG=596471

Committed: https://crrev.com/2a019912946921d445189c61af9c1ab5c3cef860
Cr-Commit-Position: refs/heads/master@{#384332}
==========

[commit-bot: I haz the power, 2016-03-31 18:17:43]

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