Linux native notifications: Handle clicks and closes

Linux native notifications:  Handle clicks and closes

This CL adds the initial wiring to GLib and
NativeNotificationDisplayService to get actions and closes working.

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

Review-Url: https://codereview.chromium.org/2805543005
Cr-Commit-Position: refs/heads/master@{#463045}
Committed: https://chromium.googlesource.com/chromium/src/+/1e5d734b182b3bd7de2c36a88a26f05f857f59ec

[Tom (Use chromium acct), 2017-04-07 00:57:04]

Patchset #1 (id:1) has been deleted

[Tom (Use chromium acct), 2017-04-07 01:18:44]

Lei and Peter ptal

Peter:  What does the
NativeNotificationDisplayService::ProcessNotificationOperation do?  I just left
it empty for now.

Also, some DEs do not support notification actions.  The only one I know of is
Unity (the Ubuntu one), and that is discussed at crbug.com/676220.

What should we do in this case?  I'm inclined to just accept this as correct
behavior, but if we really want to, we could check during
NotificationPlatformBridge::Create if actions are supported and fallback on
Chrome notifications in this case.  At any rate, please see
https://bugs.chromium.org/p/chromium/issues/detail?id=676220#c25 for a demo of
actions on GNOME

[Peter Beverloo, 2017-04-07 01:50:48]

lgtm % comment in the header file, and including |is_incognito| in FND() if we
can.

On 2017/04/07 01:18:44, Tom Anderson wrote:
> Peter:  What does the
> NativeNotificationDisplayService::ProcessNotificationOperation do?  I just
left
> it empty for now.

It finds the notification handler (the feature responsible for showing the
notification, e.g. Web Notifications or extension notifications) that handles
the given |notification_type| and passes on the event. That then eventually
boils down to JavaScript, or native handling code if they so desire.

> Also, some DEs do not support notification actions.  The only one I know of is
> Unity (the Ubuntu one), and that is discussed at crbug.com/676220.
> 
> What should we do in this case?

We expose the static `Notification.maxActions` getter in JavaScript because we
anticipated that there'd be platforms that don't support action buttons.

It's currently hardcoded to 2. We'd have to do some plumbing (potentially
synchronous IPC, ugh) to enable Blink to check with Chrome on the number of
supported actions, but I'm fine with setting that to `0` and not supporting
actions for certain platforms.

> I'm inclined to just accept this as correct behavior, but if we really want
> to, we could check during NotificationPlatformBridge::Create if actions are
> supported and fallback on Chrome notifications in this case.

I don't think action buttons have to be a deal-breaker because developers can
detect this. I'd argue that the bare minimum is support for {title, body,
origin, click event}, with a strong preference to include {icon} in there too,
but UI review may disagree.

> At any rate, please see
> https://bugs.chromium.org/p/chromium/issues/detail?id=676220#c25 for a demo of
> actions on GNOME

That is fantastic! I've already enabled them in my local builds, and will switch
the flag in my Dev builds too once this CL lands there ^_^.

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

https://codereview.chromium.org/2805543005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux.cc:43: void
ProfileLoadedCallback(NotificationCommon::Operation operation,
FYI: I've written down a task for me to remove the copies of this.

https://codereview.chromium.org/2805543005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux.cc:144:
DCHECK_EQ(data->is_incognito, is_incognito);
I think it'd be possible for an extension to create a notification with the same
|notification_id| for a given |profile_id| if the extension is enabled in
Incognito as well. Maybe FND() should include the boolean in its matching now
that we store it?

https://codereview.chromium.org/2805543005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux.cc:226:
g_variant_builder_add(&actions_builder, "s", "Settings");
Cool!

https://codereview.chromium.org/2805543005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux.cc:226:
g_variant_builder_add(&actions_builder, "s", "Settings");
In the mid-term it might be interesting to split functionality like this out in
smaller helper functions to make them unit testable. Out of interest, can we
somehow fake being the receiver of the DBus calls?

https://codereview.chromium.org/2805543005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux.cc:277: // This
notification either belongs to a different app or we
We can get notified about changes to notifications shown by other apps??

https://codereview.chromium.org/2805543005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux.cc:303:
notifications_.erase(FindNotificationData(dbus_id));
Mm. I guess std::unordered_map_.erase(nullptr) is safe enough. It's a bit
implicit, maybe a comment would help?

https://codereview.chromium.org/2805543005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux.cc:307:
g_variant_get(parameters, "(u&s)", &dbus_id, &action);
Should we DCHECK(action) here, or is that me being pedantic due to my ignorance
about these APIs?

https://codereview.chromium.org/2805543005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_linux.h (right):

https://codereview.chromium.org/2805543005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux.h:70:
CHROMEG_CALLBACK_3(NotificationPlatformBridgeLinux,
Could you add some documentation on what this does? Looking at the definition of
CHROMEG_CALLBACK_3, this would end up calling
NotificationPlatformBridgeLinux::GSignalReceiver() with the sender and three
arguments arguments, but there is no method named GSignalReceiver in this class?

[Tom (Use chromium acct), 2017-04-07 18:59:52]

On 2017/04/07 01:50:48, Peter Beverloo wrote:
> lgtm % comment in the header file, and including |is_incognito| in FND() if we
> can.
> 
> On 2017/04/07 01:18:44, Tom Anderson wrote:
> > Peter:  What does the
> > NativeNotificationDisplayService::ProcessNotificationOperation do?  I just
> left
> > it empty for now.
> 
> It finds the notification handler (the feature responsible for showing the
> notification, e.g. Web Notifications or extension notifications) that handles
> the given |notification_type| and passes on the event. That then eventually
> boils down to JavaScript, or native handling code if they so desire.
> 

Oops, typo.  I meant to ask what the |reply| arg does, and if it's safe to leave
as base::NullableString16()

> > Also, some DEs do not support notification actions.  The only one I know of
is
> > Unity (the Ubuntu one), and that is discussed at crbug.com/676220.
> > 
> > What should we do in this case?
> 
> We expose the static `Notification.maxActions` getter in JavaScript because we
> anticipated that there'd be platforms that don't support action buttons.
> 
> It's currently hardcoded to 2. We'd have to do some plumbing (potentially
> synchronous IPC, ugh) to enable Blink to check with Chrome on the number of
> supported actions, but I'm fine with setting that to `0` and not supporting
> actions for certain platforms.
> 

Ok, plmk if/when this plumbing is added so I can provide a Linux impl with
org.freedesktop.Notifications.GetCapabilities

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

https://codereview.chromium.org/2805543005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux.cc:43: void
ProfileLoadedCallback(NotificationCommon::Operation operation,
On 2017/04/07 01:50:48, Peter Beverloo wrote:
> FYI: I've written down a task for me to remove the copies of this.

Acknowledged.

https://codereview.chromium.org/2805543005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux.cc:144:
DCHECK_EQ(data->is_incognito, is_incognito);
On 2017/04/07 01:50:47, Peter Beverloo wrote:
> I think it'd be possible for an extension to create a notification with the
same
> |notification_id| for a given |profile_id| if the extension is enabled in
> Incognito as well. Maybe FND() should include the boolean in its matching now
> that we store it?

Done.  This changes the Close() impl though since it doesn't take |is_incognito|

https://codereview.chromium.org/2805543005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux.cc:226:
g_variant_builder_add(&actions_builder, "s", "Settings");
On 2017/04/07 01:50:48, Peter Beverloo wrote:
> In the mid-term it might be interesting to split functionality like this out
in
> smaller helper functions to make them unit testable. 

Done.

> Out of interest, can we
> somehow fake being the receiver of the DBus calls?

Yes.  We would have to create a private D-Bus server using
g_dbus_server_new(chromium_address).

Then, we use g_dbus_connection_new_for_address(chromium_address) to connect and
use g_dbus_connection_register_object() to register
/org/freedesktop/Notifications and install the callbacks on it.  This connection
acts as the notification server.

In client code, connect to chromium_address in the same way.  Except now when we
create the GDBusProxy, we use g_dbus_proxy_new() and give it the connection. 
Now we just pass it along to the NotificationPlatformBridgeLinux constructor as
normal.

https://codereview.chromium.org/2805543005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux.cc:277: // This
notification either belongs to a different app or we
On 2017/04/07 01:50:48, Peter Beverloo wrote:
> We can get notified about changes to notifications shown by other apps??

Yes, the linux system is anarchy :(

https://codereview.chromium.org/2805543005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux.cc:303:
notifications_.erase(FindNotificationData(dbus_id));
On 2017/04/07 01:50:48, Peter Beverloo wrote:
> Mm. I guess std::unordered_map_.erase(nullptr) is safe enough. It's a bit
> implicit, maybe a comment would help?

Done.

https://codereview.chromium.org/2805543005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux.cc:307:
g_variant_get(parameters, "(u&s)", &dbus_id, &action);
On 2017/04/07 01:50:48, Peter Beverloo wrote:
> Should we DCHECK(action) here, or is that me being pedantic due to my
ignorance
> about these APIs?

Done.

https://codereview.chromium.org/2805543005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_linux.h (right):

https://codereview.chromium.org/2805543005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux.h:70:
CHROMEG_CALLBACK_3(NotificationPlatformBridgeLinux,
On 2017/04/07 01:50:48, Peter Beverloo wrote:
> Could you add some documentation on what this does? Looking at the definition
of
> CHROMEG_CALLBACK_3, this would end up calling
> NotificationPlatformBridgeLinux::GSignalReceiver() with the sender and three
> arguments arguments, but there is no method named GSignalReceiver in this
class?

Done.  This declares GSignalReceiver

[Peter Beverloo, 2017-04-07 20:41:38]

On 2017/04/07 18:59:52, Tom Anderson wrote:
> On 2017/04/07 01:50:48, Peter Beverloo wrote:
> > lgtm % comment in the header file, and including |is_incognito| in FND() if
we
> > can.
> > 
> > On 2017/04/07 01:18:44, Tom Anderson wrote:
> > > Peter:  What does the
> > > NativeNotificationDisplayService::ProcessNotificationOperation do?  I just
> > left
> > > it empty for now.
> > 
> > It finds the notification handler (the feature responsible for showing the
> > notification, e.g. Web Notifications or extension notifications) that
handles
> > the given |notification_type| and passes on the event. That then eventually
> > boils down to JavaScript, or native handling code if they so desire.
> > 
> 
> Oops, typo.  I meant to ask what the |reply| arg does, and if it's safe to
leave
> as base::NullableString16()

Android and Mac OS X support inline replies in notifications. When one has been
provided by the user, it will be set to the |reply| argument. Setting it to
base::NullableString16 is fine.

> > > Also, some DEs do not support notification actions.  The only one I know
of
> is
> > > Unity (the Ubuntu one), and that is discussed at crbug.com/676220.
> > > 
> > > What should we do in this case?
> > 
> > We expose the static `Notification.maxActions` getter in JavaScript because
we
> > anticipated that there'd be platforms that don't support action buttons.
> > 
> > It's currently hardcoded to 2. We'd have to do some plumbing (potentially
> > synchronous IPC, ugh) to enable Blink to check with Chrome on the number of
> > supported actions, but I'm fine with setting that to `0` and not supporting
> > actions for certain platforms.
> > 
> 
> Ok, plmk if/when this plumbing is added so I can provide a Linux impl with
> org.freedesktop.Notifications.GetCapabilities

Ack

https://codereview.chromium.org/2805543005/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_linux.cc (right):

https://codereview.chromium.org/2805543005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux.cc:184: }
Filed crbug.com/709608 and will fix + cleanup.

[Tom (Use chromium acct), 2017-04-07 21:54:08]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 21:54:49]

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, 2017-04-07 22:40:05]

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

[commit-bot: I haz the power, 2017-04-07 22:40:06]

Dry run: This issue passed the CQ dry run.

[Tom (Use chromium acct), 2017-04-07 22:45:22]

The CQ bit was checked by thomasanderson@google.com

[Tom (Use chromium acct), 2017-04-07 22:45:22]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2805543005/#ps60001
(title: "Address peter@'s comments")

[commit-bot: I haz the power, 2017-04-07 22:46:12]

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, 2017-04-07 23:24:13]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491605122501000,
"parent_rev": "509a5b53985357e3dce645ad8d92b6769f438a9f", "commit_rev":
"1e5d734b182b3bd7de2c36a88a26f05f857f59ec"}

[commit-bot: I haz the power, 2017-04-07 23:25:09]

Description was changed from

==========
Linux native notifications:  Handle clicks and closes

This CL adds the initial wiring to GLib and
NativeNotificationDisplayService to get actions and closes working.

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

to

==========
Linux native notifications:  Handle clicks and closes

This CL adds the initial wiring to GLib and
NativeNotificationDisplayService to get actions and closes working.

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

Review-Url: https://codereview.chromium.org/2805543005
Cr-Commit-Position: refs/heads/master@{#463045}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e5d734b182b3bd7de2c36a88a26...
==========

[commit-bot: I haz the power, 2017-04-07 23:25:10]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/1e5d734b182b3bd7de2c36a88a26...