Make //content responsible for generating notification Ids

Generate Web Notification Ids in the //content layer

Today, Web Notifications get a random Id assigned to them by the delegate
used with the message_center::Notification object. This has a number of
implications:

  (1) We don't get super smooth "update" transitions for message center
      notifications upon replacing a notification.
  (2) We continue to rely on the delegates associated with Notification
      objects, which in themselves are incompatible with notifications
      that can outlive the process they were created by.
  (3) It's not possible for the //content layer, that has knowledge of the
      notifications that *should* be showing, to close them when certain
      events occur (for instance, the Service Worker unregistering). That
      leads to unresponsive notifications.

This patch makes the NotificationIdGenerator responsible for creating the
notification Ids, giving us a clear path towards solving these issues. In
addition, it further isolates the concept of persistent notification Ids,
reducing the number of Ids readers of this code have to keep in mind.

The NotificationDatabase's keying scheme has been changed to refer to the
generated Id as opposed to the ever-incrementing persistent notification
ID. This avoids needing separate association keys and removes a DELETE
operation prior to writing data for a potentially replaced notification.

TBR=mkwst (type change in /web/)
BUG=485985, 439950
Committed: https://crrev.com/c45944c33233a04614dca6eeed41f75e75a943be
Cr-Commit-Position: refs/heads/master@{#418241}

[Peter Beverloo, 2016-09-01 15:56:49]

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

[commit-bot: I haz the power, 2016-09-01 15:57:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 16:06:49]

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

[commit-bot: I haz the power, 2016-09-01 16:06:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Peter Beverloo, 2016-09-01 17:39:53]

Patchset #1 (id:1) has been deleted

[Peter Beverloo, 2016-09-01 17:40:00]

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

[commit-bot: I haz the power, 2016-09-01 17:40:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 18:01:59]

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

[commit-bot: I haz the power, 2016-09-01 18:02:00]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Peter Beverloo, 2016-09-02 12:00:26]

Patchset #1 (id:20001) has been deleted

[Peter Beverloo, 2016-09-02 12:00:32]

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

[commit-bot: I haz the power, 2016-09-02 12:00:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Beverloo, 2016-09-02 12:20:30]

Description was changed from

==========
Make //content responsible for generating notification Ids

BUG=
==========

to

==========
Generate Web Notification Ids in the //content layer

Today, Web Notifications get a random Id assigned to them by the delegate
used with the message_center::Notification object. This has a number of
implications:

  (1) We don't get super smooth "update" transitions for message center
      notifications upon replacing a notification.
  (2) We continue to rely on the delegates associated with Notification
      objects, which in themselves are incompatible with notifications
      that can outlive the process they were created by.
  (3) It's not possible for the //content layer, that has knowledge of the
      notifications that *should* be showing, to close them when certain
      events occur (for instance, the Service Worker unregistering). That
      leads to unresponsive notifications.

This patch makes the NotificationIdGenerator responsible for creating the
notification Ids, giving us a clear path towards solving these issues. In
addition, it further isolates the concept of persistent notification Ids,
reducing the number of Ids readers of this code have to keep in mind.

BUG=485985, 439950
==========

[Peter Beverloo, 2016-09-02 12:21:31]

peter@chromium.org changed reviewers:
+ johnme@chromium.org, miguelg@chromium.org

[Peter Beverloo, 2016-09-02 12:21:31]

+miguelg for //chrome
+johnme for the rest

Please disregard the PlatformNotificationContextImpl and NotificationDatabase
changes, I need to think about how to do them properly. (They're now just hacked
in.)

[commit-bot: I haz the power, 2016-09-02 12:26:07]

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

[commit-bot: I haz the power, 2016-09-02 12:26:08]

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

[Miguel Garcia, 2016-09-02 14:33:16]

Had a look at the chrome side and the generator itself while the rest gets
sorted out.

Nice job

https://codereview.chromium.org/2300093002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_android.cc
(right):

https://codereview.chromium.org/2300093002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_android.cc:15:
#include "base/strings/string_number_conversions.h"
I think this include can go as well (could go before but since you are removing
the conversions everywhere we might just as well clean up now)

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
File content/browser/notifications/notification_id_generator.cc (right):

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_id_generator.cc:22: const char
kPersistentPrefix[] = "p:";
perhaps document these constants?

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_id_generator.cc:73: stream <<
base::IntToString(browser_context_->IsOffTheRecord());
why do you need to pass this to generate the notification id?

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_id_generator.cc:76: // Persistent
notification ids are unique for the lifetime of the notification
why does this comment belong here? Seems an odd place to document it.

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_id_generator.cc:103: //
Non-persistent notification ids are unique per renderer process when no
Can you explain why you need the different rules for uniqueness? why not just
make all the ids unique forever regardless of the type?

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
File content/browser/notifications/notification_id_generator.h (right):

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_id_generator.h:20: // Generates
deterministic notification ids for Web Notifications.
Is this still only for web notifications?

[johnme, 2016-09-02 15:07:44]

Looks fantastic! It's so nice to have consistent names and types for the
different kinds of notification ids, and for chrome/ to only have to worry about
one of them now.

https://codereview.chromium.org/2300093002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_unittest.cc
(right):

https://codereview.chromium.org/2300093002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_unittest.cc:191: //
TODO(peter): Re-enable this test when //content is responsible for creating
Can you re-enable this test now?

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
File content/browser/notifications/notification_database.cc (right):

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_database.cc:28: // key: "DATA:"
<origin identifier> '\x00' <notification_id>
persistent_notification_id

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_database.cc:215:
storage_data.notification_id = next_notification_id_;
Please also rename notification_id in
content/browser/notifications/notification_database_data.proto to
persistent_notification_id

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
File content/browser/notifications/notification_id_generator.h (right):

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_id_generator.h:61: // notification id
will have created by the renderer with |render_process_id|.
Nit: have been

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
File content/browser/notifications/notification_message_filter.h (right):

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_message_filter.h:44: // To be called
by non-persistent notification delegates when they has closed,
s/has/have been/ or s/has/are/

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_message_filter.h:45: // so that the
close closure associated with that notification can be removed.
Nit: s/closure/callback/ for clarity?

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_message_filter.h:144: // Map mapping
notification Ids associated with non-persistent notifications
Nit: ids or IDs

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
File content/browser/notifications/platform_notification_context_impl.cc
(right):

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:172:
NotificationDatabase::Status status = database_->ReadIdAssociation(
I guess this is the hacked in part you were talking about?

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:355: status
== NotificationDatabase::STATUS_ERROR_NOT_FOUND;
Seems reasonable. Does changing this have any side-effects?

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
File content/browser/notifications/platform_notification_context_unittest.cc
(right):

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context_unittest.cc:216:
ASSERT_EQ(notification_id(), read_notification_id);
Are there any downsides to notification_id() no longer changing?

https://codereview.chromium.org/2300093002/diff/40001/content/child/notificat...
File content/child/notifications/notification_manager.h (right):

https://codereview.chromium.org/2300093002/diff/40001/content/child/notificat...
content/child/notifications/notification_manager.h:13: #include <tuple>
Unnecessary?

https://codereview.chromium.org/2300093002/diff/40001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_notification_manager.cc
(right):

https://codereview.chromium.org/2300093002/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_notification_manager.cc:82: const
auto persistent_iter = persistent_notifications_.find(notification_id);
Just do: persistent_notifications_.erase(notification_id);

https://codereview.chromium.org/2300093002/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_notification_manager.cc:189:
delete delegate;
I realise these were being deleted before, but I'm confused - do we own these?
It doesn't seem like LayoutTestNotificationManager deletes the delegates in any
other cases (and presumably if we owned them they'd be unique_ptrs).

[Peter Beverloo, 2016-09-05 15:10:32]

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

[commit-bot: I haz the power, 2016-09-05 15:10:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Beverloo, 2016-09-05 15:11:01]

All done, I also fixed test failures. This is not ready for a re-review yet
because of the PlatformNotificationContext/NotificationDatabase clean-ups.

https://codereview.chromium.org/2300093002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_android.cc
(right):

https://codereview.chromium.org/2300093002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_android.cc:15:
#include "base/strings/string_number_conversions.h"
On 2016/09/02 14:33:16, Miguel Garcia wrote:
> I think this include can go as well (could go before but since you are
removing
> the conversions everywhere we might just as well clean up now)

Done.

https://codereview.chromium.org/2300093002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_unittest.cc
(right):

https://codereview.chromium.org/2300093002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_unittest.cc:191: //
TODO(peter): Re-enable this test when //content is responsible for creating
On 2016/09/02 15:07:44, johnme wrote:
> Can you re-enable this test now?

I have no idea. Let's try!

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
File content/browser/notifications/notification_database.cc (right):

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_database.cc:28: // key: "DATA:"
<origin identifier> '\x00' <notification_id>
On 2016/09/02 15:07:44, johnme wrote:
> persistent_notification_id

Done.

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_database.cc:215:
storage_data.notification_id = next_notification_id_;
On 2016/09/02 15:07:44, johnme wrote:
> Please also rename notification_id in
> content/browser/notifications/notification_database_data.proto to
> persistent_notification_id

Done, also in the NotificationDatabaseData class, and the next_notification_id_
member here.

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
File content/browser/notifications/notification_id_generator.cc (right):

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_id_generator.cc:22: const char
kPersistentPrefix[] = "p:";
On 2016/09/02 14:33:16, Miguel Garcia wrote:
> perhaps document these constants?

With what? I don't think "Prefix applied to notification ids for persistent
notifications" adds a tremendous amount of value. I've renamed them to
kPersistent*Notification*Prefix instead, does that help?

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_id_generator.cc:73: stream <<
base::IntToString(browser_context_->IsOffTheRecord());
On 2016/09/02 14:33:16, Miguel Garcia wrote:
> why do you need to pass this to generate the notification id?

The ComputeBrowserContextHash() will yield the same value for normal and
incognito profiles I believe. OffTheRecordProfile::GetPath() returns the path of
its parent profile.

It's sad that Web Notifications don't work in Incognito mode. We shouldn't
continue to add barriers that work against making that happen.

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_id_generator.cc:76: // Persistent
notification ids are unique for the lifetime of the notification
On 2016/09/02 14:33:16, Miguel Garcia wrote:
> why does this comment belong here? Seems an odd place to document it.

It's redundant with the class-level command. I've removed it.

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_id_generator.cc:103: //
Non-persistent notification ids are unique per renderer process when no
On 2016/09/02 14:33:16, Miguel Garcia wrote:
> Can you explain why you need the different rules for uniqueness? why not just
> make all the ids unique forever regardless of the type?

We currently don't store any state for non-persistent notifications, it's all
ephemeral. Solving this would mean we either (a) stick non-persistent
notifications in the Notification database, which serves no real purpose, or (b)
invent another mechanism for continuous Id counting.

The current mechanism works fine, I don't think we should add a bunch of
complexity to change it.

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
File content/browser/notifications/notification_id_generator.h (right):

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_id_generator.h:20: // Generates
deterministic notification ids for Web Notifications.
On 2016/09/02 14:33:16, Miguel Garcia wrote:
> Is this still only for web notifications?

Yes. Web Notifications is the combination of persistent and non-persistent
notifications.

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_id_generator.h:61: // notification id
will have created by the renderer with |render_process_id|.
On 2016/09/02 15:07:44, johnme wrote:
> Nit: have been

Done.

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
File content/browser/notifications/notification_message_filter.h (right):

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_message_filter.h:44: // To be called
by non-persistent notification delegates when they has closed,
On 2016/09/02 15:07:44, johnme wrote:
> s/has/have been/ or s/has/are/

Done.

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_message_filter.h:45: // so that the
close closure associated with that notification can be removed.
On 2016/09/02 15:07:44, johnme wrote:
> Nit: s/closure/callback/ for clarity?

A closure is a callback that takes no arguments. The member in which they're
stored is also called close_closures_, as it is throughout the code. Let's not
diverge?

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_message_filter.h:144: // Map mapping
notification Ids associated with non-persistent notifications
On 2016/09/02 15:07:44, johnme wrote:
> Nit: ids or IDs

Done.

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
File content/browser/notifications/platform_notification_context_impl.cc
(right):

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:172:
NotificationDatabase::Status status = database_->ReadIdAssociation(
On 2016/09/02 15:07:44, johnme wrote:
> I guess this is the hacked in part you were talking about?

No? This is kinda fine...

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:355: status
== NotificationDatabase::STATUS_ERROR_NOT_FOUND;
On 2016/09/02 15:07:44, johnme wrote:
> Seems reasonable. Does changing this have any side-effects?

No. LevelDB already returns STATUS_OK instead of ERROR_NOT_FOUND when deleting a
key that doesn't exist.

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
File content/browser/notifications/platform_notification_context_unittest.cc
(right):

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context_unittest.cc:216:
ASSERT_EQ(notification_id(), read_notification_id);
On 2016/09/02 15:07:44, johnme wrote:
> Are there any downsides to notification_id() no longer changing?

Extra LevelDB key look-up. I'm not too worried about that, since it's optimized
for that. Upsides are described in the CL's description.

https://codereview.chromium.org/2300093002/diff/40001/content/child/notificat...
File content/child/notifications/notification_manager.h (right):

https://codereview.chromium.org/2300093002/diff/40001/content/child/notificat...
content/child/notifications/notification_manager.h:13: #include <tuple>
On 2016/09/02 15:07:44, johnme wrote:
> Unnecessary?

Done.

https://codereview.chromium.org/2300093002/diff/40001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_notification_manager.cc
(right):

https://codereview.chromium.org/2300093002/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_notification_manager.cc:82: const
auto persistent_iter = persistent_notifications_.find(notification_id);
On 2016/09/02 15:07:44, johnme wrote:
> Just do: persistent_notifications_.erase(notification_id);

Done.

https://codereview.chromium.org/2300093002/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_notification_manager.cc:189:
delete delegate;
On 2016/09/02 15:07:44, johnme wrote:
> I realise these were being deleted before, but I'm confused - do we own these?
> It doesn't seem like LayoutTestNotificationManager deletes the delegates in
any
> other cases (and presumably if we owned them they'd be unique_ptrs).

Done.

[commit-bot: I haz the power, 2016-09-05 15:51:57]

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

[commit-bot: I haz the power, 2016-09-05 15:51:58]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Peter Beverloo, 2016-09-06 15:14:51]

Description was changed from

==========
Generate Web Notification Ids in the //content layer

Today, Web Notifications get a random Id assigned to them by the delegate
used with the message_center::Notification object. This has a number of
implications:

  (1) We don't get super smooth "update" transitions for message center
      notifications upon replacing a notification.
  (2) We continue to rely on the delegates associated with Notification
      objects, which in themselves are incompatible with notifications
      that can outlive the process they were created by.
  (3) It's not possible for the //content layer, that has knowledge of the
      notifications that *should* be showing, to close them when certain
      events occur (for instance, the Service Worker unregistering). That
      leads to unresponsive notifications.

This patch makes the NotificationIdGenerator responsible for creating the
notification Ids, giving us a clear path towards solving these issues. In
addition, it further isolates the concept of persistent notification Ids,
reducing the number of Ids readers of this code have to keep in mind.

BUG=485985, 439950
==========

to

==========
Generate Web Notification Ids in the //content layer

Today, Web Notifications get a random Id assigned to them by the delegate
used with the message_center::Notification object. This has a number of
implications:

  (1) We don't get super smooth "update" transitions for message center
      notifications upon replacing a notification.
  (2) We continue to rely on the delegates associated with Notification
      objects, which in themselves are incompatible with notifications
      that can outlive the process they were created by.
  (3) It's not possible for the //content layer, that has knowledge of the
      notifications that *should* be showing, to close them when certain
      events occur (for instance, the Service Worker unregistering). That
      leads to unresponsive notifications.

This patch makes the NotificationIdGenerator responsible for creating the
notification Ids, giving us a clear path towards solving these issues. In
addition, it further isolates the concept of persistent notification Ids,
reducing the number of Ids readers of this code have to keep in mind.

The NotificationDatabase's keying scheme has been changed to refer to the
generated Id as opposed to the ever-incrementing persistent notification
ID. This avoids needing separate association keys and removes a DELETE
operation prior to writing data for a potentially replaced notification.

BUG=485985, 439950
==========

[Peter Beverloo, 2016-09-06 15:20:31]

Patchset #3 (id:80001) has been deleted

[Peter Beverloo, 2016-09-06 15:21:48]

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

[commit-bot: I haz the power, 2016-09-06 15:22:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Beverloo, 2016-09-06 15:22:34]

This is now ready for formal review.

+miguelg for //chrome
+johnme for everything else

I'll add IPC and LDB owners once you're ready.

[commit-bot: I haz the power, 2016-09-06 18:16:00]

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

[commit-bot: I haz the power, 2016-09-06 18:16:01]

Dry run: This issue passed the CQ dry run.

[johnme, 2016-09-07 17:13:14]

Looking good so far. Some initial nits; more to come.

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
File content/browser/notifications/notification_database.cc (right):

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.cc:33: // value: Decimal
string which fits into an int64_t.
"Decimal string that fits into an int64_t, containing the next persistent
notification id."

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.cc:39: const char
kNextNotificationIdKey[] = "NEXT_NOTIFICATION_ID";
kNextPersistentNotificationIdKey?

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.cc:73:
storage::GetIdentifierFromOrigin(origin).c_str(),
Drive-by: DatabaseIdentifier::CreateFromOrigin has some quirks, for example it
returns DatabaseIdentifier::UniqueFileIdentifier() for file:// URLs. Though I
guess we don't currently support persistent notifications on file:// URLs, since
we don't support SWs there?

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.cc:216: if
(written_persistent_notification_id_ != next_persistent_notification_id_) {
I like that if you create two and successfully write only the second one, it
increases the stored next id by 2. If you create two notifications and only
successfully write only the first one, we'll still increase the stored next id
by 2, but nevermind.

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
File content/browser/notifications/notification_database.h (right):

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.h:81: // Gets the next
assignable notification ID. Subsequent calls to this method
persistent notification ID

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.h:84: int64_t
GetNextNotificationId();
GetNextPersistentNotificationId

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.h:92:
NotificationDatabaseData* notification_database_data) const;
Nit: notification_data since you renamed it in WriteNotificationData (ditto in
comment above)

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.h:159: // Reads the next
available notification id from the database and returns
persistent notification id

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.h:161: // the
|next_notification_id_| member.
next_persistent_notification_id

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.h:162: Status
ReadNextNotificationId();
ReadNextPersistentNotificationId

[Miguel Garcia, 2016-09-07 17:32:33]

chrome layer lgtm. cool stuff

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
File content/browser/notifications/notification_id_generator.cc (right):

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_id_generator.cc:22: const char
kPersistentPrefix[] = "p:";
On 2016/09/05 15:11:01, Peter Beverloo wrote:
> On 2016/09/02 14:33:16, Miguel Garcia wrote:
> > perhaps document these constants?
> 
> With what? I don't think "Prefix applied to notification ids for persistent
> notifications" adds a tremendous amount of value. I've renamed them to
> kPersistent*Notification*Prefix instead, does that help?

Acknowledged.

https://codereview.chromium.org/2300093002/diff/40001/content/browser/notific...
content/browser/notifications/notification_id_generator.cc:103: //
Non-persistent notification ids are unique per renderer process when no
Works for me

On 2016/09/05 15:11:01, Peter Beverloo wrote:
> On 2016/09/02 14:33:16, Miguel Garcia wrote:
> > Can you explain why you need the different rules for uniqueness? why not
just
> > make all the ids unique forever regardless of the type?
> 
> We currently don't store any state for non-persistent notifications, it's all
> ephemeral. Solving this would mean we either (a) stick non-persistent
> notifications in the Notification database, which serves no real purpose, or
(b)
> invent another mechanism for continuous Id counting.
> 
> The current mechanism works fine, I don't think we should add a bunch of
> complexity to change it.

https://codereview.chromium.org/2300093002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/persistent_notification_handler.cc (right):

https://codereview.chromium.org/2300093002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/persistent_notification_handler.cc:19: return;  //
no need to propagate programmatic close events
nit: I think propagate back was better.

[Peter Beverloo, 2016-09-08 13:17:12]

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

[commit-bot: I haz the power, 2016-09-08 13:17:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Beverloo, 2016-09-08 13:18:56]

https://codereview.chromium.org/2300093002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/persistent_notification_handler.cc (right):

https://codereview.chromium.org/2300093002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/persistent_notification_handler.cc:19: return;  //
no need to propagate programmatic close events
On 2016/09/07 17:32:33, Miguel Garcia wrote:
> nit: I think propagate back was better.

Done.

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
File content/browser/notifications/notification_database.cc (right):

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.cc:33: // value: Decimal
string which fits into an int64_t.
On 2016/09/07 17:13:14, johnme wrote:
> "Decimal string that fits into an int64_t, containing the next persistent
> notification id."

Done.

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.cc:39: const char
kNextNotificationIdKey[] = "NEXT_NOTIFICATION_ID";
On 2016/09/07 17:13:13, johnme wrote:
> kNextPersistentNotificationIdKey?

The name ideally should match the key. We shouldn't rename the key, so let's
keep as-is.

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.cc:73:
storage::GetIdentifierFromOrigin(origin).c_str(),
On 2016/09/07 17:13:14, johnme wrote:
> Drive-by: DatabaseIdentifier::CreateFromOrigin has some quirks, for example it
> returns DatabaseIdentifier::UniqueFileIdentifier() for file:// URLs. Though I
> guess we don't currently support persistent notifications on file:// URLs,
since
> we don't support SWs there?

Yes.

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.cc:216: if
(written_persistent_notification_id_ != next_persistent_notification_id_) {
On 2016/09/07 17:13:13, johnme wrote:
> I like that if you create two and successfully write only the second one, it
> increases the stored next id by 2. If you create two notifications and only
> successfully write only the first one, we'll still increase the stored next id
> by 2, but nevermind.

That's fine. We have space for another 9,223,372,036,854,775,807 notifications
:-).

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
File content/browser/notifications/notification_database.h (right):

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.h:81: // Gets the next
assignable notification ID. Subsequent calls to this method
On 2016/09/07 17:13:14, johnme wrote:
> persistent notification ID

Done.

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.h:84: int64_t
GetNextNotificationId();
On 2016/09/07 17:13:14, johnme wrote:
> GetNextPersistentNotificationId

Done.

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.h:92:
NotificationDatabaseData* notification_database_data) const;
On 2016/09/07 17:13:14, johnme wrote:
> Nit: notification_data since you renamed it in WriteNotificationData (ditto in
> comment above)

Done.

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.h:159: // Reads the next
available notification id from the database and returns
On 2016/09/07 17:13:14, johnme wrote:
> persistent notification id

Done.

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.h:161: // the
|next_notification_id_| member.
On 2016/09/07 17:13:14, johnme wrote:
> next_persistent_notification_id

Done.

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database.h:162: Status
ReadNextNotificationId();
On 2016/09/07 17:13:14, johnme wrote:
> ReadNextPersistentNotificationId

Done.

[Peter Beverloo, 2016-09-08 13:23:51]

peter@chromium.org changed reviewers:
+ cmumford@chromium.org, falken@chromium.org, piman@chromium.org,
tsepez@chromium.org

[Peter Beverloo, 2016-09-08 13:23:52]

+piman for //content/public/
+tsepez for IPC changes (2 *messages.h files)
+falken for Service Worker plumbing changes
+cmumford for //content/browser/notifications/notification_database_*. Changing
this now is safe because there are no platforms where notifications survive
Chrome updates *yet*.

[commit-bot: I haz the power, 2016-09-08 15:20:48]

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

[commit-bot: I haz the power, 2016-09-08 15:20:49]

Dry run: This issue passed the CQ dry run.

[johnme, 2016-09-08 15:21:51]

lgtm with nits - thanks again for cleaning this up!

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
File content/browser/notifications/notification_database_data.proto (left):

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database_data.proto:16: optional
int64 notification_id = 1;
Keep a comment documenting what tag #1 was used for?

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
File content/browser/notifications/notification_database_data.proto (right):

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database_data.proto:16: optional
string notification_id = 5;
If this database does in fact get blown away by Chrome updates, could you keep
this as #1? I guess not, because that would cause problems when
DeserializeNotificationDatabaseData tries to deserialize any stale values to a
string?

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
File content/browser/notifications/notification_database_unittest.cc (right):

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database_unittest.cc:64:
database_data.notification_id = base::GenerateGUID();
Why is there a GUID here? If this is just to generate a arbitrary random string
for the test, please make a wrapper GenerateNotificationId() function, in which
you have a comment explaining that GenerateGUID is just an easy way to get a
random string (I'm assuming that the ids need to be different; otherwise you'd
have used a kNotificationId constant).

Ditto everywhere below that you use GenerateGUID.

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database_unittest.cc:267:
database->ReadNotificationData("my-id", GURL("https://chrome.com"),
Nit: s/my-id/bad-id/ ?

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database_unittest.cc:402:
database->DeleteNotificationData("9001", GURL("https://chrome.com")));
Nit: s/9001/bad-id/

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
File content/browser/notifications/platform_notification_context_impl.cc
(right):

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/platform_notification_context_impl.cc:370: //
TODO(peter): Close the notifications in |deleted_notification_ids|. See
Can this TODO be fixed with the new ID scheme?

https://codereview.chromium.org/2300093002/diff/100001/content/content_browse...
File content/content_browser.gypi (right):

https://codereview.chromium.org/2300093002/diff/100001/content/content_browse...
content/content_browser.gypi:234:
'public/browser/notification_database_data.cc',
(You'll need to move this to content/public/browser/BUILD.gn when rebasing)

https://codereview.chromium.org/2300093002/diff/100001/content/public/browser...
File content/public/browser/platform_notification_context.h (right):

https://codereview.chromium.org/2300093002/diff/100001/content/public/browser...
content/public/browser/platform_notification_context.h:56: // Writes the data
associated with a notification to a database. When this
Add "The |notification_id| field of |database_data| will be overwritten with a
newly generated ID.".

And perhaps also add a "database_data.notification_id will be generated for us"
comment to the callers for good measure, as this is rather subtle?

https://codereview.chromium.org/2300093002/diff/100001/content/public/browser...
content/public/browser/platform_notification_context.h:58: // the persistent
notification id when written successfully.
s/persistent //

https://codereview.chromium.org/2300093002/diff/120001/content/browser/notifi...
File content/browser/notifications/notification_database.cc (right):

https://codereview.chromium.org/2300093002/diff/120001/content/browser/notifi...
content/browser/notifications/notification_database.cc:323: return status;
I'm concerned that notifications that pre-date the Chrome upgrade which
introduces this patch may remain in the database, and fail to be deserialized
here. What happens when you read a non-existent string value from a proto?

Ditto below in DeleteAllNotificationDataInternal.

It seems both cases would both be self-fixing (since the database would be
considered corrupted), but they may cause some post-upgrade notifications to be
lost as a result.

[Tom Sepez, 2016-09-08 15:49:45]

Messages LGTM. Also checked your new string form comes from base::GenerateGUID
and should be unguessable.

[piman, 2016-09-08 17:12:49]

lgtm

https://codereview.chromium.org/2300093002/diff/120001/content/public/browser...
File content/public/browser/notification_database_data.h (right):

https://codereview.chromium.org/2300093002/diff/120001/content/public/browser...
content/public/browser/notification_database_data.h:22:
NotificationDatabaseData(const NotificationDatabaseData& other);
nit: worth adding a move constructor to avoid copying strings (explicit
definition of the copy constructor disables the implicit move constructor that
was there before).

Also, please consider adding copy-assign and move-assign operators
(rule-of-3/rule-of-5, see
http://en.cppreference.com/w/cpp/language/rule_of_three).

[Peter Beverloo, 2016-09-08 18:56:03]

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
File content/browser/notifications/notification_database_data.proto (left):

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database_data.proto:16: optional
int64 notification_id = 1;
On 2016/09/08 15:21:50, johnme wrote:
> Keep a comment documenting what tag #1 was used for?

Done.

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
File content/browser/notifications/notification_database_data.proto (right):

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database_data.proto:16: optional
string notification_id = 5;
On 2016/09/08 15:21:50, johnme wrote:
> If this database does in fact get blown away by Chrome updates, could you keep
> this as #1? I guess not, because that would cause problems when
> DeserializeNotificationDatabaseData tries to deserialize any stale values to a
> string?

Yes.

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
File content/browser/notifications/notification_database_unittest.cc (right):

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database_unittest.cc:64:
database_data.notification_id = base::GenerateGUID();
On 2016/09/08 15:21:50, johnme wrote:
> Why is there a GUID here? If this is just to generate a arbitrary random
string
> for the test, please make a wrapper GenerateNotificationId() function, in
which
> you have a comment explaining that GenerateGUID is just an easy way to get a
> random string (I'm assuming that the ids need to be different; otherwise you'd
> have used a kNotificationId constant).
> 
> Ditto everywhere below that you use GenerateGUID.

Done.

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database_unittest.cc:267:
database->ReadNotificationData("my-id", GURL("https://chrome.com"),
On 2016/09/08 15:21:50, johnme wrote:
> Nit: s/my-id/bad-id/ ?

Done.

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_database_unittest.cc:402:
database->DeleteNotificationData("9001", GURL("https://chrome.com")));
On 2016/09/08 15:21:50, johnme wrote:
> Nit: s/9001/bad-id/

Done.

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
File content/browser/notifications/platform_notification_context_impl.cc
(right):

https://codereview.chromium.org/2300093002/diff/100001/content/browser/notifi...
content/browser/notifications/platform_notification_context_impl.cc:370: //
TODO(peter): Close the notifications in |deleted_notification_ids|. See
On 2016/09/08 15:21:50, johnme wrote:
> Can this TODO be fixed with the new ID scheme?

Yes.

https://codereview.chromium.org/2300093002/diff/100001/content/content_browse...
File content/content_browser.gypi (right):

https://codereview.chromium.org/2300093002/diff/100001/content/content_browse...
content/content_browser.gypi:234:
'public/browser/notification_database_data.cc',
On 2016/09/08 15:21:50, johnme wrote:
> (You'll need to move this to content/public/browser/BUILD.gn when rebasing)

Acknowledged.

https://codereview.chromium.org/2300093002/diff/100001/content/public/browser...
File content/public/browser/platform_notification_context.h (right):

https://codereview.chromium.org/2300093002/diff/100001/content/public/browser...
content/public/browser/platform_notification_context.h:56: // Writes the data
associated with a notification to a database. When this
On 2016/09/08 15:21:50, johnme wrote:
> Add "The |notification_id| field of |database_data| will be overwritten with a
> newly generated ID.".
> 
> And perhaps also add a "database_data.notification_id will be generated for
us"
> comment to the callers for good measure, as this is rather subtle?

Done.

https://codereview.chromium.org/2300093002/diff/100001/content/public/browser...
content/public/browser/platform_notification_context.h:58: // the persistent
notification id when written successfully.
On 2016/09/08 15:21:50, johnme wrote:
> s/persistent //

Done.

https://codereview.chromium.org/2300093002/diff/120001/content/browser/notifi...
File content/browser/notifications/notification_database.cc (right):

https://codereview.chromium.org/2300093002/diff/120001/content/browser/notifi...
content/browser/notifications/notification_database.cc:323: return status;
On 2016/09/08 15:21:50, johnme wrote:
> I'm concerned that notifications that pre-date the Chrome upgrade which
> introduces this patch may remain in the database, and fail to be deserialized
> here. What happens when you read a non-existent string value from a proto?
> 
> Ditto below in DeleteAllNotificationDataInternal.
> 
> It seems both cases would both be self-fixing (since the database would be
> considered corrupted), but they may cause some post-upgrade notifications to
be
> lost as a result.

Why would it fail to deserialize? That'd work just fine, because that's how
protobufs work. The string would be the empty string.

This issue indeed is self-correcting. I've added two conditionals in the cases
where we *might* hit this to make sure they're not exposed anywhere until they
get removed.

https://codereview.chromium.org/2300093002/diff/120001/content/public/browser...
File content/public/browser/notification_database_data.h (right):

https://codereview.chromium.org/2300093002/diff/120001/content/public/browser...
content/public/browser/notification_database_data.h:22:
NotificationDatabaseData(const NotificationDatabaseData& other);
On 2016/09/08 17:12:49, piman wrote:
> nit: worth adding a move constructor to avoid copying strings (explicit
> definition of the copy constructor disables the implicit move constructor that
> was there before).
> 
> Also, please consider adding copy-assign and move-assign operators
> (rule-of-3/rule-of-5, see
> http://en.cppreference.com/w/cpp/language/rule_of_three).

I've added the assignment operator to this class. There is one deliberate copy
of this class now - in future CLs I plan to (a) remove NotificationDatabaseData
from the //content API, and (b) remove the need to copy the data at all.

Would you recommend a pass-by-value paradigm with std::move() (where possible)
over passing it by reference, as we currently do?

[Peter Beverloo, 2016-09-08 18:56:09]

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

[commit-bot: I haz the power, 2016-09-08 18:56:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 20:52:28]

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

[commit-bot: I haz the power, 2016-09-08 20:52:30]

Dry run: This issue passed the CQ dry run.

[falken, 2016-09-09 07:46:11]

service worker lgtm

[Peter Beverloo, 2016-09-13 12:29:23]

Description was changed from

==========
Generate Web Notification Ids in the //content layer

Today, Web Notifications get a random Id assigned to them by the delegate
used with the message_center::Notification object. This has a number of
implications:

  (1) We don't get super smooth "update" transitions for message center
      notifications upon replacing a notification.
  (2) We continue to rely on the delegates associated with Notification
      objects, which in themselves are incompatible with notifications
      that can outlive the process they were created by.
  (3) It's not possible for the //content layer, that has knowledge of the
      notifications that *should* be showing, to close them when certain
      events occur (for instance, the Service Worker unregistering). That
      leads to unresponsive notifications.

This patch makes the NotificationIdGenerator responsible for creating the
notification Ids, giving us a clear path towards solving these issues. In
addition, it further isolates the concept of persistent notification Ids,
reducing the number of Ids readers of this code have to keep in mind.

The NotificationDatabase's keying scheme has been changed to refer to the
generated Id as opposed to the ever-incrementing persistent notification
ID. This avoids needing separate association keys and removes a DELETE
operation prior to writing data for a potentially replaced notification.

BUG=485985, 439950
==========

to

==========
Generate Web Notification Ids in the //content layer

Today, Web Notifications get a random Id assigned to them by the delegate
used with the message_center::Notification object. This has a number of
implications:

  (1) We don't get super smooth "update" transitions for message center
      notifications upon replacing a notification.
  (2) We continue to rely on the delegates associated with Notification
      objects, which in themselves are incompatible with notifications
      that can outlive the process they were created by.
  (3) It's not possible for the //content layer, that has knowledge of the
      notifications that *should* be showing, to close them when certain
      events occur (for instance, the Service Worker unregistering). That
      leads to unresponsive notifications.

This patch makes the NotificationIdGenerator responsible for creating the
notification Ids, giving us a clear path towards solving these issues. In
addition, it further isolates the concept of persistent notification Ids,
reducing the number of Ids readers of this code have to keep in mind.

The NotificationDatabase's keying scheme has been changed to refer to the
generated Id as opposed to the ever-incrementing persistent notification
ID. This avoids needing separate association keys and removes a DELETE
operation prior to writing data for a potentially replaced notification.

TBR=mkwst (type change in /web/)
BUG=485985, 439950
==========

[Peter Beverloo, 2016-09-13 12:29:59]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2016-09-13 12:29:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
miguelg@chromium.org, johnme@chromium.org, piman@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2300093002/#ps140001
(title: "comments")

[commit-bot: I haz the power, 2016-09-13 12:30:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mike West, 2016-09-13 12:30:31]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2016-09-13 12:30:32]

type change in //Source/web LGTM.

[commit-bot: I haz the power, 2016-09-13 14:36:17]

Description was changed from

==========
Generate Web Notification Ids in the //content layer

Today, Web Notifications get a random Id assigned to them by the delegate
used with the message_center::Notification object. This has a number of
implications:

  (1) We don't get super smooth "update" transitions for message center
      notifications upon replacing a notification.
  (2) We continue to rely on the delegates associated with Notification
      objects, which in themselves are incompatible with notifications
      that can outlive the process they were created by.
  (3) It's not possible for the //content layer, that has knowledge of the
      notifications that *should* be showing, to close them when certain
      events occur (for instance, the Service Worker unregistering). That
      leads to unresponsive notifications.

This patch makes the NotificationIdGenerator responsible for creating the
notification Ids, giving us a clear path towards solving these issues. In
addition, it further isolates the concept of persistent notification Ids,
reducing the number of Ids readers of this code have to keep in mind.

The NotificationDatabase's keying scheme has been changed to refer to the
generated Id as opposed to the ever-incrementing persistent notification
ID. This avoids needing separate association keys and removes a DELETE
operation prior to writing data for a potentially replaced notification.

TBR=mkwst (type change in /web/)
BUG=485985, 439950
==========

to

==========
Generate Web Notification Ids in the //content layer

Today, Web Notifications get a random Id assigned to them by the delegate
used with the message_center::Notification object. This has a number of
implications:

  (1) We don't get super smooth "update" transitions for message center
      notifications upon replacing a notification.
  (2) We continue to rely on the delegates associated with Notification
      objects, which in themselves are incompatible with notifications
      that can outlive the process they were created by.
  (3) It's not possible for the //content layer, that has knowledge of the
      notifications that *should* be showing, to close them when certain
      events occur (for instance, the Service Worker unregistering). That
      leads to unresponsive notifications.

This patch makes the NotificationIdGenerator responsible for creating the
notification Ids, giving us a clear path towards solving these issues. In
addition, it further isolates the concept of persistent notification Ids,
reducing the number of Ids readers of this code have to keep in mind.

The NotificationDatabase's keying scheme has been changed to refer to the
generated Id as opposed to the ever-incrementing persistent notification
ID. This avoids needing separate association keys and removes a DELETE
operation prior to writing data for a potentially replaced notification.

TBR=mkwst (type change in /web/)
BUG=485985, 439950
==========

[commit-bot: I haz the power, 2016-09-13 14:36:19]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-09-13 14:39:00]

Description was changed from

==========
Generate Web Notification Ids in the //content layer

Today, Web Notifications get a random Id assigned to them by the delegate
used with the message_center::Notification object. This has a number of
implications:

  (1) We don't get super smooth "update" transitions for message center
      notifications upon replacing a notification.
  (2) We continue to rely on the delegates associated with Notification
      objects, which in themselves are incompatible with notifications
      that can outlive the process they were created by.
  (3) It's not possible for the //content layer, that has knowledge of the
      notifications that *should* be showing, to close them when certain
      events occur (for instance, the Service Worker unregistering). That
      leads to unresponsive notifications.

This patch makes the NotificationIdGenerator responsible for creating the
notification Ids, giving us a clear path towards solving these issues. In
addition, it further isolates the concept of persistent notification Ids,
reducing the number of Ids readers of this code have to keep in mind.

The NotificationDatabase's keying scheme has been changed to refer to the
generated Id as opposed to the ever-incrementing persistent notification
ID. This avoids needing separate association keys and removes a DELETE
operation prior to writing data for a potentially replaced notification.

TBR=mkwst (type change in /web/)
BUG=485985, 439950
==========

to

==========
Generate Web Notification Ids in the //content layer

Today, Web Notifications get a random Id assigned to them by the delegate
used with the message_center::Notification object. This has a number of
implications:

  (1) We don't get super smooth "update" transitions for message center
      notifications upon replacing a notification.
  (2) We continue to rely on the delegates associated with Notification
      objects, which in themselves are incompatible with notifications
      that can outlive the process they were created by.
  (3) It's not possible for the //content layer, that has knowledge of the
      notifications that *should* be showing, to close them when certain
      events occur (for instance, the Service Worker unregistering). That
      leads to unresponsive notifications.

This patch makes the NotificationIdGenerator responsible for creating the
notification Ids, giving us a clear path towards solving these issues. In
addition, it further isolates the concept of persistent notification Ids,
reducing the number of Ids readers of this code have to keep in mind.

The NotificationDatabase's keying scheme has been changed to refer to the
generated Id as opposed to the ever-incrementing persistent notification
ID. This avoids needing separate association keys and removes a DELETE
operation prior to writing data for a potentially replaced notification.

TBR=mkwst (type change in /web/)
BUG=485985, 439950

Committed: https://crrev.com/c45944c33233a04614dca6eeed41f75e75a943be
Cr-Commit-Position: refs/heads/master@{#418241}
==========

[commit-bot: I haz the power, 2016-09-13 14:39:04]

Patchset 5 (id:??) landed as
https://crrev.com/c45944c33233a04614dca6eeed41f75e75a943be
Cr-Commit-Position: refs/heads/master@{#418241}