diff options
22 files changed, 456 insertions, 394 deletions
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java b/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java index ff574db..f2ad64d 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java @@ -14,13 +14,19 @@ public class NotificationConstants { public static final String ACTION_CLOSE_NOTIFICATION = "org.chromium.chrome.browser.notifications.CLOSE_NOTIFICATION"; - public static final String EXTRA_NOTIFICATION_ID = "notification_id"; + /** + * Name of the Intent extra set by the framework when a notification preferences intent has + * been triggered from there, which could be one of the setting gears in system UI. + */ public static final String EXTRA_NOTIFICATION_TAG = "notification_tag"; - // TODO(peter): Remove these extras once Notifications are powered by a database on the - // native side, that contains all the additional information. - public static final String EXTRA_NOTIFICATION_PLATFORM_ID = "notification_platform_id"; - public static final String EXTRA_NOTIFICATION_DATA = "notification_data"; + /** + * Names of the Intent extras used for Intents related to notifications. These intents are set + * and owned by Chromium. + */ + public static final String EXTRA_PERSISTENT_NOTIFICATION_ID = "notification_persistent_id"; + public static final String EXTRA_NOTIFICATION_INFO_ORIGIN = "notification_info_origin"; + public static final String EXTRA_NOTIFICATION_INFO_TAG = "notification_info_tag"; /** * Unique identifier for a single sync notification. Since the notification ID is reused, diff --git a/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java b/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java index cf903b0..d4d9ca2 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java @@ -52,7 +52,11 @@ public class NotificationService extends IntentService { */ @Override public void onHandleIntent(final Intent intent) { - if (!intent.hasExtra(NotificationConstants.EXTRA_NOTIFICATION_ID)) return; + if (!intent.hasExtra(NotificationConstants.EXTRA_PERSISTENT_NOTIFICATION_ID) + || !intent.hasExtra(NotificationConstants.EXTRA_NOTIFICATION_INFO_ORIGIN) + || !intent.hasExtra(NotificationConstants.EXTRA_NOTIFICATION_INFO_TAG)) { + return; + } ThreadUtils.runOnUiThread(new Runnable() { @Override diff --git a/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java b/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java index e21b756..476649c 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java @@ -136,20 +136,15 @@ public class NotificationUIManager { } } - String notificationId = intent.getStringExtra(NotificationConstants.EXTRA_NOTIFICATION_ID); - if (NotificationConstants.ACTION_CLICK_NOTIFICATION.equals(intent.getAction())) { - if (!intent.hasExtra(NotificationConstants.EXTRA_NOTIFICATION_DATA)) { - Log.e(TAG, "Not all required notification data has been set in the intent."); - return false; - } + long persistentNotificationId = + intent.getLongExtra(NotificationConstants.EXTRA_PERSISTENT_NOTIFICATION_ID, -1); + String origin = intent.getStringExtra(NotificationConstants.EXTRA_NOTIFICATION_INFO_ORIGIN); + String tag = intent.getStringExtra(NotificationConstants.EXTRA_NOTIFICATION_INFO_TAG); - byte[] notificationData = - intent.getByteArrayExtra(NotificationConstants.EXTRA_NOTIFICATION_DATA); - return sInstance.onNotificationClicked(notificationId, notificationData); - } - - if (NotificationConstants.ACTION_CLOSE_NOTIFICATION.equals(intent.getAction())) { - return sInstance.onNotificationClosed(notificationId); + if (NotificationConstants.ACTION_CLICK_NOTIFICATION.equals(intent.getAction())) { + return sInstance.onNotificationClicked(persistentNotificationId, origin, tag); + } else if (NotificationConstants.ACTION_CLOSE_NOTIFICATION.equals(intent.getAction())) { + return sInstance.onNotificationClosed(persistentNotificationId, origin, tag); } Log.e(TAG, "Unrecognized Notification action: " + intent.getAction()); @@ -203,12 +198,25 @@ public class NotificationUIManager { applicationContext.startActivity(preferencesIntent); } - private PendingIntent getPendingIntent(String action, String notificationId, - byte[] notificationData, Uri intentData) { + /** + * Returns the PendingIntent for completing |action| on the notification identified by the data + * in the other parameters. |intentData| is used to ensure uniqueness of the PendingIntent. + * + * @param action The action this pending intent will represent. + * @param persistentNotificationId The persistent id of the notification. + * @param origin The origin to whom the notification belongs. + * @param tag The tag of the notification. May be NULL. + * @param intentData URI used to ensure uniqueness of the created PendingIntent. + */ + private PendingIntent getPendingIntent(String action, long persistentNotificationId, + String origin, @Nullable String tag, Uri intentData) { Intent intent = new Intent(action, intentData); intent.setClass(mAppContext, NotificationService.Receiver.class); - intent.putExtra(NotificationConstants.EXTRA_NOTIFICATION_ID, notificationId); - intent.putExtra(NotificationConstants.EXTRA_NOTIFICATION_DATA, notificationData); + + intent.putExtra(NotificationConstants.EXTRA_PERSISTENT_NOTIFICATION_ID, + persistentNotificationId); + intent.putExtra(NotificationConstants.EXTRA_NOTIFICATION_INFO_ORIGIN, origin); + intent.putExtra(NotificationConstants.EXTRA_NOTIFICATION_INFO_TAG, tag); return PendingIntent.getBroadcast(mAppContext, PENDING_INTENT_REQUEST_CODE, intent, PendingIntent.FLAG_UPDATE_CURRENT); @@ -227,13 +235,13 @@ public class NotificationUIManager { * If the input tag is empty the output is PREFIX + SEPARATOR + ORIGIN + SEPARATOR + * NOTIFICATION_ID. * - * @param tag A string identifier for this notification. - * @param notificationId An string identifier for this notification. + * @param persistentNotificationId The persistent id of the notification. * @param origin The origin for which the notification is shown. + * @param tag A string identifier for this notification. * @return The generated platform tag. */ - private static String makePlatformTag(@Nullable String tag, String notificationId, - String origin) { + private static String makePlatformTag(long persistentNotificationId, String origin, + @Nullable String tag) { // The given tag may contain the separator character, so add it last to make reading the // preceding origin token reliable. If no tag was specified (it is the default empty // string), make the platform tag unique by appending the notification id. @@ -242,11 +250,13 @@ public class NotificationUIManager { .append(NotificationConstants.NOTIFICATION_TAG_SEPARATOR) .append(origin) .append(NotificationConstants.NOTIFICATION_TAG_SEPARATOR); + if (TextUtils.isEmpty(tag)) { - builder.append(notificationId); + builder.append(persistentNotificationId); } else { builder.append(tag); } + return builder.toString(); } @@ -280,25 +290,24 @@ public class NotificationUIManager { } /** - * Displays a notification with the given |notificationId|, |title|, |body| and |icon|. + * Displays a notification with the given details. * + * @param persistentNotificationId The persistent id of the notification. + * @param origin Full text of the origin, including the protocol, owning this notification. * @param tag A string identifier for this notification. If the tag is not empty, the new * notification will replace the previous notification with the same tag and origin, * if present. If no matching previous notification is present, the new one will just * be added. - * @param notificationId Unique id provided by the Chrome Notification system. * @param title Title to be displayed in the notification. * @param body Message to be displayed in the notification. Will be trimmed to one line of * text by the Android notification system. * @param icon Icon to be displayed in the notification. When this isn't a valid Bitmap, a * default icon will be generated instead. - * @param origin Full text of the origin, including the protocol, owning this notification. * @param silent Whether the default sound, vibration and lights should be suppressed. - * @param notificationData Serialized data associated with the notification. */ @CalledByNative - private void displayNotification(String tag, String notificationId, String title, String body, - Bitmap icon, String origin, boolean silent, byte[] notificationData) { + private void displayNotification(long persistentNotificationId, String origin, String tag, + String title, String body, Bitmap icon, boolean silent) { if (icon == null || icon.getWidth() == 0) { icon = getIconGenerator().generateIconForUrl(origin, true); } @@ -308,7 +317,8 @@ public class NotificationUIManager { // The data used to make each intent unique according to the rules of Intent#filterEquals. // Without this, the pending intents derived from them may be reused, because extras are // not taken into account for the filterEquals comparison. - Uri intentData = Uri.parse(origin).buildUpon().fragment(notificationId).build(); + Uri intentData = Uri.parse(origin).buildUpon().fragment( + String.valueOf(persistentNotificationId)).build(); // Set up a pending intent for going to the settings screen for |origin|. Intent settingsIntent = PreferencesLauncher.createIntentForSettingsPage( @@ -316,6 +326,7 @@ public class NotificationUIManager { settingsIntent.setData(intentData); settingsIntent.putExtra(Preferences.EXTRA_SHOW_FRAGMENT_ARGUMENTS, SingleWebsitePreferences.createFragmentArgsForSite(origin)); + PendingIntent pendingSettingsIntent = PendingIntent.getActivity(mAppContext, PENDING_INTENT_REQUEST_CODE, settingsIntent, PendingIntent.FLAG_UPDATE_CURRENT); @@ -327,10 +338,10 @@ public class NotificationUIManager { .setSmallIcon(R.drawable.notification_badge) .setContentIntent(getPendingIntent( NotificationConstants.ACTION_CLICK_NOTIFICATION, - notificationId, notificationData, intentData)) + persistentNotificationId, origin, tag, intentData)) .setDeleteIntent(getPendingIntent( NotificationConstants.ACTION_CLOSE_NOTIFICATION, - notificationId, notificationData, intentData)) + persistentNotificationId, origin, tag, intentData)) .addAction(R.drawable.settings_cog, res.getString(R.string.page_info_site_settings_button), pendingSettingsIntent) @@ -340,7 +351,7 @@ public class NotificationUIManager { // has been marked as being silent, for example because it's low priority. if (!silent) notificationBuilder.setDefaults(Notification.DEFAULT_ALL); - String platformTag = makePlatformTag(tag, notificationId, origin); + String platformTag = makePlatformTag(persistentNotificationId, origin, tag); mNotificationManager.notify(platformTag, PLATFORM_ID, notificationBuilder.build()); } @@ -382,27 +393,53 @@ public class NotificationUIManager { } /** - * Closes the notification identified by |tag|, |notificationId|, and |origin|. + * Closes the notification associated with the given parameters. + * + * @param persistentNotificationId The persistent id of the notification. + * @param origin The origin to which the notification belongs. + * @param tag The tag of the notification. May be NULL. */ @CalledByNative - private void closeNotification(String tag, String notificationId, String origin) { - String platformTag = makePlatformTag(tag, notificationId, origin); + private void closeNotification(long persistentNotificationId, String origin, String tag) { + String platformTag = makePlatformTag(persistentNotificationId, origin, tag); mNotificationManager.cancel(platformTag, PLATFORM_ID); } - private boolean onNotificationClicked(String notificationId, byte[] notificationData) { - return nativeOnNotificationClicked( - mNativeNotificationManager, notificationId, notificationData); + /** + * Calls NotificationUIManagerAndroid::OnNotificationClicked in native code to indicate that + * the notification with the given parameters has been clicked on. + * + * @param persistentNotificationId The persistent id of the notification. + * @param origin The origin of the notification. + * @param tag The tag of the notification. May be NULL. + * @return Whether the manager could handle the click event. + */ + private boolean onNotificationClicked(long persistentNotificationId, String origin, + String tag) { + return nativeOnNotificationClicked(mNativeNotificationManager, persistentNotificationId, + origin, tag); } - private boolean onNotificationClosed(String notificationId) { - return nativeOnNotificationClosed(mNativeNotificationManager, notificationId); + /** + * Calls NotificationUIManagerAndroid::OnNotificationClosed in native code to indicate that + * the notification with the given parameters has been closed. This could be the result of + * user interaction or an action initiated by the framework. + * + * @param persistentNotificationId The persistent id of the notification. + * @param origin The origin of the notification. + * @param tag The tag of the notification. May be NULL. + * @return Whether the manager could handle the close event. + */ + private boolean onNotificationClosed(long persistentNotificationId, String origin, + String tag) { + return nativeOnNotificationClosed(mNativeNotificationManager, persistentNotificationId, + origin, tag); } private static native void nativeInitializeNotificationUIManager(); private native boolean nativeOnNotificationClicked(long nativeNotificationUIManagerAndroid, - String notificationId, byte[] notificationData); - private native boolean nativeOnNotificationClosed( - long nativeNotificationUIManagerAndroid, String notificationId); + long persistentNotificationId, String origin, String tag); + private native boolean nativeOnNotificationClosed(long nativeNotificationUIManagerAndroid, + long persistentNotificationId, String origin, String tag); } diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java index 3aca840..7b142dc 100644 --- a/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java +++ b/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java @@ -7,7 +7,6 @@ package org.chromium.chrome.browser.notifications; import android.app.Notification; import android.graphics.Bitmap; import android.os.Build; -import android.test.FlakyTest; import android.test.suitebuilder.annotation.LargeTest; import android.test.suitebuilder.annotation.MediumTest; import android.test.suitebuilder.annotation.SmallTest; @@ -233,28 +232,6 @@ public class NotificationUIManagerTest extends ChromeShellTestBase { } /* - * Verifies that starting the PendingIntent stored as the notification's delete intent will - * close the notification. - */ - /* @MediumTest */ - @FlakyTest - @Feature({"Browser", "Notifications"}) - public void testNotificationDeleteIntentClosesNotification() throws Exception { - setNotificationContentSettingForCurrentOrigin(ContentSetting.ALLOW); - - Notification notification = showAndGetNotification("MyNotification", "{}"); - assertEquals(1, mMockNotificationManager.getNotifications().size()); - - // Sending the PendingIntent simulates dismissing (swiping away) the notification. - assertNotNull(notification.deleteIntent); - notification.deleteIntent.send(); - // The deleteIntent should trigger a call to cancel() in the NotificationManager. - assertTrue(waitForNotificationManagerMutation()); - - assertEquals(0, mMockNotificationManager.getNotifications().size()); - } - - /* * Verifies that starting the PendingIntent stored as the notification's content intent will * start up the associated Service Worker, where the JavaScript code will close the notification * by calling event.notification.close(). diff --git a/chrome/browser/notifications/notification_ui_manager_android.cc b/chrome/browser/notifications/notification_ui_manager_android.cc index 7152638..aa5984c 100644 --- a/chrome/browser/notifications/notification_ui_manager_android.cc +++ b/chrome/browser/notifications/notification_ui_manager_android.cc @@ -9,7 +9,7 @@ #include "base/android/jni_array.h" #include "base/android/jni_string.h" #include "base/logging.h" -#include "base/pickle.h" +#include "base/strings/string_number_conversions.h" #include "chrome/browser/browser_process.h" #include "chrome/browser/notifications/notification.h" #include "chrome/browser/notifications/persistent_notification_delegate.h" @@ -29,91 +29,6 @@ using base::android::ConvertUTF8ToJavaString; namespace { -// The maximum size of the serialized pickle that carries a notification's meta -// information. Notifications carrying more data will be silently dropped - with -// an error being written to the log. -const int kMaximumSerializedNotificationSizeBytes = 1024 * 1024; - -// Persistent notifications are likely to outlive the browser process they were -// created by on Android. In order to be able to re-surface the notification -// when the user interacts with them, all relevant notification data needs to -// be serialized with the notification itself. -// -// In the near future, as more features get added to Chrome's notification -// implementation, this will be done by storing the persistent notification data -// in a database. However, to support launching Chrome for Android from a -// notification event until that exists, serialize the data in the Intent. -// -// TODO(peter): Move towards storing notification data in a database rather than -// as a serialized Intent extra. - -scoped_ptr<Pickle> SerializePersistentNotification( - const content::PlatformNotificationData& notification_data, - const GURL& origin, - int64 service_worker_registration_id) { - scoped_ptr<Pickle> pickle(new Pickle); - - // content::PlatformNotificationData - pickle->WriteString16(notification_data.title); - pickle->WriteInt(static_cast<int>(notification_data.direction)); - pickle->WriteString(notification_data.lang); - pickle->WriteString16(notification_data.body); - pickle->WriteString(notification_data.tag); - pickle->WriteString(notification_data.icon.spec()); - pickle->WriteBool(notification_data.silent); - - // The origin which is displaying the notification. - pickle->WriteString(origin.spec()); - - // The Service Worker registration this notification is associated with. - pickle->WriteInt64(service_worker_registration_id); - - if (pickle->size() > kMaximumSerializedNotificationSizeBytes) - return nullptr; - - return pickle.Pass(); -} - -bool UnserializePersistentNotification( - const Pickle& pickle, - content::PlatformNotificationData* notification_data, - GURL* origin, - int64* service_worker_registration_id) { - DCHECK(notification_data && origin && service_worker_registration_id); - PickleIterator iterator(pickle); - - std::string icon_url, origin_url; - int direction_value; - - // Unpack content::PlatformNotificationData. - if (!iterator.ReadString16(¬ification_data->title) || - !iterator.ReadInt(&direction_value) || - !iterator.ReadString(¬ification_data->lang) || - !iterator.ReadString16(¬ification_data->body) || - !iterator.ReadString(¬ification_data->tag) || - !iterator.ReadString(&icon_url) || - !iterator.ReadBool(¬ification_data->silent)) { - return false; - } - - notification_data->direction = - static_cast<content::PlatformNotificationData::NotificationDirection>( - direction_value); - notification_data->icon = GURL(icon_url); - - // Unpack the origin which displayed this notification. - if (!iterator.ReadString(&origin_url)) - return false; - - *origin = GURL(origin_url); - - // Unpack the Service Worker registration id. - if (!iterator.ReadInt64(service_worker_registration_id)) - return false; - - return true; -} - // Called when the "notificationclick" event in the Service Worker has finished // executing for a notification that was created in a previous instance of the // browser. @@ -143,8 +58,6 @@ NotificationUIManagerAndroid::NotificationUIManagerAndroid() { AttachCurrentThread(), reinterpret_cast<intptr_t>(this), base::android::GetApplicationContext())); - - // TODO(peter): Synchronize notifications with the Java side. } NotificationUIManagerAndroid::~NotificationUIManagerAndroid() { @@ -155,72 +68,35 @@ NotificationUIManagerAndroid::~NotificationUIManagerAndroid() { bool NotificationUIManagerAndroid::OnNotificationClicked( JNIEnv* env, jobject java_object, - jstring notification_id, - jbyteArray serialized_notification_data) { - std::string id = ConvertJavaStringToUTF8(env, notification_id); - - auto iter = profile_notifications_.find(id); - if (iter != profile_notifications_.end()) { - const Notification& notification = iter->second->notification(); - notification.delegate()->Click(); + jlong persistent_notification_id, + jstring java_origin, + jstring java_tag) { + GURL origin(ConvertJavaStringToUTF8(env, java_origin)); + std::string tag = ConvertJavaStringToUTF8(env, java_tag); - return true; - } - - // If the Notification were not found, it may be a persistent notification - // that outlived the Chrome browser process. In this case, try to - // unserialize the notification's serialized data and trigger the click - // event manually. - - std::vector<uint8> bytes; - base::android::JavaByteArrayToByteVector(env, serialized_notification_data, - &bytes); - if (!bytes.size()) - return false; - - content::PlatformNotificationData notification_data; - GURL origin; - int64 service_worker_registration_id; - - Pickle pickle(reinterpret_cast<const char*>(&bytes[0]), bytes.size()); - if (!UnserializePersistentNotification(pickle, ¬ification_data, &origin, - &service_worker_registration_id)) { - return false; - } - - // Store the tag and origin of this notification so that it can later be - // closed using these details. - regenerated_notification_infos_[id] = - std::make_pair(notification_data.tag, origin.spec()); - - PlatformNotificationServiceImpl* service = - PlatformNotificationServiceImpl::GetInstance(); + regenerated_notification_infos_[persistent_notification_id] = + std::make_pair(origin.spec(), tag); // TODO(peter): Rather than assuming that the last used profile is the // appropriate one for this notification, the used profile should be // stored as part of the notification's data. See https://crbug.com/437574. - service->OnPersistentNotificationClick( + PlatformNotificationServiceImpl::GetInstance()->OnPersistentNotificationClick( ProfileManager::GetLastUsedProfile(), - service_worker_registration_id, - id, + persistent_notification_id, origin, - notification_data, base::Bind(&OnEventDispatchComplete)); return true; } bool NotificationUIManagerAndroid::OnNotificationClosed( - JNIEnv* env, jobject java_object, jstring notification_id) { - std::string id = ConvertJavaStringToUTF8(env, notification_id); - - auto iter = profile_notifications_.find(id); - if (iter == profile_notifications_.end()) - return false; - - const Notification& notification = iter->second->notification(); - notification.delegate()->Close(true /** by_user **/); - RemoveProfileNotification(iter->second, true /* close */); + JNIEnv* env, + jobject java_object, + jlong persistent_notification_id, + jstring java_origin, + jstring java_tag) { + // TODO(peter): Implement handling when a notification has been closed. The + // notification database has to reflect this in its own state. return true; } @@ -242,16 +118,21 @@ void NotificationUIManagerAndroid::Add(const Notification& notification, JNIEnv* env = AttachCurrentThread(); + PersistentNotificationDelegate* delegate = + static_cast<PersistentNotificationDelegate*>(notification.delegate()); + DCHECK(delegate); + + int64_t persistent_notification_id = delegate->persistent_notification_id(); + GURL origin_url(notification.origin_url().GetOrigin()); + + ScopedJavaLocalRef<jstring> origin = ConvertUTF8ToJavaString( + env, origin_url.spec()); ScopedJavaLocalRef<jstring> tag = ConvertUTF8ToJavaString(env, notification.tag()); - ScopedJavaLocalRef<jstring> id = ConvertUTF8ToJavaString( - env, profile_notification->notification().id()); ScopedJavaLocalRef<jstring> title = ConvertUTF16ToJavaString( env, notification.title()); ScopedJavaLocalRef<jstring> body = ConvertUTF16ToJavaString( env, notification.message()); - ScopedJavaLocalRef<jstring> origin = ConvertUTF8ToJavaString( - env, notification.origin_url().GetOrigin().spec()); ScopedJavaLocalRef<jobject> icon; @@ -259,41 +140,19 @@ void NotificationUIManagerAndroid::Add(const Notification& notification, if (!icon_bitmap.isNull()) icon = gfx::ConvertToJavaBitmap(&icon_bitmap); - ScopedJavaLocalRef<jbyteArray> notification_data; - if (true /** is_persistent_notification **/) { - PersistentNotificationDelegate* delegate = - static_cast<PersistentNotificationDelegate*>(notification.delegate()); - scoped_ptr<Pickle> pickle = SerializePersistentNotification( - delegate->notification_data(), - notification.origin_url(), - delegate->service_worker_registration_id()); - - if (!pickle) { - LOG(ERROR) << - "Unable to serialize the notification, payload too large (max 1MB)."; - RemoveProfileNotification(profile_notification, true /* close */); - return; - } - - notification_data = base::android::ToJavaByteArray( - env, static_cast<const uint8*>(pickle->data()), pickle->size()); - } - Java_NotificationUIManager_displayNotification( env, java_object_.obj(), + persistent_notification_id, + origin.obj(), tag.obj(), - id.obj(), title.obj(), body.obj(), icon.obj(), - origin.obj(), - notification.silent(), - notification_data.obj()); + notification.silent()); - regenerated_notification_infos_[profile_notification->notification().id()] = - std::make_pair(notification.tag(), - notification.origin_url().GetOrigin().spec()); + regenerated_notification_infos_[persistent_notification_id] = + std::make_pair(origin_url.spec(), notification.tag()); notification.delegate()->Display(); } @@ -408,25 +267,30 @@ bool NotificationUIManagerAndroid::RegisterNotificationUIManager(JNIEnv* env) { void NotificationUIManagerAndroid::PlatformCloseNotification( const std::string& notification_id) { - auto iterator = regenerated_notification_infos_.find(notification_id); + int64_t persistent_notification_id = 0; + if (!base::StringToInt64(notification_id, &persistent_notification_id)) + return; + + const auto iterator = + regenerated_notification_infos_.find(persistent_notification_id); if (iterator == regenerated_notification_infos_.end()) return; RegeneratedNotificationInfo notification_info = iterator->second; JNIEnv* env = AttachCurrentThread(); - ScopedJavaLocalRef<jstring> tag = - ConvertUTF8ToJavaString(env, notification_info.first); ScopedJavaLocalRef<jstring> origin = + ConvertUTF8ToJavaString(env, notification_info.first); + ScopedJavaLocalRef<jstring> tag = ConvertUTF8ToJavaString(env, notification_info.second); - ScopedJavaLocalRef<jstring> java_notification_id = - ConvertUTF8ToJavaString(env, notification_id); - regenerated_notification_infos_.erase(notification_id); + regenerated_notification_infos_.erase(iterator); - Java_NotificationUIManager_closeNotification( - env, java_object_.obj(), tag.obj(), java_notification_id.obj(), - origin.obj()); + Java_NotificationUIManager_closeNotification(env, + java_object_.obj(), + persistent_notification_id, + origin.obj(), + tag.obj()); } void NotificationUIManagerAndroid::AddProfileNotification( diff --git a/chrome/browser/notifications/notification_ui_manager_android.h b/chrome/browser/notifications/notification_ui_manager_android.h index e7681bb..cc7ac32 100644 --- a/chrome/browser/notifications/notification_ui_manager_android.h +++ b/chrome/browser/notifications/notification_ui_manager_android.h @@ -17,21 +17,27 @@ class ProfileNotification; // Implementation of the Notification UI Manager for Android, which defers to // the Android framework for displaying notifications. +// +// The Android notification manager only operates reliably on notifications +// that were opened or interacted with since the last restart of Chrome. class NotificationUIManagerAndroid : public NotificationUIManager { public: NotificationUIManagerAndroid(); ~NotificationUIManagerAndroid() override; - // Called by the Java implementation when a notification has been clicked on. + // Called by the Java implementation when the notification has been clicked. bool OnNotificationClicked(JNIEnv* env, jobject java_object, - jstring notification_id, - jbyteArray serialized_notification_data); + jlong persistent_notification_id, + jstring java_origin, + jstring java_tag); - // Called by the Java implementation when a notification has been closed. + // Called by the Java implementation when the notification has been closed. bool OnNotificationClosed(JNIEnv* env, jobject java_object, - jstring notification_id); + jlong persistent_notification_id, + jstring java_origin, + jstring java_tag); // NotificationUIManager implementation; void Add(const Notification& notification, Profile* profile) override; @@ -75,11 +81,14 @@ class NotificationUIManagerAndroid : public NotificationUIManager { // Map from a notification id to the associated ProfileNotification*. std::map<std::string, ProfileNotification*> profile_notifications_; - // Holds the tag of a notification, and its origin. + // Pair containing the information necessary in order to enable closing + // notifications that were not created by this instance of the manager: the + // notification's origin and tag. This list may not contain the notifications + // that have not been interacted with since the last restart of Chrome. using RegeneratedNotificationInfo = std::pair<std::string, std::string>; - // Map from notification id to RegeneratedNotificationInfo. - std::map<std::string, RegeneratedNotificationInfo> + // Map from persistent notification id to RegeneratedNotificationInfo. + std::map<int64_t, RegeneratedNotificationInfo> regenerated_notification_infos_; base::android::ScopedJavaGlobalRef<jobject> java_object_; diff --git a/chrome/browser/notifications/persistent_notification_delegate.cc b/chrome/browser/notifications/persistent_notification_delegate.cc index 9ac9837..84a2576 100644 --- a/chrome/browser/notifications/persistent_notification_delegate.cc +++ b/chrome/browser/notifications/persistent_notification_delegate.cc @@ -19,13 +19,11 @@ void OnEventDispatchComplete(content::PersistentNotificationStatus status) {} PersistentNotificationDelegate::PersistentNotificationDelegate( content::BrowserContext* browser_context, - int64 service_worker_registration_id, - const GURL& origin, - const content::PlatformNotificationData& notification_data) + int64_t persistent_notification_id, + const GURL& origin) : browser_context_(browser_context), - service_worker_registration_id_(service_worker_registration_id), + persistent_notification_id_(persistent_notification_id), origin_(origin), - notification_data_(notification_data), id_(base::GenerateGUID()) {} PersistentNotificationDelegate::~PersistentNotificationDelegate() {} @@ -37,10 +35,8 @@ void PersistentNotificationDelegate::Close(bool by_user) {} void PersistentNotificationDelegate::Click() { PlatformNotificationServiceImpl::GetInstance()->OnPersistentNotificationClick( browser_context_, - service_worker_registration_id_, - id_, + persistent_notification_id_, origin_, - notification_data_, base::Bind(&OnEventDispatchComplete)); } diff --git a/chrome/browser/notifications/persistent_notification_delegate.h b/chrome/browser/notifications/persistent_notification_delegate.h index 28e9420..003d46b 100644 --- a/chrome/browser/notifications/persistent_notification_delegate.h +++ b/chrome/browser/notifications/persistent_notification_delegate.h @@ -5,10 +5,10 @@ #ifndef CHROME_BROWSER_NOTIFICATIONS_PERSISTENT_NOTIFICATION_DELEGATE_H_ #define CHROME_BROWSER_NOTIFICATIONS_PERSISTENT_NOTIFICATION_DELEGATE_H_ +#include <stdint.h> #include <string> #include "chrome/browser/notifications/notification_delegate.h" -#include "content/public/common/platform_notification_data.h" #include "url/gurl.h" namespace content { @@ -22,16 +22,13 @@ class PersistentNotificationDelegate : public NotificationDelegate { public: PersistentNotificationDelegate( content::BrowserContext* browser_context, - int64 service_worker_registration_id, - const GURL& origin, - const content::PlatformNotificationData& notification_data); + int64_t persistent_notification_id, + const GURL& origin); - int64 service_worker_registration_id() const { - return service_worker_registration_id_; - } - - const content::PlatformNotificationData& notification_data() const { - return notification_data_; + // Persistent id of this notification in the notification database. To be + // used when retrieving all information associated with it. + int64_t persistent_notification_id() const { + return persistent_notification_id_; } // NotificationDelegate implementation. @@ -45,9 +42,8 @@ class PersistentNotificationDelegate : public NotificationDelegate { private: content::BrowserContext* browser_context_; - int64 service_worker_registration_id_; + int64_t persistent_notification_id_; GURL origin_; - content::PlatformNotificationData notification_data_; std::string id_; }; diff --git a/chrome/browser/notifications/platform_notification_service_impl.cc b/chrome/browser/notifications/platform_notification_service_impl.cc index 5b0608c..4828551 100644 --- a/chrome/browser/notifications/platform_notification_service_impl.cc +++ b/chrome/browser/notifications/platform_notification_service_impl.cc @@ -36,6 +36,10 @@ #include "extensions/common/permissions/permissions_data.h" #endif +#if defined(OS_ANDROID) +#include "base/strings/string_number_conversions.h" +#endif + using content::BrowserThread; using message_center::NotifierId; @@ -61,20 +65,16 @@ PlatformNotificationServiceImpl::~PlatformNotificationServiceImpl() {} void PlatformNotificationServiceImpl::OnPersistentNotificationClick( content::BrowserContext* browser_context, - int64 service_worker_registration_id, - const std::string& notification_id, + int64_t persistent_notification_id, const GURL& origin, - const content::PlatformNotificationData& notification_data, const base::Callback<void(content::PersistentNotificationStatus)>& callback) const { DCHECK_CURRENTLY_ON(BrowserThread::UI); content::NotificationEventDispatcher::GetInstance() ->DispatchNotificationClickEvent( browser_context, + persistent_notification_id, origin, - service_worker_registration_id, - notification_id, - notification_data, callback); } @@ -218,7 +218,7 @@ void PlatformNotificationServiceImpl::DisplayNotification( void PlatformNotificationServiceImpl::DisplayPersistentNotification( content::BrowserContext* browser_context, - int64 service_worker_registration_id, + int64_t persistent_notification_id, const GURL& origin, const SkBitmap& icon, const content::PlatformNotificationData& notification_data) { @@ -228,14 +228,15 @@ void PlatformNotificationServiceImpl::DisplayPersistentNotification( DCHECK(profile); PersistentNotificationDelegate* delegate = new PersistentNotificationDelegate( - browser_context, - service_worker_registration_id, - origin, - notification_data); + browser_context, persistent_notification_id, origin); Notification notification = CreateNotificationFromData( profile, origin, icon, notification_data, delegate); + // TODO(peter): Remove this mapping when we have reliable id generation for + // the message_center::Notification objects. + persistent_notifications_[persistent_notification_id] = notification.id(); + GetNotificationUIManager()->Add(notification, profile); profile->GetHostContentSettingsMap()->UpdateLastUsage( @@ -244,14 +245,30 @@ void PlatformNotificationServiceImpl::DisplayPersistentNotification( void PlatformNotificationServiceImpl::ClosePersistentNotification( content::BrowserContext* browser_context, - const std::string& persistent_notification_id) { + int64_t persistent_notification_id) { DCHECK_CURRENTLY_ON(BrowserThread::UI); Profile* profile = Profile::FromBrowserContext(browser_context); DCHECK(profile); +#if defined(OS_ANDROID) + // TODO(peter): Remove this conversion when the notification ids are being + // generated by the caller of this method. + std::string textual_persistent_notification_id = + base::Int64ToString(persistent_notification_id); GetNotificationUIManager()->CancelById( - persistent_notification_id, NotificationUIManager::GetProfileID(profile)); + textual_persistent_notification_id, + NotificationUIManager::GetProfileID(profile)); +#else + auto iter = persistent_notifications_.find(persistent_notification_id); + if (iter == persistent_notifications_.end()) + return; + + GetNotificationUIManager()->CancelById( + iter->second, NotificationUIManager::GetProfileID(profile)); + + persistent_notifications_.erase(iter); +#endif } Notification PlatformNotificationServiceImpl::CreateNotificationFromData( diff --git a/chrome/browser/notifications/platform_notification_service_impl.h b/chrome/browser/notifications/platform_notification_service_impl.h index 85ef422..ce32031 100644 --- a/chrome/browser/notifications/platform_notification_service_impl.h +++ b/chrome/browser/notifications/platform_notification_service_impl.h @@ -5,6 +5,9 @@ #ifndef CHROME_BROWSER_NOTIFICATIONS_PLATFORM_NOTIFICATION_SERVICE_IMPL_H_ #define CHROME_BROWSER_NOTIFICATIONS_PLATFORM_NOTIFICATION_SERVICE_IMPL_H_ +#include <stdint.h> +#include <map> + #include "base/gtest_prod_util.h" #include "base/memory/singleton.h" #include "base/strings/string16.h" @@ -34,10 +37,8 @@ class PlatformNotificationServiceImpl // needed, on which the event will be fired. Must be called on the UI thread. void OnPersistentNotificationClick( content::BrowserContext* browser_context, - int64 service_worker_registration_id, - const std::string& notification_id, + int64_t persistent_notification_id, const GURL& origin, - const content::PlatformNotificationData& notification_data, const base::Callback<void(content::PersistentNotificationStatus)>& callback) const; @@ -63,13 +64,13 @@ class PlatformNotificationServiceImpl base::Closure* cancel_callback) override; void DisplayPersistentNotification( content::BrowserContext* browser_context, - int64 service_worker_registration_id, + int64_t persistent_notification_id, const GURL& origin, const SkBitmap& icon, const content::PlatformNotificationData& notification_data) override; void ClosePersistentNotification( content::BrowserContext* browser_context, - const std::string& persistent_notification_id) override; + int64_t persistent_notification_id) override; private: friend struct DefaultSingletonTraits<PlatformNotificationServiceImpl>; @@ -114,6 +115,10 @@ class PlatformNotificationServiceImpl // Weak reference. Ownership maintains with the test. NotificationUIManager* notification_ui_manager_for_tests_; + // Mapping between a persistent notification id and the id of the associated + // message_center::Notification object. Must only be used on the UI thread. + std::map<int64_t, std::string> persistent_notifications_; + DISALLOW_COPY_AND_ASSIGN(PlatformNotificationServiceImpl); }; diff --git a/chrome/browser/notifications/platform_notification_service_unittest.cc b/chrome/browser/notifications/platform_notification_service_unittest.cc index 2bd06aa..40a8eec 100644 --- a/chrome/browser/notifications/platform_notification_service_unittest.cc +++ b/chrome/browser/notifications/platform_notification_service_unittest.cc @@ -17,6 +17,10 @@ namespace { +#if !defined(OS_ANDROID) +const int64_t kPersistentNotificationId = 42; +#endif + class MockDesktopNotificationDelegate : public content::DesktopNotificationDelegate { public: @@ -126,13 +130,16 @@ TEST_F(PlatformNotificationServiceTest, DisplayPageCloseClosure) { // delegate given that it'd result in a use-after-free. } +// TODO(peter): Re-enable this test when //content is responsible for creating +// the notification delegate ids. +#if !defined(OS_ANDROID) TEST_F(PlatformNotificationServiceTest, PersistentNotificationDisplay) { content::PlatformNotificationData notification_data; notification_data.title = base::ASCIIToUTF16("My notification's title"); notification_data.body = base::ASCIIToUTF16("Hello, world!"); service()->DisplayPersistentNotification( - profile(), 42 /* sw_registration_id */, GURL("https://chrome.com/"), + profile(), kPersistentNotificationId, GURL("https://chrome.com/"), SkBitmap(), notification_data); ASSERT_EQ(1u, ui_manager()->GetNotificationCount()); @@ -144,10 +151,10 @@ TEST_F(PlatformNotificationServiceTest, PersistentNotificationDisplay) { EXPECT_EQ("Hello, world!", base::UTF16ToUTF8(notification.message())); - service()->ClosePersistentNotification(profile(), - notification.delegate_id()); + service()->ClosePersistentNotification(profile(), kPersistentNotificationId); EXPECT_EQ(0u, ui_manager()->GetNotificationCount()); } +#endif // !defined(OS_ANDROID) TEST_F(PlatformNotificationServiceTest, DisplayPageNotificationMatches) { content::PlatformNotificationData notification_data; diff --git a/content/browser/notifications/notification_event_dispatcher_impl.cc b/content/browser/notifications/notification_event_dispatcher_impl.cc index a9cdd2e..9f08158 100644 --- a/content/browser/notifications/notification_event_dispatcher_impl.cc +++ b/content/browser/notifications/notification_event_dispatcher_impl.cc @@ -5,11 +5,14 @@ #include "content/browser/notifications/notification_event_dispatcher_impl.h" #include "base/callback.h" +#include "base/strings/string_number_conversions.h" +#include "content/browser/notifications/platform_notification_context_impl.h" #include "content/browser/service_worker/service_worker_context_wrapper.h" #include "content/browser/service_worker/service_worker_registration.h" #include "content/browser/service_worker/service_worker_storage.h" #include "content/public/browser/browser_context.h" #include "content/public/browser/browser_thread.h" +#include "content/public/browser/notification_database_data.h" #include "content/public/browser/storage_partition.h" #include "content/public/common/platform_notification_data.h" @@ -61,8 +64,7 @@ void NotificationClickEventFinished( // Dispatches the notificationclick on |service_worker_registration| if the // registration was available. Must be called on the IO thread. void DispatchNotificationClickEventOnRegistration( - const std::string& notification_id, - const PlatformNotificationData& notification_data, + const NotificationDatabaseData& notification_database_data, const NotificationClickDispatchCompleteCallback& dispatch_complete_callback, ServiceWorkerStatusCode service_worker_status, const scoped_refptr<ServiceWorkerRegistration>& @@ -73,10 +75,17 @@ void DispatchNotificationClickEventOnRegistration( base::Bind(&NotificationClickEventFinished, dispatch_complete_callback, service_worker_registration); - service_worker_registration->active_version() - ->DispatchNotificationClickEvent(dispatch_event_callback, - notification_id, - notification_data); + + // TODO(peter): Pass the persistent notification id as an int64_t rather + // than as a string. This depends on the Blink API being updated. + std::string persistent_notification_id_string = + base::Int64ToString(notification_database_data.notification_id); + + service_worker_registration->active_version()-> + DispatchNotificationClickEvent( + dispatch_event_callback, + persistent_notification_id_string, + notification_database_data.notification_data); return; } @@ -115,21 +124,44 @@ void DispatchNotificationClickEventOnRegistration( // |service_worker_registration_id|. Must be called on the IO thread. void FindServiceWorkerRegistration( const GURL& origin, - int64 service_worker_registration_id, - const std::string& notification_id, - const PlatformNotificationData& notification_data, const NotificationClickDispatchCompleteCallback& dispatch_complete_callback, - scoped_refptr<ServiceWorkerContextWrapper> service_worker_context) { + scoped_refptr<ServiceWorkerContextWrapper> service_worker_context, + bool success, + const NotificationDatabaseData& notification_database_data) { DCHECK_CURRENTLY_ON(BrowserThread::IO); + if (!success) { + BrowserThread::PostTask( + BrowserThread::UI, + FROM_HERE, + base::Bind(dispatch_complete_callback, + PERSISTENT_NOTIFICATION_STATUS_DATABASE_ERROR)); + return; + } + service_worker_context->context()->storage()->FindRegistrationForId( - service_worker_registration_id, + notification_database_data.service_worker_registration_id, origin, base::Bind(&DispatchNotificationClickEventOnRegistration, - notification_id, - notification_data, + notification_database_data, dispatch_complete_callback)); } +// Reads the data associated with the |persistent_notification_id| belonging to +// |origin| from the notification context. +void ReadNotificationDatabaseData( + int64_t persistent_notification_id, + const GURL& origin, + const NotificationClickDispatchCompleteCallback& dispatch_complete_callback, + scoped_refptr<ServiceWorkerContextWrapper> service_worker_context, + scoped_refptr<PlatformNotificationContextImpl> notification_context) { + DCHECK_CURRENTLY_ON(BrowserThread::IO); + notification_context->ReadNotificationData( + persistent_notification_id, + origin, + base::Bind(&FindServiceWorkerRegistration, + origin, dispatch_complete_callback, service_worker_context)); +} + } // namespace // static @@ -149,29 +181,33 @@ NotificationEventDispatcherImpl::~NotificationEventDispatcherImpl() {} void NotificationEventDispatcherImpl::DispatchNotificationClickEvent( BrowserContext* browser_context, + int64_t persistent_notification_id, const GURL& origin, - int64 service_worker_registration_id, - const std::string& notification_id, - const PlatformNotificationData& notification_data, const NotificationClickDispatchCompleteCallback& dispatch_complete_callback) { DCHECK_CURRENTLY_ON(BrowserThread::UI); + DCHECK_GT(persistent_notification_id, 0); + DCHECK(origin.is_valid()); StoragePartition* partition = BrowserContext::GetStoragePartitionForSite(browser_context, origin); + scoped_refptr<ServiceWorkerContextWrapper> service_worker_context = static_cast<ServiceWorkerContextWrapper*>( partition->GetServiceWorkerContext()); + scoped_refptr<PlatformNotificationContextImpl> notification_context = + static_cast<PlatformNotificationContextImpl*>( + partition->GetPlatformNotificationContext()); + BrowserThread::PostTask( BrowserThread::IO, FROM_HERE, - base::Bind(&FindServiceWorkerRegistration, + base::Bind(&ReadNotificationDatabaseData, + persistent_notification_id, origin, - service_worker_registration_id, - notification_id, - notification_data, dispatch_complete_callback, - service_worker_context)); + service_worker_context, + notification_context)); } } // namespace content diff --git a/content/browser/notifications/notification_event_dispatcher_impl.h b/content/browser/notifications/notification_event_dispatcher_impl.h index 800e306..86265db 100644 --- a/content/browser/notifications/notification_event_dispatcher_impl.h +++ b/content/browser/notifications/notification_event_dispatcher_impl.h @@ -19,10 +19,8 @@ class NotificationEventDispatcherImpl : public NotificationEventDispatcher { // NotificationEventDispatcher implementation. void DispatchNotificationClickEvent( BrowserContext* browser_context, + int64_t persistent_notification_id, const GURL& origin, - int64 service_worker_registration_id, - const std::string& notification_id, - const PlatformNotificationData& notification_data, const NotificationClickDispatchCompleteCallback& dispatch_complete_callback) override; diff --git a/content/browser/notifications/notification_message_filter.cc b/content/browser/notifications/notification_message_filter.cc index f1aa17a..200dff0 100644 --- a/content/browser/notifications/notification_message_filter.cc +++ b/content/browser/notifications/notification_message_filter.cc @@ -12,6 +12,7 @@ #include "content/public/browser/browser_thread.h" #include "content/public/browser/content_browser_client.h" #include "content/public/browser/desktop_notification_delegate.h" +#include "content/public/browser/notification_database_data.h" #include "content/public/browser/platform_notification_service.h" #include "content/public/browser/render_process_host.h" #include "content/public/common/content_client.h" @@ -27,14 +28,21 @@ NotificationMessageFilter::NotificationMessageFilter( process_id_(process_id), notification_context_(notification_context), resource_context_(resource_context), - browser_context_(browser_context) {} + browser_context_(browser_context), + weak_factory_io_(this) {} NotificationMessageFilter::~NotificationMessageFilter() {} void NotificationMessageFilter::DidCloseNotification(int notification_id) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); + close_closures_.erase(notification_id); } +void NotificationMessageFilter::OnDestruct() const { + BrowserThread::DeleteOnIOThread::Destruct(this); +} + bool NotificationMessageFilter::OnMessageReceived(const IPC::Message& message) { bool handled = true; IPC_BEGIN_MESSAGE_MAP(NotificationMessageFilter, message) @@ -59,24 +67,15 @@ bool NotificationMessageFilter::OnMessageReceived(const IPC::Message& message) { void NotificationMessageFilter::OverrideThreadForMessage( const IPC::Message& message, content::BrowserThread::ID* thread) { if (message.type() == PlatformNotificationHostMsg_Show::ID || - message.type() == PlatformNotificationHostMsg_ShowPersistent::ID || - message.type() == PlatformNotificationHostMsg_Close::ID || - message.type() == PlatformNotificationHostMsg_ClosePersistent::ID) + message.type() == PlatformNotificationHostMsg_Close::ID) *thread = BrowserThread::UI; } void NotificationMessageFilter::OnCheckNotificationPermission( const GURL& origin, blink::WebNotificationPermission* permission) { DCHECK_CURRENTLY_ON(BrowserThread::IO); - PlatformNotificationService* service = - GetContentClient()->browser()->GetPlatformNotificationService(); - if (service) { - *permission = service->CheckPermissionOnIOThread(resource_context_, - origin, - process_id_); - } else { - *permission = blink::WebNotificationPermissionDenied; - } + + *permission = GetPermissionForOriginOnIO(origin); } void NotificationMessageFilter::OnShowPlatformNotification( @@ -116,25 +115,58 @@ void NotificationMessageFilter::OnShowPersistentNotification( const GURL& origin, const SkBitmap& icon, const PlatformNotificationData& notification_data) { - DCHECK_CURRENTLY_ON(BrowserThread::UI); - if (!RenderProcessHost::FromID(process_id_)) + DCHECK_CURRENTLY_ON(BrowserThread::IO); + if (GetPermissionForOriginOnIO(origin) != + blink::WebNotificationPermissionAllowed) { + BadMessageReceived(); return; + } - PlatformNotificationService* service = - GetContentClient()->browser()->GetPlatformNotificationService(); - DCHECK(service); + NotificationDatabaseData database_data; + database_data.origin = origin; + database_data.service_worker_registration_id = service_worker_registration_id; + database_data.notification_data = notification_data; + + // TODO(peter): Significantly reduce the amount of information we need to + // retain outside of the database for displaying notifications. + notification_context_->WriteNotificationData( + origin, + database_data, + base::Bind(&NotificationMessageFilter::DidWritePersistentNotificationData, + weak_factory_io_.GetWeakPtr(), + request_id, + origin, + icon, + notification_data)); +} - if (!VerifyNotificationPermissionGranted(service, origin)) - return; +void NotificationMessageFilter::DidWritePersistentNotificationData( + int request_id, + const GURL& origin, + const SkBitmap& icon, + const PlatformNotificationData& notification_data, + bool success, + int64_t persistent_notification_id) { + DCHECK_CURRENTLY_ON(BrowserThread::IO); - service->DisplayPersistentNotification(browser_context_, - service_worker_registration_id, origin, - icon, notification_data); + if (success) { + PlatformNotificationService* service = + GetContentClient()->browser()->GetPlatformNotificationService(); + DCHECK(service); + + BrowserThread::PostTask( + BrowserThread::UI, + FROM_HERE, + base::Bind(&PlatformNotificationService::DisplayPersistentNotification, + base::Unretained(service), // The service is a singleton. + browser_context_, + persistent_notification_id, + origin, + icon, + notification_data)); + } - // TODO(peter): Confirm display of the persistent notification after the - // data has been stored using the |notification_context_|. - Send(new PlatformNotificationMsg_DidShowPersistent(request_id, - true /* success */)); + Send(new PlatformNotificationMsg_DidShowPersistent(request_id, success)); } void NotificationMessageFilter::OnGetNotifications( @@ -168,27 +200,68 @@ void NotificationMessageFilter::OnClosePlatformNotification( void NotificationMessageFilter::OnClosePersistentNotification( const GURL& origin, - const std::string& persistent_notification_id) { - DCHECK_CURRENTLY_ON(BrowserThread::UI); - if (!RenderProcessHost::FromID(process_id_)) + int64_t persistent_notification_id) { + DCHECK_CURRENTLY_ON(BrowserThread::IO); + if (GetPermissionForOriginOnIO(origin) != + blink::WebNotificationPermissionAllowed) { + BadMessageReceived(); return; + } PlatformNotificationService* service = GetContentClient()->browser()->GetPlatformNotificationService(); DCHECK(service); - // TODO(peter): Use |service_worker_registration_id| and |origin| when feeding - // the close event through the notification database. + // There's no point in waiting until the database data has been removed before + // closing the notification presented to the user. Post that task immediately. + BrowserThread::PostTask( + BrowserThread::UI, + FROM_HERE, + base::Bind(&PlatformNotificationService::ClosePersistentNotification, + base::Unretained(service), // The service is a singleton. + browser_context_, + persistent_notification_id)); + + notification_context_->DeleteNotificationData( + persistent_notification_id, + origin, + base::Bind(&NotificationMessageFilter:: + DidDeletePersistentNotificationData, + weak_factory_io_.GetWeakPtr())); +} + +void NotificationMessageFilter::DidDeletePersistentNotificationData( + bool success) { + DCHECK_CURRENTLY_ON(BrowserThread::IO); + // TODO(peter): Consider feeding back to the renderer that the notification + // has been closed. +} + +blink::WebNotificationPermission +NotificationMessageFilter::GetPermissionForOriginOnIO( + const GURL& origin) const { + DCHECK_CURRENTLY_ON(BrowserThread::IO); + + PlatformNotificationService* service = + GetContentClient()->browser()->GetPlatformNotificationService(); + if (!service) + return blink::WebNotificationPermissionDenied; - service->ClosePersistentNotification(browser_context_, - persistent_notification_id); + return service->CheckPermissionOnIOThread(resource_context_, + origin, + process_id_); } bool NotificationMessageFilter::VerifyNotificationPermissionGranted( PlatformNotificationService* service, const GURL& origin) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); + blink::WebNotificationPermission permission = - service->CheckPermissionOnUIThread(browser_context_, origin, process_id_); + service->CheckPermissionOnUIThread(browser_context_, + origin, + process_id_); + if (permission == blink::WebNotificationPermissionAllowed) return true; diff --git a/content/browser/notifications/notification_message_filter.h b/content/browser/notifications/notification_message_filter.h index 9499a133..5eaed3c 100644 --- a/content/browser/notifications/notification_message_filter.h +++ b/content/browser/notifications/notification_message_filter.h @@ -8,6 +8,7 @@ #include <map> #include "base/callback_forward.h" +#include "base/memory/weak_ptr.h" #include "content/public/browser/browser_message_filter.h" #include "third_party/WebKit/public/platform/modules/notifications/WebNotificationPermission.h" @@ -35,6 +36,7 @@ class NotificationMessageFilter : public BrowserMessageFilter { void DidCloseNotification(int notification_id); // BrowserMessageFilter implementation. Called on the UI thread. + void OnDestruct() const override; bool OnMessageReceived(const IPC::Message& message) override; void OverrideThreadForMessage( const IPC::Message& message, content::BrowserThread::ID* thread) override; @@ -43,6 +45,9 @@ class NotificationMessageFilter : public BrowserMessageFilter { ~NotificationMessageFilter() override; private: + friend class base::DeleteHelper<NotificationMessageFilter>; + friend class BrowserThread; + void OnCheckNotificationPermission( const GURL& origin, blink::WebNotificationPermission* permission); void OnShowPlatformNotification( @@ -63,7 +68,29 @@ class NotificationMessageFilter : public BrowserMessageFilter { void OnClosePlatformNotification(int notification_id); void OnClosePersistentNotification( const GURL& origin, - const std::string& persistent_notification_id); + int64_t persistent_notification_id); + + // Callback to be invoked by the notification context when the notification + // data for the persistent notification may have been written, as indicated by + // |success|. Will present the notification to the user when successful. + void DidWritePersistentNotificationData( + int request_id, + const GURL& origin, + const SkBitmap& icon, + const PlatformNotificationData& notification_data, + bool success, + int64_t persistent_notification_id); + + // Callback to be invoked when the data associated with a persistent + // notification has been removed by the database, unless an error occurred, + // which will be indicated by |success|. + void DidDeletePersistentNotificationData(bool success); + + // Returns the permission status for |origin|. Must only be used on the IO + // thread. If the PlatformNotificationService is unavailable, permission will + // assumed to be denied. + blink::WebNotificationPermission GetPermissionForOriginOnIO( + const GURL& origin) const; // Verifies that Web Notification permission has been granted for |origin| in // cases where the renderer shouldn't send messages if it weren't the case. If @@ -80,6 +107,10 @@ class NotificationMessageFilter : public BrowserMessageFilter { // Map mapping notification ids to their associated close closures. std::map<int, base::Closure> close_closures_; + + base::WeakPtrFactory<NotificationMessageFilter> weak_factory_io_; + + DISALLOW_COPY_AND_ASSIGN(NotificationMessageFilter); }; } // namespace content diff --git a/content/child/notifications/notification_manager.cc b/content/child/notifications/notification_manager.cc index 6f5e8fe..96963df 100644 --- a/content/child/notifications/notification_manager.cc +++ b/content/child/notifications/notification_manager.cc @@ -5,6 +5,7 @@ #include "content/child/notifications/notification_manager.h" #include "base/lazy_instance.h" +#include "base/strings/string_number_conversions.h" #include "base/strings/utf_string_conversions.h" #include "base/thread_task_runner_handle.h" #include "base/threading/thread_local.h" @@ -164,10 +165,23 @@ void NotificationManager::close(blink::WebNotificationDelegate* delegate) { void NotificationManager::closePersistent( const blink::WebSerializedOrigin& origin, - const blink::WebString& persistent_notification_id) { + const blink::WebString& persistent_notification_id_string) { + // TODO(peter): Blink should store the persistent_notification_id as an + // int64_t instead of a string. The id, created by Chromium, is a decimal + // number that fits in an int64_t, so convert it until the API updates. + base::string16 string_value = persistent_notification_id_string; + + int64_t persistent_notification_id = 0; + if (!base::StringToInt64(string_value, + &persistent_notification_id)) { + NOTREACHED() << "Unable to close persistent notification; invalid id: " + << string_value; + return; + } + thread_safe_sender_->Send(new PlatformNotificationHostMsg_ClosePersistent( GURL(origin.string()), - base::UTF16ToUTF8(persistent_notification_id))); + persistent_notification_id)); } void NotificationManager::notifyDelegateDestroyed( diff --git a/content/common/platform_notification_messages.h b/content/common/platform_notification_messages.h index 3ed9b2f..a4ae706 100644 --- a/content/common/platform_notification_messages.h +++ b/content/common/platform_notification_messages.h @@ -99,7 +99,7 @@ IPC_MESSAGE_CONTROL1(PlatformNotificationHostMsg_Close, IPC_MESSAGE_CONTROL2(PlatformNotificationHostMsg_ClosePersistent, GURL /* origin */, - std::string /* persistent_notification_id */) + int64_t /* persistent_notification_id */) IPC_SYNC_MESSAGE_CONTROL1_1(PlatformNotificationHostMsg_CheckPermission, GURL /* origin */, diff --git a/content/public/browser/notification_event_dispatcher.h b/content/public/browser/notification_event_dispatcher.h index 0e79ff1..4509bacf 100644 --- a/content/public/browser/notification_event_dispatcher.h +++ b/content/public/browser/notification_event_dispatcher.h @@ -5,6 +5,7 @@ #ifndef CONTENT_PUBLIC_BROWSER_NOTIFICATION_EVENT_DISPATCHER_H_ #define CONTENT_PUBLIC_BROWSER_NOTIFICATION_EVENT_DISPATCHER_H_ +#include <stdint.h> #include <string> #include "base/callback_forward.h" @@ -30,14 +31,12 @@ class CONTENT_EXPORT NotificationEventDispatcher { base::Callback<void(PersistentNotificationStatus)>; // Dispatches the "notificationclick" event on the Service Worker associated - // with |origin| and |service_worker_registration_id|. The |callback| will - // be invoked when it's known whether the event successfully executed. + // with |persistent_notification_id| belonging to |origin|. The |callback| + // will be invoked when it's known whether the event successfully executed. virtual void DispatchNotificationClickEvent( BrowserContext* browser_context, + int64_t persistent_notification_id, const GURL& origin, - int64 service_worker_registration_id, - const std::string& notification_id, - const PlatformNotificationData& notification_data, const NotificationClickDispatchCompleteCallback& dispatch_complete_callback) = 0; diff --git a/content/public/browser/platform_notification_service.h b/content/public/browser/platform_notification_service.h index 52a2b72e..afcc319 100644 --- a/content/public/browser/platform_notification_service.h +++ b/content/public/browser/platform_notification_service.h @@ -5,6 +5,7 @@ #ifndef CONTENT_PUBLIC_BROWSER_PLATFORM_NOTIFICATION_SERVICE_H_ #define CONTENT_PUBLIC_BROWSER_PLATFORM_NOTIFICATION_SERVICE_H_ +#include <stdint.h> #include <string> #include "base/callback_forward.h" @@ -61,7 +62,7 @@ class CONTENT_EXPORT PlatformNotificationService { // the user. This method must be called on the UI thread. virtual void DisplayPersistentNotification( BrowserContext* browser_context, - int64 service_worker_registration_id, + int64_t persistent_notification_id, const GURL& origin, const SkBitmap& icon, const PlatformNotificationData& notification_data) = 0; @@ -70,7 +71,7 @@ class CONTENT_EXPORT PlatformNotificationService { // |persistent_notification_id|. This method must be called on the UI thread. virtual void ClosePersistentNotification( BrowserContext* browser_context, - const std::string& persistent_notification_id) = 0; + int64_t persistent_notification_id) = 0; }; } // namespace content diff --git a/content/public/common/persistent_notification_status.h b/content/public/common/persistent_notification_status.h index f77aa0e..b8f1409 100644 --- a/content/public/common/persistent_notification_status.h +++ b/content/public/common/persistent_notification_status.h @@ -20,7 +20,11 @@ enum PersistentNotificationStatus { // The event has been delivered, but the developer extended the event with a // promise that has been rejected. - PERSISTENT_NOTIFICATION_STATUS_EVENT_WAITUNTIL_REJECTED + PERSISTENT_NOTIFICATION_STATUS_EVENT_WAITUNTIL_REJECTED, + + // The event could not be delivered because the data associated with the + // notification could not be read from the database. + PERSISTENT_NOTIFICATION_STATUS_DATABASE_ERROR, }; } // content diff --git a/content/shell/browser/layout_test/layout_test_notification_manager.cc b/content/shell/browser/layout_test/layout_test_notification_manager.cc index 9eb7b44..27b6a44 100644 --- a/content/shell/browser/layout_test/layout_test_notification_manager.cc +++ b/content/shell/browser/layout_test/layout_test_notification_manager.cc @@ -24,10 +24,6 @@ void OnEventDispatchComplete(PersistentNotificationStatus status) {} LayoutTestNotificationManager::LayoutTestNotificationManager() : weak_factory_(this) {} -LayoutTestNotificationManager::PersistentNotification::PersistentNotification() - : browser_context(nullptr), - service_worker_registration_id(0) {} - LayoutTestNotificationManager::~LayoutTestNotificationManager() {} PermissionStatus LayoutTestNotificationManager::RequestPermission( @@ -84,7 +80,7 @@ void LayoutTestNotificationManager::DisplayNotification( void LayoutTestNotificationManager::DisplayPersistentNotification( BrowserContext* browser_context, - int64 service_worker_registration_id, + int64_t persistent_notification_id, const GURL& origin, const SkBitmap& icon, const PlatformNotificationData& notification_data) { @@ -96,16 +92,14 @@ void LayoutTestNotificationManager::DisplayPersistentNotification( PersistentNotification notification; notification.browser_context = browser_context; notification.origin = origin; - notification.notification_data = notification_data; - notification.service_worker_registration_id = service_worker_registration_id; - notification.persistent_id = base::GenerateGUID(); + notification.persistent_id = persistent_notification_id; persistent_notifications_[title] = notification; } void LayoutTestNotificationManager::ClosePersistentNotification( BrowserContext* browser_context, - const std::string& persistent_notification_id) { + int64_t persistent_notification_id) { for (const auto& iter : persistent_notifications_) { if (iter.second.persistent_id != persistent_notification_id) continue; @@ -134,10 +128,8 @@ void LayoutTestNotificationManager::SimulateClick(const std::string& title) { content::NotificationEventDispatcher::GetInstance() ->DispatchNotificationClickEvent( notification.browser_context, - notification.origin, - notification.service_worker_registration_id, notification.persistent_id, - notification.notification_data, + notification.origin, base::Bind(&OnEventDispatchComplete)); } diff --git a/content/shell/browser/layout_test/layout_test_notification_manager.h b/content/shell/browser/layout_test/layout_test_notification_manager.h index 45f9b7b..d2cd2f5 100644 --- a/content/shell/browser/layout_test/layout_test_notification_manager.h +++ b/content/shell/browser/layout_test/layout_test_notification_manager.h @@ -5,6 +5,7 @@ #ifndef CONTENT_SHELL_BROWSER_LAYOUT_TEST_LAYOUT_TEST_NOTIFICATION_MANAGER_H_ #define CONTENT_SHELL_BROWSER_LAYOUT_TEST_LAYOUT_TEST_NOTIFICATION_MANAGER_H_ +#include <stdint.h> #include <map> #include <string> @@ -13,7 +14,6 @@ #include "base/synchronization/lock.h" #include "content/public/browser/platform_notification_service.h" #include "content/public/common/permission_status.mojom.h" -#include "content/public/common/platform_notification_data.h" #include "third_party/WebKit/public/platform/modules/notifications/WebNotificationPermission.h" #include "url/gurl.h" @@ -70,13 +70,13 @@ class LayoutTestNotificationManager : public PlatformNotificationService { base::Closure* cancel_callback) override; void DisplayPersistentNotification( BrowserContext* browser_context, - int64 service_worker_registration_id, + int64_t persistent_notification_id, const GURL& origin, const SkBitmap& icon, const PlatformNotificationData& notification_data) override; void ClosePersistentNotification( BrowserContext* browser_context, - const std::string& persistent_notification_id) override; + int64_t persistent_notification_id) override; private: // Closes the notification titled |title|. Must be called on the UI thread. @@ -90,13 +90,9 @@ class LayoutTestNotificationManager : public PlatformNotificationService { // Structure to represent the information of a persistent notification. struct PersistentNotification { - PersistentNotification(); - - BrowserContext* browser_context; + BrowserContext* browser_context = nullptr; GURL origin; - int64 service_worker_registration_id; - PlatformNotificationData notification_data; - std::string persistent_id; + int64_t persistent_id = 0; }; std::map<GURL, blink::WebNotificationPermission> permission_map_; |