Add support for native extension notifications

Add support for native extension notifications

Add the ability to:
- Remove attribution
- Remove the settings button



BUG=571056
Review-Url: https://codereview.chromium.org/2799343003
Cr-Commit-Position: refs/heads/master@{#462906}
Committed: https://chromium.googlesource.com/chromium/src/+/d34ce64d14b289d4faddbe13daf17a204bb5bb6f

[Miguel Garcia, 2017-04-06 21:42:58]

The CQ bit was checked by miguelg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 21:43:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Miguel Garcia, 2017-04-06 21:44:37]

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

[commit-bot: I haz the power, 2017-04-06 22:38:09]

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

[commit-bot: I haz the power, 2017-04-06 22:38:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Beverloo, 2017-04-07 02:26:53]

https://codereview.chromium.org/2799343003/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/notifications/extension_notification_handler.h
(right):

https://codereview.chromium.org/2799343003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/notifications/extension_notification_handler.h:13:
class ExtensionNotificationHandler : public NonPersistentNotificationHandler {
Hmm. I like the idea of reasoning about extension notifications as
non-persistent notifications as it describes their behaviour. At some point we
could update them to be persistent notifications.

https://codereview.chromium.org/2799343003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/notifications/notifications_api.cc (right):

https://codereview.chromium.org/2799343003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/notifications/notifications_api.cc:447:
optional_fields.context_message = base::UTF8ToUTF16("");
Why is this necessary? I think this is a no-op? It's a base::string16 member so
it should be initialized as the empty string.

If we really need to initialize it, you could use base::EmptyString16(), but I'm
sceptical.

https://codereview.chromium.org/2799343003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/notifications/notifications_api.cc:590:
notification->set_context_message(base::UTF8ToUTF16(""));
(dito)

https://codereview.chromium.org/2799343003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://codereview.chromium.org/2799343003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_mac.mm:184:
notification_type != NotificationCommon::EXTENSION;
Just checking for "notification_type != EXTENSION" should be sufficient, right?

https://codereview.chromium.org/2799343003/diff/1/chrome/browser/ui/cocoa/not...
File chrome/browser/ui/cocoa/notifications/notification_builder_mac.mm (right):

https://codereview.chromium.org/2799343003/diff/1/chrome/browser/ui/cocoa/not...
chrome/browser/ui/cocoa/notifications/notification_builder_mac.mm:167: BOOL
showSettingsButton = (type.unsignedIntegerValue != kExtensionType);
Could we just give the notificationData_ dictionary another boolean for
kShowSettingButton? We need some hacky behaviour (for now), but we can save this
code from it.

[Miguel Garcia, 2017-04-07 13:28:16]

The CQ bit was checked by miguelg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 13:28:33]

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 13:30:03]

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

[commit-bot: I haz the power, 2017-04-07 13:30:04]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Miguel Garcia, 2017-04-07 13:32:07]

PTAL

As agreed feel free to integrate this or part of this directly into the
dependent patch.

https://codereview.chromium.org/2799343003/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/notifications/extension_notification_handler.h
(right):

https://codereview.chromium.org/2799343003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/notifications/extension_notification_handler.h:13:
class ExtensionNotificationHandler : public NonPersistentNotificationHandler {
On 2017/04/07 02:26:53, Peter Beverloo wrote:
> Hmm. I like the idea of reasoning about extension notifications as
> non-persistent notifications as it describes their behaviour. At some point we
> could update them to be persistent notifications.

Acknowledged.

https://codereview.chromium.org/2799343003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/notifications/notifications_api.cc (right):

https://codereview.chromium.org/2799343003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/notifications/notifications_api.cc:447:
optional_fields.context_message = base::UTF8ToUTF16("");
On 2017/04/07 02:26:53, Peter Beverloo wrote:
> Why is this necessary? I think this is a no-op? It's a base::string16 member
so
> it should be initialized as the empty string.
> 
> If we really need to initialize it, you could use base::EmptyString16(), but
I'm
> sceptical.

You are right, it was not needed.

https://codereview.chromium.org/2799343003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/notifications/notifications_api.cc:590:
notification->set_context_message(base::UTF8ToUTF16(""));
On 2017/04/07 02:26:53, Peter Beverloo wrote:
> (dito)

Acknowledged.

https://codereview.chromium.org/2799343003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://codereview.chromium.org/2799343003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_mac.mm:184:
notification_type != NotificationCommon::EXTENSION;
On 2017/04/07 02:26:53, Peter Beverloo wrote:
> Just checking for "notification_type != EXTENSION" should be sufficient,
right?

mm no? We need the class prefix :)

https://codereview.chromium.org/2799343003/diff/1/chrome/browser/ui/cocoa/not...
File chrome/browser/ui/cocoa/notifications/notification_builder_mac.mm (right):

https://codereview.chromium.org/2799343003/diff/1/chrome/browser/ui/cocoa/not...
chrome/browser/ui/cocoa/notifications/notification_builder_mac.mm:167: BOOL
showSettingsButton = (type.unsignedIntegerValue != kExtensionType);
On 2017/04/07 02:26:53, Peter Beverloo wrote:
> Could we just give the notificationData_ dictionary another boolean for
> kShowSettingButton? We need some hacky behaviour (for now), but we can save
this
> code from it.

Yes, in fact we need to store the fact so the response clicks can be processed
correctly, see changes in the response_builder too.

[Miguel Garcia, 2017-04-07 13:32:25]

The CQ bit was checked by miguelg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 13:32:46]

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 13:34:26]

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

[commit-bot: I haz the power, 2017-04-07 13:34:27]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Miguel Garcia, 2017-04-07 16:07:54]

The CQ bit was checked by miguelg@chromium.org to run a CQ dry run

[Miguel Garcia, 2017-04-07 16:08:26]

Description was changed from

==========
A few fixes for native extension notifications

- Remove attribution
- Remove the settings button
- Propagate click and close events
- Fix crash when closing the browser due to using the profile
  after it has been destroyed

BUG=571056
==========

to

==========
Add the ability to

- Remove attribution
- Remove the settings button



BUG=571056
==========

[commit-bot: I haz the power, 2017-04-07 16:08:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Miguel Garcia, 2017-04-07 16:08:52]

PTAL As agreed I made this patch independent so it can land directly

[Miguel Garcia, 2017-04-07 16:09:11]

CC Robert as well

[Peter Beverloo, 2017-04-07 16:19:05]

lgtm

https://codereview.chromium.org/2799343003/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/notifications/notification_builder_mac.mm (right):

https://codereview.chromium.org/2799343003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/notification_builder_mac.mm:154:
objectForKey:notification_constants::kNotificationType];
Move back to line 219 w/o the comment now that |type| isn't used for that
anymore.

https://codereview.chromium.org/2799343003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/notification_builder_mac.mm:156: //
Extensions don't have a settings button.
nit: this layer doesn't care, I'd just generalize as "Not all notifications have
a settings button". Which, in turn, is implied by the fact that it's called "has
settings button".

https://codereview.chromium.org/2799343003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/notification_builder_mac.mm:187: [toast
setHasActionButton:NO];
Is this necessary? Surely that's the default?

https://codereview.chromium.org/2799343003/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.mm
(right):

https://codereview.chromium.org/2799343003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.mm:89:
alternateButtons.count - 1)
nit: this would become a lot easier to read if you'd extract an `isLastButton`
local:

BOOL isLastButton = actionIndex.unsignedLongValue == alternateButtons.count - 1;

[Peter Beverloo, 2017-04-07 16:21:59]

Please do clean up the CLs description. Or, rather, make it whole again.

[commit-bot: I haz the power, 2017-04-07 17:09:57]

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

[commit-bot: I haz the power, 2017-04-07 17:09:58]

Dry run: This issue passed the CQ dry run.

[Miguel Garcia, 2017-04-07 17:13:26]

Description was changed from

==========
Add the ability to

- Remove attribution
- Remove the settings button



BUG=571056
==========

to

==========
Add support for native extension notifications

Add the ability to:
- Remove attribution
- Remove the settings button



BUG=571056
==========

[Miguel Garcia, 2017-04-07 17:13:34]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2017-04-07 17:13:57]

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 17:19:58]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491585214932990,
"parent_rev": "1222984cb4d97e3e51a0d524ca623a18d39d1d4a", "commit_rev":
"d34ce64d14b289d4faddbe13daf17a204bb5bb6f"}

[commit-bot: I haz the power, 2017-04-07 17:20:38]

Description was changed from

==========
Add support for native extension notifications

Add the ability to:
- Remove attribution
- Remove the settings button



BUG=571056
==========

to

==========
Add support for native extension notifications

Add the ability to:
- Remove attribution
- Remove the settings button



BUG=571056

Review-Url: https://codereview.chromium.org/2799343003
Cr-Commit-Position: refs/heads/master@{#462906}
Committed:
https://chromium.googlesource.com/chromium/src/+/d34ce64d14b289d4faddbe13daf1...
==========

[commit-bot: I haz the power, 2017-04-07 17:20:39]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d34ce64d14b289d4faddbe13daf1...