Plumb Blink notification actions to button UI on desktop

Plumb Blink notification actions to button UI on desktop

Click handling will be hooked up later.

Depends on:
- https://codereview.chromium.org/1263043002
- https://codereview.chromium.org/1263043003

BUG=513671
R=avi@chromium.org, palmer@chromium.org, peter@chromium.org
TBR=sky@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/5f170b9a3b404f702a29958f3be0cb5085f956f3

[johnme, 2015-07-30 16:34:22]

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

[Peter Beverloo, 2015-07-30 17:02:05]

Could you include the database changes in this patch as well, please? The
plumbing is trivial enough for that.

Thanks!

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

https://codereview.chromium.org/1267673003/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:340: for
(content::PlatformNotificationAction action : notification_data.actions)
const& to avoid the copies.

Fine to use auto.

https://codereview.chromium.org/1267673003/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:342:
notification.set_buttons(buttons);
micro nit: blank line above this one.

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

https://codereview.chromium.org/1267673003/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_unittest.cc:218:
EXPECT_EQ("Button 1", base::UTF16ToUTF8(notification.buttons()[0].title));
Could we cache a reference to notification.buttons() to avoid needing the "()[]"
stuff? We're using it three times anyway.

https://codereview.chromium.org/1267673003/diff/1/content/child/notifications...
File content/child/notifications/notification_data_conversions.cc (right):

https://codereview.chromium.org/1267673003/diff/1/content/child/notifications...
content/child/notifications/notification_data_conversions.cc:33:
platform_data.actions.clear();
This is not necessary - it's a new object.

https://codereview.chromium.org/1267673003/diff/1/content/child/notifications...
content/child/notifications/notification_data_conversions.cc:61:
web_data.actions.swap(cleared_and_resized);
This is not necessary - it's a new object.

https://codereview.chromium.org/1267673003/diff/1/content/child/notifications...
content/child/notifications/notification_data_conversions.cc:61:
web_data.actions.swap(cleared_and_resized);
Just use web_data.actions.resize()?

https://codereview.chromium.org/1267673003/diff/1/content/child/notifications...
File content/child/notifications/notification_data_conversions_unittest.cc
(right):

https://codereview.chromium.org/1267673003/diff/1/content/child/notifications...
content/child/notifications/notification_data_conversions_unittest.cc:40:
web_actions[0].action = kAction1Name;
Did you compile this? You're assigning a char* to a WebString, which is UTF-16.

https://codereview.chromium.org/1267673003/diff/1/content/common/platform_not...
File content/common/platform_notification_messages.h (right):

https://codereview.chromium.org/1267673003/diff/1/content/common/platform_not...
content/common/platform_notification_messages.h:53:
IPC_STRUCT_TRAITS_MEMBER(actions)
The receiving side (browser) must verify that the number of buttons in this
structure is equal to or lower than whatever maximum we take. (There should be a
constant in /content/common/.)

https://codereview.chromium.org/1267673003/diff/1/content/public/common/platf...
File content/public/common/platform_notification_action.h (right):

https://codereview.chromium.org/1267673003/diff/1/content/public/common/platf...
content/public/common/platform_notification_action.h:17: struct CONTENT_EXPORT
PlatformNotificationAction {
This is a very straightforward struct. Could we just have this in
platform_notification_data.h?

(I don't think it needs this level of documentation either. Just saying "it
could be a button" is fine.)

[johnme, 2015-07-31 18:21:31]

Addressed review comments. Haven't yet added the database changes.

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

https://codereview.chromium.org/1267673003/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:340: for
(content::PlatformNotificationAction action : notification_data.actions)
On 2015/07/30 17:02:04, Peter Beverloo wrote:
> const& to avoid the copies.
> 
> Fine to use auto.

Done.

https://codereview.chromium.org/1267673003/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:342:
notification.set_buttons(buttons);
On 2015/07/30 17:02:05, Peter Beverloo wrote:
> micro nit: blank line above this one.

Done.

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

https://codereview.chromium.org/1267673003/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_unittest.cc:218:
EXPECT_EQ("Button 1", base::UTF16ToUTF8(notification.buttons()[0].title));
On 2015/07/30 17:02:05, Peter Beverloo wrote:
> Could we cache a reference to notification.buttons() to avoid needing the
"()[]"
> stuff? We're using it three times anyway.

Done (const auto& buttons)

https://codereview.chromium.org/1267673003/diff/1/content/child/notifications...
File content/child/notifications/notification_data_conversions.cc (right):

https://codereview.chromium.org/1267673003/diff/1/content/child/notifications...
content/child/notifications/notification_data_conversions.cc:33:
platform_data.actions.clear();
On 2015/07/30 17:02:05, Peter Beverloo wrote:
> This is not necessary - it's a new object.

Done.

https://codereview.chromium.org/1267673003/diff/1/content/child/notifications...
content/child/notifications/notification_data_conversions.cc:61:
web_data.actions.swap(cleared_and_resized);
On 2015/07/30 17:02:05, Peter Beverloo wrote:
> Just use web_data.actions.resize()?

WebVector has no resize.

https://codereview.chromium.org/1267673003/diff/1/content/child/notifications...
content/child/notifications/notification_data_conversions.cc:61:
web_data.actions.swap(cleared_and_resized);
On 2015/07/30 17:02:05, Peter Beverloo wrote:
> This is not necessary - it's a new object.

Renamed this, to avoid the implication that it's clearing anything.

https://codereview.chromium.org/1267673003/diff/1/content/child/notifications...
File content/child/notifications/notification_data_conversions_unittest.cc
(right):

https://codereview.chromium.org/1267673003/diff/1/content/child/notifications...
content/child/notifications/notification_data_conversions_unittest.cc:40:
web_actions[0].action = kAction1Name;
On 2015/07/30 17:02:05, Peter Beverloo wrote:
> Did you compile this? You're assigning a char* to a WebString, which is
UTF-16.

Done.

https://codereview.chromium.org/1267673003/diff/1/content/common/platform_not...
File content/common/platform_notification_messages.h (right):

https://codereview.chromium.org/1267673003/diff/1/content/common/platform_not...
content/common/platform_notification_messages.h:53:
IPC_STRUCT_TRAITS_MEMBER(actions)
On 2015/07/30 17:02:05, Peter Beverloo wrote:
> The receiving side (browser) must verify that the number of buttons in this
> structure is equal to or lower than whatever maximum we take. (There should be
a
> constant in /content/common/.)

Done (SanitizeNotificationData in notification_message_filter.cc now takes care
of this).

https://codereview.chromium.org/1267673003/diff/1/content/public/common/platf...
File content/public/common/platform_notification_action.h (right):

https://codereview.chromium.org/1267673003/diff/1/content/public/common/platf...
content/public/common/platform_notification_action.h:17: struct CONTENT_EXPORT
PlatformNotificationAction {
On 2015/07/30 17:02:05, Peter Beverloo wrote:
> This is a very straightforward struct. Could we just have this in
> platform_notification_data.h?
> 
> (I don't think it needs this level of documentation either. Just saying "it
> could be a button" is fine.)

Done.

[Peter Beverloo, 2015-07-31 19:17:51]

lgtm % nits. Thanks!

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

https://codereview.chromium.org/1267673003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_unittest.cc:190:
notification_data.actions.resize(2);
nit: operator[] will do this for you (that's why it can't be const)

https://codereview.chromium.org/1267673003/diff/40001/content/browser/notific...
File content/browser/notifications/notification_database_data_unittest.cc
(right):

https://codereview.chromium.org/1267673003/diff/40001/content/browser/notific...
content/browser/notifications/notification_database_data_unittest.cc:46: for
(int i = 0; i < 2; i++) {
nit: might as well use the constant (in case we ever want to set it to 1).

https://codereview.chromium.org/1267673003/diff/40001/content/browser/notific...
content/browser/notifications/notification_database_data_unittest.cc:48:
notification_action.action = IntToString(i);
base::

https://codereview.chromium.org/1267673003/diff/40001/content/browser/notific...
content/browser/notifications/notification_database_data_unittest.cc:49:
notification_action.title = IntToString16(i);
dito

https://codereview.chromium.org/1267673003/diff/40001/content/common/notifica...
File content/common/notification_constants.h (right):

https://codereview.chromium.org/1267673003/diff/40001/content/common/notifica...
content/common/notification_constants.h:10: // All known content embedders that
support web notifications support up to 2
// Maximum number of actions on a Platform Notification.

[johnme, 2015-07-31 19:38:25]

Addressed review comments, and fixed tests - thanks!

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

https://codereview.chromium.org/1267673003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_unittest.cc:190:
notification_data.actions.resize(2);
On 2015/07/31 19:17:51, Peter Beverloo wrote:
> nit: operator[] will do this for you (that's why it can't be const)

Only for maps, not for vectors!

https://codereview.chromium.org/1267673003/diff/40001/content/browser/notific...
File content/browser/notifications/notification_database_data_unittest.cc
(right):

https://codereview.chromium.org/1267673003/diff/40001/content/browser/notific...
content/browser/notifications/notification_database_data_unittest.cc:46: for
(int i = 0; i < 2; i++) {
On 2015/07/31 19:17:51, Peter Beverloo wrote:
> nit: might as well use the constant (in case we ever want to set it to 1).

Done.

https://codereview.chromium.org/1267673003/diff/40001/content/browser/notific...
content/browser/notifications/notification_database_data_unittest.cc:48:
notification_action.action = IntToString(i);
On 2015/07/31 19:17:51, Peter Beverloo wrote:
> base::

Done.

https://codereview.chromium.org/1267673003/diff/40001/content/browser/notific...
content/browser/notifications/notification_database_data_unittest.cc:49:
notification_action.title = IntToString16(i);
On 2015/07/31 19:17:51, Peter Beverloo wrote:
> dito

Done.

https://codereview.chromium.org/1267673003/diff/40001/content/common/notifica...
File content/common/notification_constants.h (right):

https://codereview.chromium.org/1267673003/diff/40001/content/common/notifica...
content/common/notification_constants.h:10: // All known content embedders that
support web notifications support up to 2
On 2015/07/31 19:17:51, Peter Beverloo wrote:
> // Maximum number of actions on a Platform Notification.

Done.

[johnme, 2015-08-03 15:59:41]

johnme@chromium.org changed reviewers:
+ avi@chromium.org, palmer@chromium.org

[johnme, 2015-08-03 15:59:41]

palmer@chromium.org: Please review
content/common/platform_notification_messages.h

avi@chromium.org: Please review changes in content/public/ and
content/common/notification_constants.h

Thanks!

[johnme, 2015-08-03 16:00:40]

johnme@chromium.org changed reviewers:
+ sky@chromium.org

[Avi (use Gerrit), 2015-08-03 16:07:51]

lgtm

[palmer, 2015-08-03 21:39:01]

IPC security LGTM.

[johnme, 2015-08-04 09:44:59]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-08-04 09:44:59]

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/1267673003/#ps60001
(title: "Address review comments, and fix tests")

[commit-bot: I haz the power, 2015-08-04 09:45:05]

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

[commit-bot: I haz the power, 2015-08-04 09:55:56]

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

[commit-bot: I haz the power, 2015-08-04 09:55:57]

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

[johnme, 2015-08-04 10:46:53]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-08-04 10:46:53]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
palmer@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1267673003/#ps80001
(title: "Fix typo")

[commit-bot: I haz the power, 2015-08-04 10:47:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1267673003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1267673003/80001

[commit-bot: I haz the power, 2015-08-04 11:02:11]

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

[commit-bot: I haz the power, 2015-08-04 11:02:11]

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

[johnme, 2015-08-04 18:01:36]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-08-04 18:01:36]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
palmer@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1267673003/#ps100001
(title: "Fix size_t")

[commit-bot: I haz the power, 2015-08-04 18:04:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1267673003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1267673003/100001

[commit-bot: I haz the power, 2015-08-04 19:55:19]

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

[commit-bot: I haz the power, 2015-08-04 19:55:20]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[johnme, 2015-08-04 22:19:30]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2015-08-04 22:20:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1267673003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1267673003/100001

[commit-bot: I haz the power, 2015-08-04 23:52:57]

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

[commit-bot: I haz the power, 2015-08-04 23:52:58]

Failed to commit the patch.

[johnme, 2015-08-05 01:01:17]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2015-08-05 01:01:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1267673003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1267673003/100001

[commit-bot: I haz the power, 2015-08-05 01:10:43]

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

[commit-bot: I haz the power, 2015-08-05 01:10:45]

Failed to commit the patch.

[commit-bot: I haz the power, 2015-08-05 01:11:07]

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

[johnme, 2015-08-05 10:37:08]

Committed patchset #6 (id:100001) manually as
5f170b9a3b404f702a29958f3be0cb5085f956f3 (presubmit successful).