From 6a55c1954c8536f1ea2f0745f7da09600bd3ecb0 Mon Sep 17 00:00:00 2001 From: peter Date: Thu, 7 May 2015 07:08:50 -0700 Subject: [PATCH] Add test coverage for PushSubscriptionOptions.userVisibleOnly This CL changes the Push API browser test suite to use the new PushSubscriptionOptions dictionary instead of relying on the manifest for providing the userVisibleOnly value. In order to make sure that we don't break backwards compatibility, add a new test to cover that as well. Additionally, Blink has switched over to the new methods, so get rid of the old ones. This CL is part of a series of four: [1] https://codereview.chromium.org/1047533002/ [2] https://codereview.chromium.org/1043723003/ [3] https://codereview.chromium.org/1044663002/ [4] This patch. BUG=471534, 446883 Review URL: https://codereview.chromium.org/1044673002 Cr-Commit-Position: refs/heads/master@{#328757} --- .../push_messaging/push_messaging_browsertest.cc | 66 +++++++++++++++++++++- chrome/test/data/push_messaging/manifest.json | 3 +- ....json => manifest_with_user_visible_false.json} | 2 +- ...t.json => manifest_with_user_visible_true.json} | 2 +- .../manifest_without_user_visible_only.json | 3 - chrome/test/data/push_messaging/push_test.js | 8 ++- .../data/push_messaging/test_bad_manifest.html | 7 ++- .../test_no_subscription_options.html | 14 +++++ .../test_user_visible_only_manifest.html | 14 +++++ content/child/push_messaging/push_provider.cc | 32 +---------- content/child/push_messaging/push_provider.h | 16 ------ .../push_messaging/push_messaging_dispatcher.cc | 14 +---- .../push_messaging/push_messaging_dispatcher.h | 6 +- 13 files changed, 111 insertions(+), 76 deletions(-) copy chrome/test/data/push_messaging/{manifest.json => manifest_with_user_visible_false.json} (52%) copy chrome/test/data/push_messaging/{manifest.json => manifest_with_user_visible_true.json} (52%) delete mode 100644 chrome/test/data/push_messaging/manifest_without_user_visible_only.json create mode 100644 chrome/test/data/push_messaging/test_no_subscription_options.html create mode 100644 chrome/test/data/push_messaging/test_user_visible_only_manifest.html diff --git a/chrome/browser/push_messaging/push_messaging_browsertest.cc b/chrome/browser/push_messaging/push_messaging_browsertest.cc index 1677a01230ff..23f6fdf609b6 100644 --- a/chrome/browser/push_messaging/push_messaging_browsertest.cc +++ b/chrome/browser/push_messaging/push_messaging_browsertest.cc @@ -39,9 +39,6 @@ #include "content/public/test/test_utils.h" #include "ui/base/window_open_disposition.h" -#if defined(OS_ANDROID) -#include "base/android/build_info.h" -#endif namespace { // Class to instantiate on the stack that is meant to be used with @@ -204,6 +201,20 @@ class PushMessagingBadManifestBrowserTest : public PushMessagingBrowserTest { } }; +class PushMessagingManifestUserVisibleOnlyTrueTest + : public PushMessagingBrowserTest { + std::string GetTestURL() override { + return "files/push_messaging/test_user_visible_only_manifest.html"; + } +}; + +class PushMessagingBrowserTestEmptySubscriptionOptions + : public PushMessagingBrowserTest { + std::string GetTestURL() override { + return "files/push_messaging/test_no_subscription_options.html"; + } +}; + IN_PROC_BROWSER_TEST_F(PushMessagingBadManifestBrowserTest, SubscribeFailsNotVisibleMessages) { std::string script_result; @@ -311,6 +322,55 @@ IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, SubscribeFailureNoManifest) { // TODO(johnme): Test subscribing from a worker - see https://crbug.com/437298. +IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTestEmptySubscriptionOptions, + RegisterFailureEmptyPushSubscriptionOptions) { + std::string script_result; + + ASSERT_TRUE(RunScript("registerServiceWorker()", &script_result)); + ASSERT_EQ("ok - service worker registered", script_result); + + InfoBarResponder accepting_responder(GetInfoBarService(), true); + ASSERT_TRUE(RunScript("requestNotificationPermission();", &script_result)); + ASSERT_EQ("permission status - granted", script_result); + + ASSERT_TRUE(RunScript("subscribePush()", &script_result)); + EXPECT_EQ("AbortError - Registration failed - permission denied", + script_result); +} + +IN_PROC_BROWSER_TEST_F(PushMessagingBadManifestBrowserTest, + RegisterFailsNotVisibleMessages) { + std::string script_result; + + ASSERT_TRUE(RunScript("registerServiceWorker()", &script_result)); + ASSERT_EQ("ok - service worker registered", script_result); + ASSERT_TRUE(RunScript("subscribePush()", &script_result)); + EXPECT_EQ("AbortError - Registration failed - permission denied", + script_result); +} + +IN_PROC_BROWSER_TEST_F(PushMessagingManifestUserVisibleOnlyTrueTest, + ManifestKeyConsidered) { + // Chrome 42 introduced the "gcm_user_visible_only" manifest key, but Chrome + // 43 supersedes this by the standardized PushSubscriptionOptions.userVisible + // option. We maintain support for the manifest key without specifying the + // subscription option, so verify that it is still being considered. + std::string script_result; + + ASSERT_TRUE(RunScript("registerServiceWorker()", &script_result)); + ASSERT_EQ("ok - service worker registered", script_result); + + InfoBarResponder accepting_responder(GetInfoBarService(), true); + ASSERT_TRUE(RunScript("requestNotificationPermission();", &script_result)); + EXPECT_EQ("permission status - granted", script_result); + + ASSERT_TRUE(RunScript("subscribePush()", &script_result)); + EXPECT_EQ(GetEndpointForSubscriptionId("1-0"), script_result); + + ASSERT_TRUE(RunScript("permissionState()", &script_result)); + EXPECT_EQ("permission status - granted", script_result); +} + IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, SubscribePersisted) { std::string script_result; diff --git a/chrome/test/data/push_messaging/manifest.json b/chrome/test/data/push_messaging/manifest.json index 51cc951746ae..b43e22ca2434 100644 --- a/chrome/test/data/push_messaging/manifest.json +++ b/chrome/test/data/push_messaging/manifest.json @@ -1,4 +1,3 @@ { - "gcm_sender_id": "1234567890", - "gcm_user_visible_only" : true + "gcm_sender_id": "1234567890" } diff --git a/chrome/test/data/push_messaging/manifest.json b/chrome/test/data/push_messaging/manifest_with_user_visible_false.json similarity index 52% copy from chrome/test/data/push_messaging/manifest.json copy to chrome/test/data/push_messaging/manifest_with_user_visible_false.json index 51cc951746ae..213940918407 100644 --- a/chrome/test/data/push_messaging/manifest.json +++ b/chrome/test/data/push_messaging/manifest_with_user_visible_false.json @@ -1,4 +1,4 @@ { "gcm_sender_id": "1234567890", - "gcm_user_visible_only" : true + "gcm_user_visible_only": false } diff --git a/chrome/test/data/push_messaging/manifest.json b/chrome/test/data/push_messaging/manifest_with_user_visible_true.json similarity index 52% copy from chrome/test/data/push_messaging/manifest.json copy to chrome/test/data/push_messaging/manifest_with_user_visible_true.json index 51cc951746ae..09b805688944 100644 --- a/chrome/test/data/push_messaging/manifest.json +++ b/chrome/test/data/push_messaging/manifest_with_user_visible_true.json @@ -1,4 +1,4 @@ { "gcm_sender_id": "1234567890", - "gcm_user_visible_only" : true + "gcm_user_visible_only": true } diff --git a/chrome/test/data/push_messaging/manifest_without_user_visible_only.json b/chrome/test/data/push_messaging/manifest_without_user_visible_only.json deleted file mode 100644 index b43e22ca2434..000000000000 --- a/chrome/test/data/push_messaging/manifest_without_user_visible_only.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "gcm_sender_id": "1234567890" -} diff --git a/chrome/test/data/push_messaging/push_test.js b/chrome/test/data/push_messaging/push_test.js index dda5908647df..3412439feace 100644 --- a/chrome/test/data/push_messaging/push_test.js +++ b/chrome/test/data/push_messaging/push_test.js @@ -7,6 +7,10 @@ var resultQueue = new ResultQueue(); var pushSubscription = null; +var pushSubscriptionOptions = { + userVisibleOnly: true +}; + // Sends data back to the test. This must be in response to an earlier // request, but it's ok to respond asynchronously. The request blocks until // the response is sent. @@ -94,7 +98,7 @@ function removeManifest() { function subscribePush() { navigator.serviceWorker.ready.then(function(swRegistration) { - return swRegistration.pushManager.subscribe() + return swRegistration.pushManager.subscribe(pushSubscriptionOptions) .then(function(subscription) { pushSubscription = subscription; sendResultToTest(subscription.endpoint); @@ -104,7 +108,7 @@ function subscribePush() { function permissionState() { navigator.serviceWorker.ready.then(function(swRegistration) { - return swRegistration.pushManager.permissionState() + return swRegistration.pushManager.permissionState(pushSubscriptionOptions) .then(function(permission) { sendResultToTest('permission status - ' + permission); }); diff --git a/chrome/test/data/push_messaging/test_bad_manifest.html b/chrome/test/data/push_messaging/test_bad_manifest.html index 0ca3cbb98a18..5c206385495d 100644 --- a/chrome/test/data/push_messaging/test_bad_manifest.html +++ b/chrome/test/data/push_messaging/test_bad_manifest.html @@ -2,10 +2,13 @@ Push API Bad Manifest Test - + -

Push API Bad Manifest Test

+

Push API: gcm_user_visible_only=false in the Manifest

+ diff --git a/chrome/test/data/push_messaging/test_no_subscription_options.html b/chrome/test/data/push_messaging/test_no_subscription_options.html new file mode 100644 index 000000000000..291e690318aa --- /dev/null +++ b/chrome/test/data/push_messaging/test_no_subscription_options.html @@ -0,0 +1,14 @@ + + + + Push API Test: Valid manifest, but no PushSubscriptionOptions + + + + +

Push API Test

+ + + diff --git a/chrome/test/data/push_messaging/test_user_visible_only_manifest.html b/chrome/test/data/push_messaging/test_user_visible_only_manifest.html new file mode 100644 index 000000000000..6d9560dadf62 --- /dev/null +++ b/chrome/test/data/push_messaging/test_user_visible_only_manifest.html @@ -0,0 +1,14 @@ + + + + Push API Bad Manifest Test + + + + +

Push API: gcm_user_visible_only=true in the Manifest

+ + + diff --git a/content/child/push_messaging/push_provider.cc b/content/child/push_messaging/push_provider.cc index 23033b5820d7..0d85dee2a393 100644 --- a/content/child/push_messaging/push_provider.cc +++ b/content/child/push_messaging/push_provider.cc @@ -76,15 +76,7 @@ void PushProvider::subscribe( int64 service_worker_registration_id = GetServiceWorkerRegistrationId(service_worker_registration); thread_safe_sender_->Send(new PushMessagingHostMsg_RegisterFromWorker( - request_id, service_worker_registration_id, options.userVisible)); -} - -void PushProvider::registerPushMessaging( - blink::WebServiceWorkerRegistration* service_worker_registration, - blink::WebPushSubscriptionCallbacks* callbacks) { - subscribe(service_worker_registration, - blink::WebPushSubscriptionOptions(), - callbacks); + request_id, service_worker_registration_id, options.userVisibleOnly)); } void PushProvider::unsubscribe( @@ -102,12 +94,6 @@ void PushProvider::unsubscribe( request_id, service_worker_registration_id)); } -void PushProvider::unregister( - blink::WebServiceWorkerRegistration* service_worker_registration, - blink::WebPushUnsubscribeCallbacks* callbacks) { - unsubscribe(service_worker_registration, callbacks); -} - void PushProvider::getSubscription( blink::WebServiceWorkerRegistration* service_worker_registration, blink::WebPushSubscriptionCallbacks* callbacks) { @@ -121,12 +107,6 @@ void PushProvider::getSubscription( request_id, service_worker_registration_id)); } -void PushProvider::getRegistration( - blink::WebServiceWorkerRegistration* service_worker_registration, - blink::WebPushSubscriptionCallbacks* callbacks) { - getSubscription(service_worker_registration, callbacks); -} - void PushProvider::getPermissionStatus( blink::WebServiceWorkerRegistration* service_worker_registration, const blink::WebPushSubscriptionOptions& options, @@ -138,15 +118,7 @@ void PushProvider::getPermissionStatus( int64 service_worker_registration_id = GetServiceWorkerRegistrationId(service_worker_registration); thread_safe_sender_->Send(new PushMessagingHostMsg_GetPermissionStatus( - request_id, service_worker_registration_id, options.userVisible)); -} - -void PushProvider::getPermissionStatus( - blink::WebServiceWorkerRegistration* service_worker_registration, - blink::WebPushPermissionStatusCallbacks* callbacks) { - getPermissionStatus(service_worker_registration, - blink::WebPushSubscriptionOptions(), - callbacks); + request_id, service_worker_registration_id, options.userVisibleOnly)); } bool PushProvider::OnMessageReceived(const IPC::Message& message) { diff --git a/content/child/push_messaging/push_provider.h b/content/child/push_messaging/push_provider.h index 39cb2e06fe19..e44bf41b943a 100644 --- a/content/child/push_messaging/push_provider.h +++ b/content/child/push_messaging/push_provider.h @@ -44,32 +44,16 @@ class PushProvider : public blink::WebPushProvider, blink::WebServiceWorkerRegistration* service_worker_registration, const blink::WebPushSubscriptionOptions& options, blink::WebPushSubscriptionCallbacks* callbacks); - // TODO(peter): Remove this method when Blink switched over to the above. - virtual void registerPushMessaging( - blink::WebServiceWorkerRegistration* service_worker_registration, - blink::WebPushSubscriptionCallbacks* callbacks); virtual void unsubscribe( blink::WebServiceWorkerRegistration* service_worker_registration, blink::WebPushUnsubscribeCallbacks* callbacks); - // TODO(peter): Remove this method when Blink switched over to the above. - virtual void unregister( - blink::WebServiceWorkerRegistration* service_worker_registration, - blink::WebPushUnsubscribeCallbacks* callbacks); virtual void getSubscription( blink::WebServiceWorkerRegistration* service_worker_registration, blink::WebPushSubscriptionCallbacks* callbacks); - // TODO(peter): Remove this method when Blink switched over to the above. - virtual void getRegistration( - blink::WebServiceWorkerRegistration* service_worker_registration, - blink::WebPushSubscriptionCallbacks* callbacks); virtual void getPermissionStatus( blink::WebServiceWorkerRegistration* service_worker_registration, const blink::WebPushSubscriptionOptions& options, blink::WebPushPermissionStatusCallbacks* callbacks); - // TODO(peter): Remove this method when Blink switched over to the above. - virtual void getPermissionStatus( - blink::WebServiceWorkerRegistration* service_worker_registration, - blink::WebPushPermissionStatusCallbacks* callbacks); // Called by the PushDispatcher. bool OnMessageReceived(const IPC::Message& message); diff --git a/content/renderer/push_messaging/push_messaging_dispatcher.cc b/content/renderer/push_messaging/push_messaging_dispatcher.cc index db8ef1951096..e3f838a1af4d 100644 --- a/content/renderer/push_messaging/push_messaging_dispatcher.cc +++ b/content/renderer/push_messaging/push_messaging_dispatcher.cc @@ -47,22 +47,14 @@ void PushMessagingDispatcher::subscribe( DCHECK(callbacks); RenderFrameImpl::FromRoutingID(routing_id()) ->manifest_manager() - ->GetManifest(base::Bind(&PushMessagingDispatcher::DoRegister, + ->GetManifest(base::Bind(&PushMessagingDispatcher::DoSubscribe, base::Unretained(this), service_worker_registration, options, callbacks)); } -void PushMessagingDispatcher::registerPushMessaging( - blink::WebServiceWorkerRegistration* service_worker_registration, - blink::WebPushSubscriptionCallbacks* callbacks) { - subscribe(service_worker_registration, - blink::WebPushSubscriptionOptions(), - callbacks); -} - -void PushMessagingDispatcher::DoRegister( +void PushMessagingDispatcher::DoSubscribe( blink::WebServiceWorkerRegistration* service_worker_registration, const blink::WebPushSubscriptionOptions& options, blink::WebPushSubscriptionCallbacks* callbacks, @@ -85,7 +77,7 @@ void PushMessagingDispatcher::DoRegister( // TODO(peter): Display a deprecation warning if gcm_user_visible_only is // set to true. See https://crbug.com/471534 const bool user_visible = manifest.gcm_user_visible_only || - options.userVisible; + options.userVisibleOnly; Send(new PushMessagingHostMsg_RegisterFromDocument( routing_id(), request_id, diff --git a/content/renderer/push_messaging/push_messaging_dispatcher.h b/content/renderer/push_messaging/push_messaging_dispatcher.h index da548db853ce..56eaa852820d 100644 --- a/content/renderer/push_messaging/push_messaging_dispatcher.h +++ b/content/renderer/push_messaging/push_messaging_dispatcher.h @@ -43,12 +43,8 @@ class PushMessagingDispatcher : public RenderFrameObserver, blink::WebServiceWorkerRegistration* service_worker_registration, const blink::WebPushSubscriptionOptions& options, blink::WebPushSubscriptionCallbacks* callbacks); - // TODO(peter): Remove this method when Blink switched over to the above. - virtual void registerPushMessaging( - blink::WebServiceWorkerRegistration* service_worker_registration, - blink::WebPushSubscriptionCallbacks* callbacks); // override - void DoRegister( + void DoSubscribe( blink::WebServiceWorkerRegistration* service_worker_registration, const blink::WebPushSubscriptionOptions& options, blink::WebPushSubscriptionCallbacks* callbacks, -- 2.11.4.GIT