Beginnings of synchronizing notifications in the notification database.

Beginnings of synchronizing notifications in the notification database.

For both desktop Chrome and Android we need to support a mechanism for
synchronizing open notifications. For desktop the most important case
is where Chrome just started up, where there won't be any notifications
since the message center is owned by Chrome.

For Android, there are more cases, since the platform hosts displaying
the notifications and informs Chrome through Intents of mutations. This
patch leaves synchronization there mostly out of scope, while still
laying the early groundwork towards supporting this.

BUG=442143
Committed: https://crrev.com/5b494b3b5f13d6a89afead81e3081a298e10b23b
Cr-Commit-Position: refs/heads/master@{#330583}

[Peter Beverloo, 2015-05-14 14:27:14]

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

[Peter Beverloo, 2015-05-14 14:27:16]

+johnme for review

[johnme, 2015-05-14 15:22:33]

https://codereview.chromium.org/1127013008/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/1127013008/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:240: for
(const auto& iter : profile_notifications_) {
s/iter/entry/ or 'pair' or something, ditto below.

https://codereview.chromium.org/1127013008/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:253: // The
profile pointer can be weak, the instance may have been destroyed, so
Eww gross. Since this is a new method, can we just DCHECK(profile)?

https://codereview.chromium.org/1127013008/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/1127013008/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:317: //
TODO(peter): Filter for persistent notifications only.
How hard would it be to implement this TODO now? IIRC non-persistent
notifications have GUIDs (with dashes), so anything that base::StringToInt64
succeeds on should be persistent? This would tie in nicely with
displayed_notifications being a std::set<int64_t>*

https://codereview.chromium.org/1127013008/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:326: #endif
Micro-nit: #endif  // !defined(OS_ANDROID)

https://codereview.chromium.org/1127013008/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.h (right):

https://codereview.chromium.org/1127013008/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.h:9: #include
<map>
+<set> & <string>

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

https://codereview.chromium.org/1127013008/diff/20001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:69:
!displayed_notifications.size()) {
Until you implement GetDisplayedPersistentNotifications on Android, it's a
little odd that the contents of the set is always ignored. But I guess the TODO
covers this.

https://codereview.chromium.org/1127013008/diff/20001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:356:
DestroyDatabase();
- Shouldn't this go after `database_.reset(new
NotificationDatabase(GetDatabasePath()));` ?

- You seem to be ignoring the case where it returns false. Though I grant that
it's not obvious how to handle that. At least add a DLOG(ERROR) perhaps?

https://codereview.chromium.org/1127013008/diff/20001/content/public/browser/...
File content/public/browser/platform_notification_service.h (right):

https://codereview.chromium.org/1127013008/diff/20001/content/public/browser/...
content/public/browser/platform_notification_service.h:77: // Writesthe ids of
all currently displaying persistent notifications for the
Writesthe

https://codereview.chromium.org/1127013008/diff/20001/content/public/browser/...
content/public/browser/platform_notification_service.h:79: // platform is able
to provide for such a set.
no for

https://codereview.chromium.org/1127013008/diff/20001/content/public/browser/...
content/public/browser/platform_notification_service.h:82:
std::set<std::string>* displayed_notifications) = 0;
Surely this should be std::set<int64_t>? Or is this intended to become a set of
tags rather than ids, in the style of your not yet landed
https://codereview.chromium.org/1133813002/ ?

[Peter Beverloo, 2015-05-14 16:03:35]

All done! Thanks for the review!

https://codereview.chromium.org/1127013008/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/1127013008/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:240: for
(const auto& iter : profile_notifications_) {
On 2015/05/14 15:22:32, johnme wrote:
> s/iter/entry/ or 'pair' or something, ditto below.

Done.

https://codereview.chromium.org/1127013008/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:253: // The
profile pointer can be weak, the instance may have been destroyed, so
On 2015/05/14 15:22:32, johnme wrote:
> Eww gross. Since this is a new method, can we just DCHECK(profile)?

As discussed offline, this is a common pattern in the API :(. Added a TODO to
update this and GAIBPASO() to use ProfileID.

https://codereview.chromium.org/1127013008/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/1127013008/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:317: //
TODO(peter): Filter for persistent notifications only.
On 2015/05/14 15:22:32, johnme wrote:
> How hard would it be to implement this TODO now? IIRC non-persistent
> notifications have GUIDs (with dashes), so anything that base::StringToInt64
> succeeds on should be persistent? This would tie in nicely with
> displayed_notifications being a std::set<int64_t>*

No, that's not the case, the delegate ids for persistent notifications still are
GUIDs. (See notification_object_proxy.cc.) This is two steps in the future - we
need //content to generate the notification Ids (which is in review separately)
and then to identify persistent notifications so that we can fulfill the TODOs
in PlatformNotificationContextImpl as well.

https://codereview.chromium.org/1127013008/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:326: #endif
On 2015/05/14 15:22:32, johnme wrote:
> Micro-nit: #endif  // !defined(OS_ANDROID)

Done.

https://codereview.chromium.org/1127013008/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.h (right):

https://codereview.chromium.org/1127013008/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.h:9: #include
<map>
On 2015/05/14 15:22:32, johnme wrote:
> +<set> & <string>

Done.

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

https://codereview.chromium.org/1127013008/diff/20001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:69:
!displayed_notifications.size()) {
On 2015/05/14 15:22:32, johnme wrote:
> Until you implement GetDisplayedPersistentNotifications on Android, it's a
> little odd that the contents of the set is always ignored. But I guess the
TODO
> covers this.

Supporting the Android case is covered by
|notification_synchronization_supported| being false. Using the IDs here depends
on deterministic ID generation, which, as I said earlier, is in review now as
well.

https://codereview.chromium.org/1127013008/diff/20001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:356:
DestroyDatabase();
On 2015/05/14 15:22:32, johnme wrote:
> - Shouldn't this go after `database_.reset(new
> NotificationDatabase(GetDatabasePath()));` ?

Done

> - You seem to be ignoring the case where it returns false. Though I grant that
> it's not obvious how to handle that. At least add a DLOG(ERROR) perhaps?

No? We don't have to prune if the platform does not support getting the
displaying notifications, e.g. Android.

https://codereview.chromium.org/1127013008/diff/20001/content/public/browser/...
File content/public/browser/platform_notification_service.h (right):

https://codereview.chromium.org/1127013008/diff/20001/content/public/browser/...
content/public/browser/platform_notification_service.h:77: // Writesthe ids of
all currently displaying persistent notifications for the
On 2015/05/14 15:22:32, johnme wrote:
> Writesthe

Oops. Done.

https://codereview.chromium.org/1127013008/diff/20001/content/public/browser/...
content/public/browser/platform_notification_service.h:79: // platform is able
to provide for such a set.
On 2015/05/14 15:22:32, johnme wrote:
> no for

Done.

https://codereview.chromium.org/1127013008/diff/20001/content/public/browser/...
content/public/browser/platform_notification_service.h:82:
std::set<std::string>* displayed_notifications) = 0;
On 2015/05/14 15:22:32, johnme wrote:
> Surely this should be std::set<int64_t>? Or is this intended to become a set
of
> tags rather than ids, in the style of your not yet landed
> https://codereview.chromium.org/1133813002/ ?

These should be strings, as is visible in my API playground CL here:
  https://codereview.chromium.org/1116693002/

The contents will indeed be the deterministic notification IDs.

[johnme, 2015-05-14 16:40:51]

lgtm with error handling

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

https://codereview.chromium.org/1127013008/diff/20001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:356:
DestroyDatabase();
On 2015/05/14 16:03:35, Peter Beverloo wrote:
> On 2015/05/14 15:22:32, johnme wrote:
> > - You seem to be ignoring the case where it returns false. Though I grant
that
> > it's not obvious how to handle that. At least add a DLOG(ERROR) perhaps?
> 
> No? We don't have to prune if the platform does not support getting the
> displaying notifications, e.g. Android.

I'm talking about when DestroyDatabase returns false. You don't handle that
error currently.

[Peter Beverloo, 2015-05-14 17:45:16]

On 2015/05/14 16:40:51, johnme wrote:
> lgtm with error handling
> 
>
https://codereview.chromium.org/1127013008/diff/20001/content/browser/notific...
> File content/browser/notifications/platform_notification_context_impl.cc
> (right):
> 
>
https://codereview.chromium.org/1127013008/diff/20001/content/browser/notific...
> content/browser/notifications/platform_notification_context_impl.cc:356:
> DestroyDatabase();
> On 2015/05/14 16:03:35, Peter Beverloo wrote:
> > On 2015/05/14 15:22:32, johnme wrote:
> > > - You seem to be ignoring the case where it returns false. Though I grant
> that
> > > it's not obvious how to handle that. At least add a DLOG(ERROR) perhaps?
> > 
> > No? We don't have to prune if the platform does not support getting the
> > displaying notifications, e.g. Android.
> 
> I'm talking about when DestroyDatabase returns false. You don't handle that
> error currently.

Oops, done! We don't have to handle that in this case, because it just means
that we were not able to destroy the files. The next call to Open(), which we
now always do, will capture the state of the database and fall back to the rest
of the error paths.

[Peter Beverloo, 2015-05-14 17:56:22]

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

[Peter Beverloo, 2015-05-14 17:56:23]

+avi for content/

[Avi (use Gerrit), 2015-05-14 19:55:45]

lgtm

[Peter Beverloo, 2015-05-19 17:55:24]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-05-19 17:55:24]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org,
avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1127013008/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-05-19 17:56:25]

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

[commit-bot: I haz the power, 2015-05-19 19:35:26]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-19 19:36:14]

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