Integrate the notification database with the normal code path.

Integrate the notification database with the normal code path.

This patch actually integrates the Web Notification database with the
existing code paths, which means that it will be used for any persistent
notification being shown from this patch forward.

The database is completely tested with a series of unit tests, whereas
all existing browser tests, layout tests and instrumentation tests will
continue to exercise the full flows.

Please mind that this is the first part in a series. I realize that
there's a fair amount of TODOs in the code. Resolving these takes larger
refactorings (in case of reducing PlatformNotificationService
knowledge) or three-sided patches (in case of the Blink API). These
will be addressed in follow-ups.

Design document:
  http://goo.gl/TciXVp

BUG=447628
Committed: https://crrev.com/2ee1f3c08f9d1e46bdeb42dc8435dd30d81f0ae6
Cr-Commit-Position: refs/heads/master@{#325232}

[Peter Beverloo, 2015-03-31 19:09:55]

Patchset #2 (id:20001) has been deleted

[Peter Beverloo, 2015-03-31 19:10:02]

Patchset #1 (id:1) has been deleted

[Peter Beverloo, 2015-03-31 19:15:53]

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

[Peter Beverloo, 2015-03-31 19:15:53]

+dewittj, johnme

Hi guys - I'd very much appreciate a review of this code. There's a lot of
TODOs, but I hope you trust me to actually follow up on those.

Two things to note:

- The Android implementation is still pending. I'm probably going to end up
using base::Int64ToString for now, but will work this out tomorrow.

- In parallel, I'm working on moving the browser tests to //content in
https://codereview.chromium.org/1050623003. That should land close to this.

[Peter Beverloo, 2015-03-31 19:25:42]

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

[Peter Beverloo, 2015-03-31 19:25:42]

Actually, I'd expect the //content API changes to be stable for this patch - let
me request the stamps for those.

+avi for content/
+mkwst for IPC and layout tests

[dewittj, 2015-03-31 19:46:40]

FYI I'm out of office till Monday.

Justin

On Tue, Mar 31, 2015, 12:25  <peter@chromium.org> wrote:

> Actually, I'd expect the //content API changes to be stable for this patch
> - let
> me request the stamps for those.
>
> +avi for content/
> +mkwst for IPC and layout tests
>
> https://codereview.chromium.org/1026853002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Avi (use Gerrit), 2015-03-31 20:36:21]

You have my content LGTM stamp; get a review from an area expert.

https://codereview.chromium.org/1026853002/diff/80001/chrome/browser/notifica...
File chrome/browser/notifications/persistent_notification_delegate.h (right):

https://codereview.chromium.org/1026853002/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/persistent_notification_delegate.h:8: #include
<stdint.h>
Thank you!

https://codereview.chromium.org/1026853002/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/persistent_notification_delegate.h:28: int64
persistent_notification_id() const {
int64_t

[Mike West, 2015-04-01 10:02:53]

IPC LGTM.

[johnme, 2015-04-01 17:39:37]

https://codereview.chromium.org/1026853002/diff/80001/chrome/browser/notifica...
File chrome/browser/notifications/persistent_notification_delegate.h (right):

https://codereview.chromium.org/1026853002/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/persistent_notification_delegate.h:28: int64
persistent_notification_id() const {
What's the relation between this and id()? Could id() just be
base::Int64ToString(persistent_notification_id_) ? If you can't unify these,
please clearly document their different purposes.

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

https://codereview.chromium.org/1026853002/diff/80001/content/public/browser/...
content/public/browser/platform_notification_service.h:65: int64
service_worker_registration_id,
s/int64 service_worker_registration_id/int64_t persistent_notification_id/

[Peter Beverloo, 2015-04-01 18:32:17]

https://codereview.chromium.org/1026853002/diff/80001/chrome/browser/notifica...
File chrome/browser/notifications/persistent_notification_delegate.h (right):

https://codereview.chromium.org/1026853002/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/persistent_notification_delegate.h:28: int64
persistent_notification_id() const {
On 2015/03/31 20:36:21, Avi wrote:
> int64_t

Done.

https://codereview.chromium.org/1026853002/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/persistent_notification_delegate.h:28: int64
persistent_notification_id() const {
On 2015/04/01 17:39:37, johnme wrote:
> What's the relation between this and id()? Could id() just be
> base::Int64ToString(persistent_notification_id_) ? If you can't unify these,
> please clearly document their different purposes.

id() is what we tell the NotificationUIManager (on desktop the Message Center),
p_n_id() is the reference for the NotificationDatabase. Theoretically we can
unify them, but I'd rather not touch that area in this patch. Added a comment.

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

https://codereview.chromium.org/1026853002/diff/80001/content/public/browser/...
content/public/browser/platform_notification_service.h:65: int64
service_worker_registration_id,
On 2015/04/01 17:39:37, johnme wrote:
> s/int64 service_worker_registration_id/int64_t persistent_notification_id/

Done.

[johnme, 2015-04-02 17:21:07]

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

https://codereview.chromium.org/1026853002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_impl.cc:253:
DLOG(ERROR) << "Implement Android behavior";
Did this use to work before? Would it be hard to keep it working?

https://codereview.chromium.org/1026853002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/platform_notification_service_impl.h (right):

https://codereview.chromium.org/1026853002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_impl.h:120:
std::map<int64_t, std::string> persistent_notifications_;
Please add to comment: Must only be used on UI thread.

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
File content/browser/notifications/notification_message_filter.cc (left):

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.cc:62: message.type()
== PlatformNotificationHostMsg_ShowPersistent::ID ||
It's a little hard to read the threading guarantees of this class. It'd probably
be helpful to do one of the following:
- Add comments in the header explaining which methods are run on what thread.
- Rename methods to indicate the thread they run on.
- Add a class-level comment explaining that methods run on both the IO and UI
threads, and readers should refer to the DCHECK_CURRENTLY_ONs.

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

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.cc:36: void
NotificationMessageFilter::DidCloseNotification(int notification_id) {
Please DCHECK_CURRENTLY_ON(BrowserThread::UI)

Also, what's the lifetime of the PageNotificationDelegate which calls this? Does
anything prevent the NotificationMessageFilter being destroyed before or during
this call?

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.cc:126:
notification_context_->WriteNotificationData(
Nit: It seems it might be cleaner for
PlatformNotificationService::DisplayPersistentNotification to be reponsible for
writing the data to the DB? Especially since you're passing the NotificationData
there anyway.

Similarly for ClosePersistentNotification.

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.cc:229: void
NotificationMessageFilter::DidDeletePersistentNotificationData(
Nit: Please DCHECK_CURRENTLY_ON(BrowserThread::IO)

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.cc:231: //
TODO(peter): Consider feeding back to the renderer that the notification
Add a TODO for UMA logging?

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.cc:250: bool
NotificationMessageFilter::VerifyNotificationPermissionGranted(
Nit: please add DCHECK_CURRENTLY_ON(BrowserThread::UI)

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

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.h:27: class
CONTENT_EXPORT NotificationMessageFilter : public BrowserMessageFilter {
Why does this need CONTENT_EXPORT?

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.h:48: friend class
NotificationBrowserTest;
Please add this in https://codereview.chromium.org/1050623003 instead of this
patch.

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.h:82: int64_t
notification_id);
s/notification_id/persistent_notification_id/ !!

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.h:111:
base::WeakPtrFactory<NotificationMessageFilter> weak_factory_;
If you're handing out IO thread weak ptrs, you need to ensure their factory is
deleted on the IO thread. Add an OnDestruct method like
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/pu...

Also nit: please rename to weak_factory_io_to_io_ or similar, to clarify proper
usage.

https://codereview.chromium.org/1026853002/diff/100001/content/child/notifica...
File content/child/notifications/notification_manager.cc (right):

https://codereview.chromium.org/1026853002/diff/100001/content/child/notifica...
content/child/notifications/notification_manager.cc:177: DLOG(ERROR) << "Unable
to close persistent notification; invalid id: "
Nit: NOTREACHED() << ...?

[Peter Beverloo, 2015-04-07 17:46:11]

Thank you for the thorough review, John!

The Android part is now included. Please take another look at that :-).

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

https://codereview.chromium.org/1026853002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_impl.cc:253:
DLOG(ERROR) << "Implement Android behavior";
On 2015/04/02 17:21:06, johnme wrote:
> Did this use to work before? Would it be hard to keep it working?

This is working now.

https://codereview.chromium.org/1026853002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/platform_notification_service_impl.h (right):

https://codereview.chromium.org/1026853002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_impl.h:120:
std::map<int64_t, std::string> persistent_notifications_;
On 2015/04/02 17:21:06, johnme wrote:
> Please add to comment: Must only be used on UI thread.

Done.

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
File content/browser/notifications/notification_message_filter.cc (left):

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.cc:62: message.type()
== PlatformNotificationHostMsg_ShowPersistent::ID ||
On 2015/04/02 17:21:07, johnme wrote:
> It's a little hard to read the threading guarantees of this class. It'd
probably
> be helpful to do one of the following:
> - Add comments in the header explaining which methods are run on what thread.
> - Rename methods to indicate the thread they run on.
> - Add a class-level comment explaining that methods run on both the IO and UI
> threads, and readers should refer to the DCHECK_CURRENTLY_ONs.

Could we consider this out of scope for this CL please? I plan to switch
notifications to use Mojo in M44 timeframe in either case.

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

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.cc:36: void
NotificationMessageFilter::DidCloseNotification(int notification_id) {
On 2015/04/02 17:21:07, johnme wrote:
> Please DCHECK_CURRENTLY_ON(BrowserThread::UI)

Done

On 2015/04/02 17:21:07, johnme wrote:
> Also, what's the lifetime of the PageNotificationDelegate which calls this?
Does
> anything prevent the NotificationMessageFilter being destroyed before or
during
> this call?

It gets the RPH based on the process_id. The RPH::FromID() method ensures that
it's only called on the UI thread, so I would expect this to be fine. I have not
seen any crashes in this regard.

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.cc:126:
notification_context_->WriteNotificationData(
On 2015/04/02 17:21:06, johnme wrote:
> Nit: It seems it might be cleaner for
> PlatformNotificationService::DisplayPersistentNotification to be reponsible
for
> writing the data to the DB? Especially since you're passing the
NotificationData
> there anyway.
> 
> Similarly for ClosePersistentNotification.

No. The Service will become dumber - it will only display or close them. This is
the first patch in a series, the following ones will change the API of PNS to
take a message_center::Notification object rather than the raw
PlatformNotificationData. However, that's a refactoring that's too big to
include in this patch.

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.cc:229: void
NotificationMessageFilter::DidDeletePersistentNotificationData(
On 2015/04/02 17:21:07, johnme wrote:
> Nit: Please DCHECK_CURRENTLY_ON(BrowserThread::IO)

Done.

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.cc:231: //
TODO(peter): Consider feeding back to the renderer that the notification
On 2015/04/02 17:21:07, johnme wrote:
> Add a TODO for UMA logging?

We already log UMA as part of the PlatformNotificationContext. There's no need
to log more here.

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.cc:250: bool
NotificationMessageFilter::VerifyNotificationPermissionGranted(
On 2015/04/02 17:21:07, johnme wrote:
> Nit: please add DCHECK_CURRENTLY_ON(BrowserThread::UI)

Done.

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

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.h:27: class
CONTENT_EXPORT NotificationMessageFilter : public BrowserMessageFilter {
On 2015/04/02 17:21:07, johnme wrote:
> Why does this need CONTENT_EXPORT?

For the NotificationBrowserTest. Removed per the next comment.

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.h:48: friend class
NotificationBrowserTest;
On 2015/04/02 17:21:07, johnme wrote:
> Please add this in https://codereview.chromium.org/1050623003 instead of this
> patch.

Done.

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.h:82: int64_t
notification_id);
On 2015/04/02 17:21:07, johnme wrote:
> s/notification_id/persistent_notification_id/ !!

Done.

https://codereview.chromium.org/1026853002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_message_filter.h:111:
base::WeakPtrFactory<NotificationMessageFilter> weak_factory_;
On 2015/04/02 17:21:07, johnme wrote:
> If you're handing out IO thread weak ptrs, you need to ensure their factory is
> deleted on the IO thread. Add an OnDestruct method like
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/pu...
> 
> Also nit: please rename to weak_factory_io_to_io_ or similar, to clarify
proper
> usage.

Added the OnDestruct method. Renamed the member to weak_factory_io_.

https://codereview.chromium.org/1026853002/diff/100001/content/child/notifica...
File content/child/notifications/notification_manager.cc (right):

https://codereview.chromium.org/1026853002/diff/100001/content/child/notifica...
content/child/notifications/notification_manager.cc:177: DLOG(ERROR) << "Unable
to close persistent notification; invalid id: "
On 2015/04/02 17:21:07, johnme wrote:
> Nit: NOTREACHED() << ...?

Done.

[johnme, 2015-04-08 17:33:32]

https://codereview.chromium.org/1026853002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java
(right):

https://codereview.chromium.org/1026853002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java:22:
public static final String EXTRA_NOTIFICATION_TAG = "notification_tag";
The comment seems inaccurate - from reading the code, it seems like
EXTRA_NOTIFICATION_TAG gets used in two cases:
- set by the system, in which case its value was constructed by makePlatformTag
- set by getPendingIntent, in which case its value is the actual tag provided by
the webapp

I'd recommend using a different intent key name in getPendingIntent, given that
these are semantically different datatypes.

https://codereview.chromium.org/1026853002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java:25:
* Intent extras psased to Chrome for intents created by the
NotificationUIManager.
passed

https://codereview.chromium.org/1026853002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java
(right):

https://codereview.chromium.org/1026853002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:55:
if (!intent.hasExtra(NotificationConstants.EXTRA_PERSISTENT_NOTIFICATION_ID))
return;
Shouldn't you also be checking for the origin? If not, it seems that
dispatchNotificationEvent could pass a null origin to
onNotificationClicked/Closed (and unlike tag, that parameter isn't marked as
"May be NULL").

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
File chrome/browser/notifications/notification_ui_manager_android.cc (right):

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.cc:12: #include
"base/pickle.h"
Unused now.

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.cc:77: std::string
tag = ConvertJavaStringToUTF8(env, java_tag);
Is java_tag definitely non-null here? ConvertJavaStringToUTF8 logs a warning if
it's null.

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.cc:101: //
TODO(peter): Implement handling when a notification has been closed. The
Do you have a follow-up patch for this? Ideally we wouldn't leave it in the
interim state for too long (as trunk users will start building up stale
notifications in the DB).

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.cc:124: GURL
origin_url(notification.origin_url().GetOrigin());
Micro-nit: move this after `int64_t persistent_notification_id`?

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.cc:275: if
(!base::StringToInt64(notification_id, &persistent_notification_id))
How does this work? Isn't notification_id here a delegate_id, i.e. just a random
GUID? I see that you hacked the CancelById call in
PlatformNotificationServiceImpl::ClosePersistentNotification to be
platform-specific, but what about other methods like CancelAll?

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
File chrome/browser/notifications/notification_ui_manager_android.h (right):

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.h:79:
std::map<std::string, ProfileNotification*> profile_notifications_;
Could you maybe get rid of this now? It's not yet unused, but the remaining uses
seem vestigial?

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_impl.cc:244:
GetNotificationUIManager()->CancelById(
Eww eww eww. CancelById takes a delegate_id (random GUID) as a parameter, but
you're passing a stringified persistent notification id (database int64).

It seems it'd be cleaner to add a separate
CancelPersistentNotificationById(int64_t p_n_i) method instead? (and preferably
use it on all platforms, not just Android?)

[Peter Beverloo, 2015-04-08 18:57:48]

https://codereview.chromium.org/1026853002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java
(right):

https://codereview.chromium.org/1026853002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java:22:
public static final String EXTRA_NOTIFICATION_TAG = "notification_tag";
On 2015/04/08 17:33:32, johnme wrote:
> The comment seems inaccurate - from reading the code, it seems like
> EXTRA_NOTIFICATION_TAG gets used in two cases:
> - set by the system, in which case its value was constructed by
makePlatformTag
> - set by getPendingIntent, in which case its value is the actual tag provided
by
> the webapp
> 
> I'd recommend using a different intent key name in getPendingIntent, given
that
> these are semantically different datatypes.

I've updated the comment. They will contain exactly the same value, so I'm all
for reusing this.

https://codereview.chromium.org/1026853002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java:25:
* Intent extras psased to Chrome for intents created by the
NotificationUIManager.
On 2015/04/08 17:33:32, johnme wrote:
> passed

Done.

https://codereview.chromium.org/1026853002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java
(right):

https://codereview.chromium.org/1026853002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java:55:
if (!intent.hasExtra(NotificationConstants.EXTRA_PERSISTENT_NOTIFICATION_ID))
return;
On 2015/04/08 17:33:32, johnme wrote:
> Shouldn't you also be checking for the origin? If not, it seems that
> dispatchNotificationEvent could pass a null origin to
> onNotificationClicked/Closed (and unlike tag, that parameter isn't marked as
> "May be NULL").

Checking for all three now.

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
File chrome/browser/notifications/notification_ui_manager_android.cc (right):

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.cc:12: #include
"base/pickle.h"
On 2015/04/08 17:33:32, johnme wrote:
> Unused now.

Done.

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.cc:77: std::string
tag = ConvertJavaStringToUTF8(env, java_tag);
On 2015/04/08 17:33:32, johnme wrote:
> Is java_tag definitely non-null here? ConvertJavaStringToUTF8 logs a warning
if
> it's null.

Yes.

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.cc:101: //
TODO(peter): Implement handling when a notification has been closed. The
On 2015/04/08 17:33:32, johnme wrote:
> Do you have a follow-up patch for this? Ideally we wouldn't leave it in the
> interim state for too long (as trunk users will start building up stale
> notifications in the DB).

Not immediately.

I already have a series of patches depending on this one - I'll have one for
this tomorrow...

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.cc:124: GURL
origin_url(notification.origin_url().GetOrigin());
On 2015/04/08 17:33:32, johnme wrote:
> Micro-nit: move this after `int64_t persistent_notification_id`?

Done.

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.cc:275: if
(!base::StringToInt64(notification_id, &persistent_notification_id))
On 2015/04/08 17:33:32, johnme wrote:
> How does this work? Isn't notification_id here a delegate_id, i.e. just a
random
> GUID? I see that you hacked the CancelById call in
> PlatformNotificationServiceImpl::ClosePersistentNotification to be
> platform-specific, but what about other methods like CancelAll?

This was broken before this patch as well :(. As said in another comment, I'll
pull out all the non-persistent notification stuff in a follow up patch.

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
File chrome/browser/notifications/notification_ui_manager_android.h (right):

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.h:79:
std::map<std::string, ProfileNotification*> profile_notifications_;
On 2015/04/08 17:33:32, johnme wrote:
> Could you maybe get rid of this now? It's not yet unused, but the remaining
uses
> seem vestigial?

We still have some boilerplate code supporting non-persistent notifications. I'd
prefer to do that clean-up in another patch.

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_impl.cc:244:
GetNotificationUIManager()->CancelById(
On 2015/04/08 17:33:32, johnme wrote:
> Eww eww eww. CancelById takes a delegate_id (random GUID) as a parameter, but
> you're passing a stringified persistent notification id (database int64).
> 
> It seems it'd be cleaner to add a separate
> CancelPersistentNotificationById(int64_t p_n_i) method instead? (and
preferably
> use it on all platforms, not just Android?)

This is a strictly temporary measure which will go away when //content/
generates the delegate Ids. Introducing another method on the
NotificationUIManager interface doesn't seem worth it if this will go away Real
Soon now.

[johnme, 2015-04-09 10:06:11]

notifications lgtm once you use a different intent extras key for non-platform
tag. I look forward to the follow-up patches removing the dead code.

https://codereview.chromium.org/1026853002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java
(right):

https://codereview.chromium.org/1026853002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java:22:
public static final String EXTRA_NOTIFICATION_TAG = "notification_tag";
On 2015/04/08 18:57:47, Peter Beverloo wrote:
> On 2015/04/08 17:33:32, johnme wrote:
> > The comment seems inaccurate - from reading the code, it seems like
> > EXTRA_NOTIFICATION_TAG gets used in two cases:
> > - set by the system, in which case its value was constructed by
> makePlatformTag
> > - set by getPendingIntent, in which case its value is the actual tag
provided
> by
> > the webapp
> > 
> > I'd recommend using a different intent key name in getPendingIntent, given
> that
> > these are semantically different datatypes.
> 
> I've updated the comment. They will contain exactly the same value, so I'm all
> for reusing this.

They don't contain the same value. Look at
NotificationUIManager.displayNotification: it passes the raw tag (from JS) to
getPendingIntent, but passes the output of makePlatformTag to
mNotificationManager.notify. And you only call getOriginFromTag from
launchNotificationPreferences.

Please use a different extras key for non-platform tag (or always use platform
tags in extras, if you'd prefer).

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
File chrome/browser/notifications/notification_ui_manager_android.cc (right):

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.cc:275: if
(!base::StringToInt64(notification_id, &persistent_notification_id))
On 2015/04/08 18:57:47, Peter Beverloo wrote:
> On 2015/04/08 17:33:32, johnme wrote:
> > How does this work? Isn't notification_id here a delegate_id, i.e. just a
> random
> > GUID? I see that you hacked the CancelById call in
> > PlatformNotificationServiceImpl::ClosePersistentNotification to be
> > platform-specific, but what about other methods like CancelAll?
> 
> This was broken before this patch as well :(. As said in another comment, I'll
> pull out all the non-persistent notification stuff in a follow up patch.

Hmm, so this patch breaks PlatformNotificationServiceImpl's CancelNotification
method; but I guess that's only used for non-persistent notifications, which
shouldn't happen anyway. All this dead code makes reviews hard. Fine. I added a
task to the spreadsheet for pulling out the non-persistent notification code.

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/1026853002/diff/120001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_impl.cc:244:
GetNotificationUIManager()->CancelById(
On 2015/04/08 18:57:48, Peter Beverloo wrote:
> On 2015/04/08 17:33:32, johnme wrote:
> > Eww eww eww. CancelById takes a delegate_id (random GUID) as a parameter,
but
> > you're passing a stringified persistent notification id (database int64).
> > 
> > It seems it'd be cleaner to add a separate
> > CancelPersistentNotificationById(int64_t p_n_i) method instead? (and
> preferably
> > use it on all platforms, not just Android?)
> 
> This is a strictly temporary measure which will go away when //content/
> generates the delegate Ids. Introducing another method on the
> NotificationUIManager interface doesn't seem worth it if this will go away
Real
> Soon now.

Fine. Added a task to the spreadsheet.

[Peter Beverloo, 2015-04-09 13:09:00]

https://codereview.chromium.org/1026853002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java
(right):

https://codereview.chromium.org/1026853002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java:22:
public static final String EXTRA_NOTIFICATION_TAG = "notification_tag";
On 2015/04/09 10:06:11, johnme wrote:
> On 2015/04/08 18:57:47, Peter Beverloo wrote:
> > On 2015/04/08 17:33:32, johnme wrote:
> > > The comment seems inaccurate - from reading the code, it seems like
> > > EXTRA_NOTIFICATION_TAG gets used in two cases:
> > > - set by the system, in which case its value was constructed by
> > makePlatformTag
> > > - set by getPendingIntent, in which case its value is the actual tag
> provided
> > by
> > > the webapp
> > > 
> > > I'd recommend using a different intent key name in getPendingIntent, given
> > that
> > > these are semantically different datatypes.
> > 
> > I've updated the comment. They will contain exactly the same value, so I'm
all
> > for reusing this.
> 
> They don't contain the same value. Look at
> NotificationUIManager.displayNotification: it passes the raw tag (from JS) to
> getPendingIntent, but passes the output of makePlatformTag to
> mNotificationManager.notify. And you only call getOriginFromTag from
> launchNotificationPreferences.
> 
> Please use a different extras key for non-platform tag (or always use platform
> tags in extras, if you'd prefer).

Done.

[dewittj, 2015-04-10 17:02:19]

This patch seems okay to me with the following caveats:

* It looks like a large portion of this code is actually cleanup/refactor (e.g.
the string->int64 for persistent notification IDs) rather than the stated goal
of integrating the database.  Would the patches become more focused if cleanup
was landed before feature work?
* I am also concerned about zombie notifications from users on trunk/dev that
may receive a Chrome that never deletes from the DB.  Is there a global GC
planned for the database?  (this may be necessary anyway to remain clean in the
face of crashes etc.)
* The semantics of regenerated notifications are unclear, especially how it is
correct across restarts of chrome.  Is it worth separating that logic out into
its own class?

https://codereview.chromium.org/1026853002/diff/200001/chrome/browser/notific...
File chrome/browser/notifications/notification_ui_manager_android.cc (right):

https://codereview.chromium.org/1026853002/diff/200001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.cc:122:
static_cast<PersistentNotificationDelegate*>(notification.delegate());
This could be dangerous in the face of Chrome-internal code creating
non-persistent notifications.  Have you investigated whether there are any ::Add
calls elsewhere in the codebase that might violate this undocumented restriction
that on android all delegates must be PersistentNotificationDelegates?  Should
we DCHECK in other delegate constructors if used on Android?

https://codereview.chromium.org/1026853002/diff/200001/chrome/browser/notific...
File chrome/browser/notifications/notification_ui_manager_android.h (right):

https://codereview.chromium.org/1026853002/diff/200001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.h:81: // Pair
containing a regenerated notification's origin and tag.
Can you add comments around what a "regenerated notification" is and what its
use is?

[Peter Beverloo, 2015-04-13 16:22:32]

Thanks for your review! Please take a look at my answers and update.

> * It looks like a large portion of this code is actually cleanup/refactor
(e.g.
> the string->int64 for persistent notification IDs) rather than the stated goal
> of integrating the database.  Would the patches become more focused if cleanup
> was landed before feature work?

Splitting this up will be difficult as changing the data type relies on how we
work with the data. I've already done a fair amount of ground work before this
patch to make hooking up the database as simple as we can.

> * I am also concerned about zombie notifications from users on trunk/dev that
> may receive a Chrome that never deletes from the DB.  Is there a global GC
> planned for the database?  (this may be necessary anyway to remain clean in
the
> face of crashes etc.)

For Android, we're going to have zombie notifications. Until the framework
allows us to see what notifications the app is showing, enabling Chrome to
synchronize, we're going to try to be as accurate as we can. (Tracked in
b/17320461.) Getting this right may take a release or two given the device
landscape.

For desktop, we can do such synchronization on browser start-up because the
information provided by the message center will be reliable. We know exactly
what's showing and can diff against that.

> * The semantics of regenerated notifications are unclear, especially how it is
> correct across restarts of chrome.  Is it worth separating that logic out into
> its own class?

I clarified that in the comment, for now. I'm confident we can get rid of this
logic altogether when moving to message_center::Notification objects created by
//content.

https://codereview.chromium.org/1026853002/diff/200001/chrome/browser/notific...
File chrome/browser/notifications/notification_ui_manager_android.cc (right):

https://codereview.chromium.org/1026853002/diff/200001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.cc:122:
static_cast<PersistentNotificationDelegate*>(notification.delegate());
On 2015/04/10 17:02:19, dewittj wrote:
> This could be dangerous in the face of Chrome-internal code creating
> non-persistent notifications.  Have you investigated whether there are any
::Add
> calls elsewhere in the codebase that might violate this undocumented
restriction
> that on android all delegates must be PersistentNotificationDelegates?  Should
> we DCHECK in other delegate constructors if used on Android?

There are no other users right now.

When //content creates the Notification objects, we don't need to do this
anymore since we can rely on the delegate_id instead to provide this uniqueness
instead (or still a GUID in case for non-persistent notifications).

https://codereview.chromium.org/1026853002/diff/200001/chrome/browser/notific...
File chrome/browser/notifications/notification_ui_manager_android.h (right):

https://codereview.chromium.org/1026853002/diff/200001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.h:81: // Pair
containing a regenerated notification's origin and tag.
On 2015/04/10 17:02:19, dewittj wrote:
> Can you add comments around what a "regenerated notification" is and what its
> use is?

Done.

[dewittj, 2015-04-13 18:58:53]

lgtm with a few more documentation nits

https://codereview.chromium.org/1026853002/diff/220001/chrome/browser/notific...
File chrome/browser/notifications/notification_ui_manager_android.h (right):

https://codereview.chromium.org/1026853002/diff/220001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.h:52: void
CancelAll() override;
please document that these functions will only operate on notifications that
were opened or clicked since the last restart of Chrome.

https://codereview.chromium.org/1026853002/diff/220001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.h:81: // Pair
containing the information necessary in order to enable working with
nit: this is only used for closing notifications (AFAICT), maybe this comment
should be more specific, like:

// ... to enable closing notifications that were not created ... We track
notifications on a best-effort basis, and may not have all notifications in this
list if they have not been interacted with since the last restart of Chrome.

[Peter Beverloo, 2015-04-14 14:09:29]

https://codereview.chromium.org/1026853002/diff/220001/chrome/browser/notific...
File chrome/browser/notifications/notification_ui_manager_android.h (right):

https://codereview.chromium.org/1026853002/diff/220001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.h:52: void
CancelAll() override;
On 2015/04/13 18:58:53, dewittj wrote:
> please document that these functions will only operate on notifications that
> were opened or clicked since the last restart of Chrome.

Done. Added it to the class-level comment.

https://codereview.chromium.org/1026853002/diff/220001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.h:81: // Pair
containing the information necessary in order to enable working with
On 2015/04/13 18:58:53, dewittj wrote:
> nit: this is only used for closing notifications (AFAICT), maybe this comment
> should be more specific, like:
> 
> // ... to enable closing notifications that were not created ... We track
> notifications on a best-effort basis, and may not have all notifications in
this
> list if they have not been interacted with since the last restart of Chrome.

Done.

[Peter Beverloo, 2015-04-14 17:49:12]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-04-14 17:49:13]

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

[commit-bot: I haz the power, 2015-04-14 17:50:11]

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

[commit-bot: I haz the power, 2015-04-14 19:03:23]

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

[commit-bot: I haz the power, 2015-04-14 19:03:25]

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

[Peter Beverloo, 2015-04-15 11:36:37]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-04-15 11:36:38]

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

[commit-bot: I haz the power, 2015-04-15 11:36:44]

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

[commit-bot: I haz the power, 2015-04-15 13:09:24]

Committed patchset #13 (id:280001)

[commit-bot: I haz the power, 2015-04-15 13:10:31]

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