Clean up the NotificationUIManagerAndroid.

Clean up the NotificationUIManagerAndroid.

When this class was introduced, it served the needs for non-persistent
Web Notifications (and other kinds of notifications), with the knowledge
that it wouldn't be able to reliably provide many of the API methods
defined in the NotificationUIManager interface. That was OK.

Today, we only ship persistent Web Notifications, which can outlive the
browser process, and don't use the NotificationUIManagerAndroid for any
other purpose. However, the code to support other kinds of notifications
is still more or less in place, with no tests or users. It's broken.

Therefore we remove it. This CL also removes a test that doesn't make
sense anymore, since this stops us from double-closing notifications.

BUG=

Committed: https://crrev.com/4b8f24f0f29941118c12ade3156de721e5c2c496
Cr-Commit-Position: refs/heads/master@{#325273}

[Peter Beverloo, 2015-04-09 15:24:07]

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

[Peter Beverloo, 2015-04-09 15:24:07]

+johnme and dewittj for review

This is the first follow-up to the integration patch.
  https://codereview.chromium.org/1026853002/

[johnme, 2015-04-09 17:00:02]

Woohoo!

https://codereview.chromium.org/1072043003/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java
(left):

https://codereview.chromium.org/1072043003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java:242:
public void testNotificationDeleteIntentClosesNotification() throws Exception {
I don't understand why this test doesn't make sense anymore? What do you mean by
"it stops us from double-closing notifications"?

https://codereview.chromium.org/1072043003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_ui_manager_android.cc (left):

https://codereview.chromium.org/1072043003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:37: //
TODO(peter): Decide if we want to forcefully shut down the browser process
This TODO seems to have gotten lost?

https://codereview.chromium.org/1072043003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_ui_manager_android.cc (right):

https://codereview.chromium.org/1072043003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:154: if
(!base::StringToInt64(delegate_id, &persistent_notification_id))
Nit: please add a TODO about changing delegate_ids in content/.

https://codereview.chromium.org/1072043003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:185:
NOTIMPLEMENTED();
This is currently used by PushMessagingServiceImpl::RequireUserVisibleUX. One of
us should update that to use the notifications database.

https://codereview.chromium.org/1072043003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:196:
NOTIMPLEMENTED();
Doesn't not implementing this mean that NotificationSystemObserver::Observe[1]
will fail to close incognito notifications when the last incognito tab is
closed?

Why don't we make these methods (at least the CancelAll* ones) work by reading
the IDs from the database? It doesn't matter if you try to cancel a notification
the user has already swiped away...

[1]:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/not...

https://codereview.chromium.org/1072043003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_ui_manager_android.h (right):

https://codereview.chromium.org/1072043003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.h:27: class
NotificationUIManagerAndroid : public NotificationUIManager {
With all the NOTIMPLEMENTEDs, the only methods of NotificationUIManager you
actually implement are Add and CancelById (and you misinterpret the id for
CancelById ;) ).

I suggested below that you add implementations for at least CancelAll* backed by
the notifications database. But alternatively, maybe you should create a new
PersistentNotificationUIManager class, and have this class only implement that,
while MessageCenterNotificationManager would implement both that and
NotificationUIManager?

[Peter Beverloo, 2015-04-09 17:34:42]

Thanks for the review!

https://codereview.chromium.org/1072043003/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java
(left):

https://codereview.chromium.org/1072043003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java:242:
public void testNotificationDeleteIntentClosesNotification() throws Exception {
On 2015/04/09 17:00:01, johnme wrote:
> I don't understand why this test doesn't make sense anymore? What do you mean
by
> "it stops us from double-closing notifications"?

Firstly - good point, I'm now removing this test in the larger integration CL.

The reason that this test worked is because, prior to the integration CL, the
close handler in NotificationUIManagerAndroid called RemoveProfileNotification
which then closed the notification (again!) on the platform.

That obviously is not necessary, since the OnNotificationClosed gets called
*when* a notification closes. This test was kind of bogus to start with and only
worked due to an implementation detail.

https://codereview.chromium.org/1072043003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_ui_manager_android.cc (left):

https://codereview.chromium.org/1072043003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:37: //
TODO(peter): Decide if we want to forcefully shut down the browser process
On 2015/04/09 17:00:01, johnme wrote:
> This TODO seems to have gotten lost?

It's still captured in NotificationService.java:91. Since we don't have any
concrete plans, simplifying this code for now is the better option.

https://codereview.chromium.org/1072043003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_ui_manager_android.cc (right):

https://codereview.chromium.org/1072043003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:154: if
(!base::StringToInt64(delegate_id, &persistent_notification_id))
On 2015/04/09 17:00:01, johnme wrote:
> Nit: please add a TODO about changing delegate_ids in content/.

Done.

https://codereview.chromium.org/1072043003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:185:
NOTIMPLEMENTED();
On 2015/04/09 17:00:01, johnme wrote:
> This is currently used by PushMessagingServiceImpl::RequireUserVisibleUX. One
of
> us should update that to use the notifications database.

There's already a row for that on the spreadsheet, because we're aware about it
being unreliable. I'm happy to take it tomorrow unless you're jumping to?

https://codereview.chromium.org/1072043003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:196:
NOTIMPLEMENTED();
On 2015/04/09 17:00:01, johnme wrote:
> Doesn't not implementing this mean that NotificationSystemObserver::Observe[1]
> will fail to close incognito notifications when the last incognito tab is
> closed?
> 
> Why don't we make these methods (at least the CancelAll* ones) work by reading
> the IDs from the database? It doesn't matter if you try to cancel a
notification
> the user has already swiped away...
> 
> [1]:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/not...

I'm not sure what our incognito story is right now - it's definitely not well
thought out or tested.

Using the notification database only for persistent notifications was a
trade-off we made a couple of weeks ago. The door's not closed for extensions
and/or other notifications to be part of this, but it'd be inherently incomplete
because we create a bunch of notifications on Android in Java as well, whereas
on desktop this is the only entry point.

https://codereview.chromium.org/1072043003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_ui_manager_android.h (right):

https://codereview.chromium.org/1072043003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.h:27: class
NotificationUIManagerAndroid : public NotificationUIManager {
On 2015/04/09 17:00:01, johnme wrote:
> With all the NOTIMPLEMENTEDs, the only methods of NotificationUIManager you
> actually implement are Add and CancelById (and you misinterpret the id for
> CancelById ;) ).
> 
> I suggested below that you add implementations for at least CancelAll* backed
by
> the notifications database. But alternatively, maybe you should create a new
> PersistentNotificationUIManager class, and have this class only implement
that,
> while MessageCenterNotificationManager would implement both that and
> NotificationUIManager?

That could work out, but requires some additional thinking. I like the idea.

Filed https://crbug.com/475599 and added it to the spreadsheet.

[johnme, 2015-04-10 11:37:47]

notifications lgtm

https://codereview.chromium.org/1072043003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_ui_manager_android.cc (right):

https://codereview.chromium.org/1072043003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:185:
NOTIMPLEMENTED();
On 2015/04/09 17:34:42, Peter Beverloo wrote:
> On 2015/04/09 17:00:01, johnme wrote:
> > This is currently used by PushMessagingServiceImpl::RequireUserVisibleUX.
One
> of
> > us should update that to use the notifications database.
> 
> There's already a row for that on the spreadsheet, because we're aware about
it
> being unreliable. I'm happy to take it tomorrow unless you're jumping to?

There's a difference between unreliable-across-restarts and totally broken ;)
But we're far from a branch point, so I'm happy for that to be a follow-up
patch, that I'll make sure to chase up.

https://codereview.chromium.org/1072043003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:196:
NOTIMPLEMENTED();
On 2015/04/09 17:34:42, Peter Beverloo wrote:
> On 2015/04/09 17:00:01, johnme wrote:
> > Doesn't not implementing this mean that
NotificationSystemObserver::Observe[1]
> > will fail to close incognito notifications when the last incognito tab is
> > closed?
> > 
> > Why don't we make these methods (at least the CancelAll* ones) work by
reading
> > the IDs from the database? It doesn't matter if you try to cancel a
> notification
> > the user has already swiped away...
> > 
> > [1]:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/not...
> 
> I'm not sure what our incognito story is right now - it's definitely not well
> thought out or tested.

Ok, we chatted offline and you explained that the notifications database will
get cleared when exiting incognito, which will cause the notifications to be
closed (though that last part isn't yet hooked up - added a task to the
spreadsheet). Sounds reasonable.

[dewittj, 2015-04-10 17:06:59]

lgtm

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

https://codereview.chromium.org/1072043003/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_ui_manager_android.cc:188:
NOTIMPLEMENTED();
I wonder if we should be harsher about these methods and dcheck.  I certainly
don't think all developers are going to be running Android as their development
platform and returning just plain wrong results might cause bugs

[Peter Beverloo, 2015-04-14 17:53:40]

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

https://codereview.chromium.org/1072043003/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_ui_manager_android.cc:188:
NOTIMPLEMENTED();
On 2015/04/10 17:06:59, dewittj wrote:
> I wonder if we should be harsher about these methods and dcheck.  I certainly
> don't think all developers are going to be running Android as their
development
> platform and returning just plain wrong results might cause bugs

That sounds good to me. Changed them to NOTREACHED().

[Peter Beverloo, 2015-04-15 17:01:56]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-04-15 17:01:57]

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

[commit-bot: I haz the power, 2015-04-15 17:03:01]

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

[commit-bot: I haz the power, 2015-04-15 17:57:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-15 17:58:44]

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