Add NotificationDisplayServiceProxy

Add NotificationDisplayServiceProxy

BUG=676220
R=thestig@chromium.org,peter@chromium.org

[Tom (Use chromium acct), 2017-04-19 01:44:20]

thestig and peter ptal

Hopefully this CL should ground some discussion about the async initialization.

Still 2 TODOs and need to remove "const" in the mac and android impls.  And
maybe rename NotificationDisplayServiceProxy to
NotificationDisplayServiceProxyLinux.

[Peter Beverloo, 2017-04-19 13:26:41]

A few comments inline. I have a preference of coalescing this into the
NativeNotificationDisplayService, changing the assumption that initialising the
NotificationPlatformBridge can be synchronous.

I've also started a thread with some process thoughts on moving forward.

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

https://codereview.chromium.org/2828503005/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_display_service.h:45: virtual void
GetDisplayed(const DisplayedNotificationsCallback& callback) = 0;
You probably have to rebase at some point - the `const` qualifiers went over a
week ago.

https://codereview.chromium.org/2813943002/

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

https://codereview.chromium.org/2828503005/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_display_service_proxy.cc:19: auto*
npbl = static_cast<NotificationPlatformBridgeLinux*>(
Here and elsewhere (impls, nds, etc) - Chromium style has a reasonably strong
dislike towards using acronyms for identifiers. Maybe substitute like:

npbl -> platform_bridge_linux
impls -> implementations
nds -> display_service

https://codereview.chromium.org/2828503005/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_display_service_proxy.cc:23:
base::Unretained(this)));
There's two race conditions here:

  (1) We can hit the DCHECK on notification_platform_bridge_linux.cc:269 when
multiple profiles are being initialised at once.
  (2) Super unlikely, but |this| may not outlive the |npbl|. Use a WeakPtr?

https://codereview.chromium.org/2828503005/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_display_service_proxy.cc:50: //
TODO(thomasanderson): clean up |g_browser_process->notification_bridge_|?
Can the NPBL just stop its own thread when it identifies that the object proxy
cannot be obtained?

https://codereview.chromium.org/2828503005/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_display_service_proxy.cc:82:
notification_type, notification_id, notification));
Instead of this pattern, consider something like:

=========================
if (initialized) {
  GetNotificationDisplayService()->Display(..);
  return;
}

QueueTask(base::Bind(&NotificationDisplayServiceProxy::Display,
                     base::Unretained(this), ...));
=========================

That'll avoid having to copy all the arguments in the common case, as well as
the closures. See e.g. the GCMDelayedTaskController for an application of this.

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

https://codereview.chromium.org/2828503005/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_display_service_proxy.h:8: #include
<queue>
#include <memory>
#include "base/callback.h"
class Profile;

(iwyu)

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

https://codereview.chromium.org/2828503005/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_linux.h:32: void
IsConnected(const base::Callback<void(bool)>& callback);
nit: there's a bunch of massive refactoring CLs in flight where people update
base::Callback to base::OnceCallback. Looks like the semantics of this qualify.

[Tom (Use chromium acct), 2017-04-20 01:43:47]

Closing this CL (too many merge conflicts) and merging into
https://codereview.chromium.org/2821533003/

On 2017/04/19 13:26:41, Peter Beverloo wrote:
> A few comments inline. I have a preference of coalescing this into the
> NativeNotificationDisplayService, changing the assumption that initialising
the
> NotificationPlatformBridge can be synchronous.
> 

Done.

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

https://codereview.chromium.org/2828503005/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_display_service.h:45: virtual void
GetDisplayed(const DisplayedNotificationsCallback& callback) = 0;
On 2017/04/19 13:26:41, Peter Beverloo wrote:
> You probably have to rebase at some point - the `const` qualifiers went over a
> week ago.
> 
> https://codereview.chromium.org/2813943002/

Rebased.

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

https://codereview.chromium.org/2828503005/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_display_service_proxy.cc:19: auto*
npbl = static_cast<NotificationPlatformBridgeLinux*>(
On 2017/04/19 13:26:41, Peter Beverloo wrote:
> Here and elsewhere (impls, nds, etc) - Chromium style has a reasonably strong
> dislike towards using acronyms for identifiers. Maybe substitute like:
> 
> npbl -> platform_bridge_linux
> impls -> implementations
> nds -> display_service

Done.

https://codereview.chromium.org/2828503005/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_display_service_proxy.cc:23:
base::Unretained(this)));
On 2017/04/19 13:26:41, Peter Beverloo wrote:
> There's two race conditions here:
> 
>   (1) We can hit the DCHECK on notification_platform_bridge_linux.cc:269 when
> multiple profiles are being initialised at once.

Good catch, fixed!

>   (2) Super unlikely, but |this| may not outlive the |npbl|. Use a WeakPtr?

Done.

https://codereview.chromium.org/2828503005/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_display_service_proxy.cc:50: //
TODO(thomasanderson): clean up |g_browser_process->notification_bridge_|?
On 2017/04/19 13:26:41, Peter Beverloo wrote:
> Can the NPBL just stop its own thread when it identifies that the object proxy
> cannot be obtained?

Done.

https://codereview.chromium.org/2828503005/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_display_service_proxy.cc:82:
notification_type, notification_id, notification));
On 2017/04/19 13:26:41, Peter Beverloo wrote:
> Instead of this pattern, consider something like:
> 
> =========================
> if (initialized) {
>   GetNotificationDisplayService()->Display(..);
>   return;
> }
> 
> QueueTask(base::Bind(&NotificationDisplayServiceProxy::Display,
>                      base::Unretained(this), ...));
> =========================
> 
> That'll avoid having to copy all the arguments in the common case, as well as
> the closures. See e.g. the GCMDelayedTaskController for an application of
this.

Done.

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

https://codereview.chromium.org/2828503005/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_display_service_proxy.h:8: #include
<queue>
On 2017/04/19 13:26:41, Peter Beverloo wrote:
> #include <memory>
> #include "base/callback.h"
> class Profile;
> 
> (iwyu)

Done.

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

https://codereview.chromium.org/2828503005/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_linux.h:32: void
IsConnected(const base::Callback<void(bool)>& callback);
On 2017/04/19 13:26:41, Peter Beverloo wrote:
> nit: there's a bunch of massive refactoring CLs in flight where people update
> base::Callback to base::OnceCallback. Looks like the semantics of this
qualify.

Done.

[Tom (Use chromium acct), 2017-04-20 01:43:53]

Description was changed from

==========
Add NotificationDisplayServiceProxy

BUG=676220
R=thestig@chromium.org,peter@chromium.org
==========

to

==========
Add NotificationDisplayServiceProxy

BUG=676220
R=thestig@chromium.org,peter@chromium.org
==========