Notification actions may have an icon url.

Notification actions may have an icon url.

This is behind a runtime flag for two reasons:
* The implementation is incomplete.
* We're still evaluating the API design.

Intent to Implement and Ship: Notification Action Icons
https://groups.google.com/a/chromium.org/d/msg/blink-dev/IM0HxOP7HOA/y8tu6iq1CgAJ

BUG=581336
Committed: https://crrev.com/4a3e17c874bc4c4c90e5b0f8ec568520964695d4
Cr-Commit-Position: refs/heads/master@{#374649}

[Michael van Ouwerkerk, 2016-01-27 11:45:27]

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

[Michael van Ouwerkerk, 2016-01-27 11:45:28]

Peter, could you take a look please?

[Peter Beverloo, 2016-01-27 13:24:45]

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

https://codereview.chromium.org/1644573002/diff/1/content/public/common/platf...
content/public/common/platform_notification_data.h:31: GURL icon;
Please make sure that this struct and the IPC_STRUCT_TRAITS in
platform_notification_messages.h stay in sync. Do you want to do serialization
to the database in a separate patch, or add it to this CL? (It should be fairly
trivial.)

https://codereview.chromium.org/1644573002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/notifications/NotificationAction.idl
(right):

https://codereview.chromium.org/1644573002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/NotificationAction.idl:10:
[RuntimeEnabled=NotificationExperimental] USVString icon;
nit: since this bit us in the past, have you considered introducing a feature
flag specific to actions?

https://codereview.chromium.org/1644573002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/notifications/NotificationData.cpp
(right):

https://codereview.chromium.org/1644573002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/NotificationData.cpp:83: if
(RuntimeEnabledFeatures::notificationExperimentalEnabled()) {
We don't need this check anymore, the bindings folks fixed it in this CL:

https://codereview.chromium.org/1412433004

[Michael van Ouwerkerk, 2016-01-27 14:24:32]

Thanks Peter.

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

https://codereview.chromium.org/1644573002/diff/1/content/public/common/platf...
content/public/common/platform_notification_data.h:31: GURL icon;
On 2016/01/27 13:24:45, Peter Beverloo wrote:
> Please make sure that this struct and the IPC_STRUCT_TRAITS in
> platform_notification_messages.h stay in sync. Do you want to do serialization
> to the database in a separate patch, or add it to this CL? (It should be
fairly
> trivial.)

Done.

https://codereview.chromium.org/1644573002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/notifications/NotificationAction.idl
(right):

https://codereview.chromium.org/1644573002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/NotificationAction.idl:10:
[RuntimeEnabled=NotificationExperimental] USVString icon;
On 2016/01/27 13:24:45, Peter Beverloo wrote:
> nit: since this bit us in the past, have you considered introducing a feature
> flag specific to actions?

I did consider it, but wanted to get this out for review early. I'm glad you
asked. Let's make a point of actually doing it. Done.

https://codereview.chromium.org/1644573002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/notifications/NotificationData.cpp
(right):

https://codereview.chromium.org/1644573002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/NotificationData.cpp:83: if
(RuntimeEnabledFeatures::notificationExperimentalEnabled()) {
On 2016/01/27 13:24:45, Peter Beverloo wrote:
> We don't need this check anymore, the bindings folks fixed it in this CL:
> 
> https://codereview.chromium.org/1412433004

Oh look at that. Nice.

[Michael van Ouwerkerk, 2016-01-27 14:27:05]

mvanouwerkerk@chromium.org changed reviewers:
+ mkwst@chromium.org

[Michael van Ouwerkerk, 2016-01-27 14:27:05]

Mike, could you please take a look for platform_notification_messages.h and
RuntimeEnabledFeatures.in?

Avi, could you please take a look for
content/public/common/platform_notification_data.h?

[Michael van Ouwerkerk, 2016-01-27 14:27:32]

mvanouwerkerk@chromium.org changed reviewers:
+ avi@chromium.org

[Michael van Ouwerkerk, 2016-01-27 14:27:32]

Avi, could you please take a look for
content/public/common/platform_notification_data.h?

[Michael van Ouwerkerk, 2016-01-27 14:30:06]

mvanouwerkerk@chromium.org changed reviewers:
+ jochen@chromium.org
- avi@chromium.org

[Michael van Ouwerkerk, 2016-01-27 14:30:06]

Jochen, since Avi is OOO, could you take a look please for
content/public/common/platform_notification_data.h?

[Peter Beverloo, 2016-01-27 17:14:23]

lgtm

Would you please send the Intent to Implement before landing this CL, and refer
to it in the description?

https://codereview.chromium.org/1644573002/diff/20001/content/child/notificat...
File content/child/notifications/notification_data_conversions.cc (right):

https://codereview.chromium.org/1644573002/diff/20001/content/child/notificat...
content/child/notifications/notification_data_conversions.cc:54:
blink::WebStringToGURL(web_data.actions[i].icon.string());
PTAL at content/child/notifications/notification_data_conversions_unittest.cc

https://codereview.chromium.org/1644573002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/NotificationData.cpp
(right):

https://codereview.chromium.org/1644573002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationData.cpp:88:
webAction.icon = iconUrl;
PTAL at third_party/WebKit/Source/modules/notifications/NotificationDataTest.cpp

(An invalid icon test might be useful, too)

[jochen (gone - plz use gerrit), 2016-01-28 12:12:24]

https://codereview.chromium.org/1644573002/diff/20001/content/child/notificat...
File content/child/notifications/notification_data_conversions.cc (right):

https://codereview.chromium.org/1644573002/diff/20001/content/child/notificat...
content/child/notifications/notification_data_conversions.cc:54:
blink::WebStringToGURL(web_data.actions[i].icon.string());
should there be a max length of the URL?

https://codereview.chromium.org/1644573002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1644573002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:133:
NotificationActionIcons status=test
it says "test", but there are no tests, and that's really the most important
part

[Michael van Ouwerkerk, 2016-02-01 14:39:23]

Thanks guys. Jochen: please take another look.

https://codereview.chromium.org/1644573002/diff/20001/content/child/notificat...
File content/child/notifications/notification_data_conversions.cc (right):

https://codereview.chromium.org/1644573002/diff/20001/content/child/notificat...
content/child/notifications/notification_data_conversions.cc:54:
blink::WebStringToGURL(web_data.actions[i].icon.string());
On 2016/01/28 12:12:24, jochen wrote:
> should there be a max length of the URL?

Are you asking because this might be too large for Chrome to handle? Shouldn't
that kind of check take place in the JS bindings already, or as soon after that
as possible? In this case, this enters the system as a string from JS, is passed
into ExecutionContext.completeUrl, and then into a KURL constructor.

https://codereview.chromium.org/1644573002/diff/20001/content/child/notificat...
content/child/notifications/notification_data_conversions.cc:54:
blink::WebStringToGURL(web_data.actions[i].icon.string());
On 2016/01/27 17:14:23, Peter Beverloo wrote:
> PTAL at content/child/notifications/notification_data_conversions_unittest.cc

Done.

https://codereview.chromium.org/1644573002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/NotificationData.cpp
(right):

https://codereview.chromium.org/1644573002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationData.cpp:88:
webAction.icon = iconUrl;
On 2016/01/27 17:14:23, Peter Beverloo wrote:
> PTAL at
third_party/WebKit/Source/modules/notifications/NotificationDataTest.cpp
> 
> (An invalid icon test might be useful, too)

Done.

https://codereview.chromium.org/1644573002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1644573002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:133:
NotificationActionIcons status=test
On 2016/01/28 12:12:24, jochen wrote:
> it says "test", but there are no tests, and that's really the most important
> part

The layout tests require more of this feature to be implemented to make
meaningful assertions, so I've removed the test status for now. I've updated the
relevant unit tests though.

[jochen (gone - plz use gerrit), 2016-02-01 14:40:23]

lgtm

[Michael van Ouwerkerk, 2016-02-04 11:30:00]

Mike, could you take a look please for platform_notification_messages.h?

[Mike West, 2016-02-04 18:03:11]

Adding the icon data to the IPC LGTM.

[Michael van Ouwerkerk, 2016-02-08 18:26:12]

Description was changed from

==========
Notification actions may have an icon url.

This is behind a runtime flag for two reasons:
* The implementation is incomplete.
* We're still evaluating the API design.

BUG=581336
==========

to

==========
Notification actions may have an icon url.

This is behind a runtime flag for two reasons:
* The implementation is incomplete.
* We're still evaluating the API design.

Intent to Implement and Ship: Notification Action Icons
https://groups.google.com/a/chromium.org/d/msg/blink-dev/IM0HxOP7HOA/y8tu6iq1...

BUG=581336
==========

[Michael van Ouwerkerk, 2016-02-10 11:34:04]

The CQ bit was checked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2016-02-10 11:34:05]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
jochen@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1644573002/#ps120001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-02-10 11:34:15]

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

[commit-bot: I haz the power, 2016-02-10 13:03:48]

Description was changed from

==========
Notification actions may have an icon url.

This is behind a runtime flag for two reasons:
* The implementation is incomplete.
* We're still evaluating the API design.

Intent to Implement and Ship: Notification Action Icons
https://groups.google.com/a/chromium.org/d/msg/blink-dev/IM0HxOP7HOA/y8tu6iq1...

BUG=581336
==========

to

==========
Notification actions may have an icon url.

This is behind a runtime flag for two reasons:
* The implementation is incomplete.
* We're still evaluating the API design.

Intent to Implement and Ship: Notification Action Icons
https://groups.google.com/a/chromium.org/d/msg/blink-dev/IM0HxOP7HOA/y8tu6iq1...

BUG=581336
==========

[commit-bot: I haz the power, 2016-02-10 13:03:49]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-10 13:04:18]

Failed to apply the patch.

[commit-bot: I haz the power, 2016-02-10 13:05:10]

Description was changed from

==========
Notification actions may have an icon url.

This is behind a runtime flag for two reasons:
* The implementation is incomplete.
* We're still evaluating the API design.

Intent to Implement and Ship: Notification Action Icons
https://groups.google.com/a/chromium.org/d/msg/blink-dev/IM0HxOP7HOA/y8tu6iq1...

BUG=581336
==========

to

==========
Notification actions may have an icon url.

This is behind a runtime flag for two reasons:
* The implementation is incomplete.
* We're still evaluating the API design.

Intent to Implement and Ship: Notification Action Icons
https://groups.google.com/a/chromium.org/d/msg/blink-dev/IM0HxOP7HOA/y8tu6iq1...

BUG=581336

Committed: https://crrev.com/4a3e17c874bc4c4c90e5b0f8ec568520964695d4
Cr-Commit-Position: refs/heads/master@{#374649}
==========

[commit-bot: I haz the power, 2016-02-10 13:05:11]

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