Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 993893002: Add the data attribute to the Notification object (Closed)

Created:
2 years, 8 months ago by Sanghyun Park
Modified:
2 years, 8 months ago
CC:
vivekg, arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, dglazkov+blink, vivekg_samsung
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Add the data attribute to the Notification object When creating a Notification from JavaScript, the specification defines a "data" attribute that authors can use to store arbitrary, serializable content in. The Web Notification specification: https://notifications.spec.whatwg.org/#dom-notification-data BUG=442129 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191882

Patch Set 1 : [WIP] modified wrong comment. #

Total comments: 31

Patch Set 2 : #

Total comments: 21

Patch Set 3 : #

Total comments: 18

Patch Set 4 : #

Total comments: 6

Patch Set 5 : #

Patch Set 6 : Fix layout test failures. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+85 lines, -1 line) Patch
A LayoutTests/http/tests/notifications/notification-data-property.html View 1 2 3 4 1 chunk +46 lines, -0 lines 0 comments Download
M LayoutTests/http/tests/notifications/notification-properties.html View 1 2 3 4 3 chunks +4 lines, -1 line 0 comments Download
M LayoutTests/webexposed/global-interface-listing-dedicated-worker-expected.txt View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/webexposed/global-interface-listing-expected.txt View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/webexposed/global-interface-listing-shared-worker-expected.txt View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M Source/modules/notifications/Notification.h View 1 2 3 4 5 chunks +9 lines, -0 lines 0 comments Download
M Source/modules/notifications/Notification.cpp View 1 2 3 4 4 chunks +20 lines, -0 lines 0 comments Download
M Source/modules/notifications/Notification.idl View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M Source/modules/notifications/NotificationOptions.idl View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/platform/RuntimeEnabledFeatures.in View 1 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 53 (13 generated)
Sanghyun Park
2 years, 8 months ago (2015-03-10 09:01:18 UTC) #3
Peter Beverloo
Thank you for the patch!! Please see my attached comments. Actually supporting this for persistent ...
2 years, 8 months ago (2015-03-10 23:34:27 UTC) #4
Sanghyun Park
Wow. Thank you for kind and detail review. :) I'll upload fixed version. https://codereview.chromium.org/993893002/diff/20001/LayoutTests/http/tests/notifications/notification-properties.html File ...
2 years, 8 months ago (2015-03-11 10:59:52 UTC) #5
Sanghyun Park
2 years, 8 months ago (2015-03-11 15:04:23 UTC) #8
Peter Beverloo
+haraken Thank you, that looks much better already! I've got mostly nits, and one larger ...
2 years, 8 months ago (2015-03-12 01:16:27 UTC) #10
haraken
> +haraken > > Thank you, that looks much better already! I've got mostly nits, ...
2 years, 8 months ago (2015-03-12 01:22:47 UTC) #12
Peter Beverloo
On 2015/03/12 01:22:47, haraken wrote: > > I see that the bindings do have an ...
2 years, 8 months ago (2015-03-12 01:43:52 UTC) #13
yhirano
On 2015/03/12 01:22:47, haraken wrote: > > +haraken > > > > Thank you, that ...
2 years, 8 months ago (2015-03-12 02:16:31 UTC) #14
haraken
On 2015/03/12 02:16:31, yhirano wrote: > On 2015/03/12 01:22:47, haraken wrote: > > > +haraken ...
2 years, 8 months ago (2015-03-12 02:22:18 UTC) #15
Peter Beverloo
Thank you for your input!! On 2015/03/12 02:22:18, haraken wrote: > On 2015/03/12 02:16:31, yhirano ...
2 years, 8 months ago (2015-03-12 02:28:50 UTC) #16
haraken
On 2015/03/12 02:28:50, Peter Beverloo wrote: > Thank you for your input!! > > On ...
2 years, 8 months ago (2015-03-12 02:32:25 UTC) #17
Sanghyun Park
Thank you for review :) https://codereview.chromium.org/993893002/diff/80001/LayoutTests/http/tests/notifications/notification-data-property.html File LayoutTests/http/tests/notifications/notification-data-property.html (right): https://codereview.chromium.org/993893002/diff/80001/LayoutTests/http/tests/notifications/notification-data-property.html#newcode19 LayoutTests/http/tests/notifications/notification-data-property.html:19: if (value.constructor.toString().indexOf("Object") > -1) ...
2 years, 8 months ago (2015-03-12 02:58:47 UTC) #18
Sanghyun Park
On 2015/03/12 02:32:25, haraken wrote: > On 2015/03/12 02:28:50, Peter Beverloo wrote: > > Thank ...
2 years, 8 months ago (2015-03-12 05:03:40 UTC) #19
haraken
On 2015/03/12 05:03:40, sh919.park wrote: > On 2015/03/12 02:32:25, haraken wrote: > > On 2015/03/12 ...
2 years, 8 months ago (2015-03-12 05:07:18 UTC) #20
Sanghyun Park
On 2015/03/12 05:07:18, haraken wrote: > On 2015/03/12 05:03:40, sh919.park wrote: > > On 2015/03/12 ...
2 years, 8 months ago (2015-03-12 05:19:39 UTC) #21
haraken
On 2015/03/12 05:19:39, sh919.park wrote: > On 2015/03/12 05:07:18, haraken wrote: > > On 2015/03/12 ...
2 years, 8 months ago (2015-03-12 06:03:29 UTC) #22
Sanghyun Park
On 2015/03/12 06:03:29, haraken wrote: > On 2015/03/12 05:19:39, sh919.park wrote: > > On 2015/03/12 ...
2 years, 8 months ago (2015-03-12 06:36:37 UTC) #23
Jens Widell
On 2015/03/12 01:22:47, haraken wrote: > Yeah, [RaisesException] might not be a good name. We ...
2 years, 8 months ago (2015-03-12 08:43:44 UTC) #24
haraken
On 2015/03/12 06:36:37, sh919.park wrote: > On 2015/03/12 06:03:29, haraken wrote: > > On 2015/03/12 ...
2 years, 8 months ago (2015-03-12 08:50:03 UTC) #25
haraken
On 2015/03/12 08:43:44, Jens Widell wrote: > On 2015/03/12 01:22:47, haraken wrote: > > Yeah, ...
2 years, 8 months ago (2015-03-12 08:50:52 UTC) #26
Sanghyun Park
On 2015/03/12 08:50:03, haraken wrote: > On 2015/03/12 06:36:37, sh919.park wrote: > > On 2015/03/12 ...
2 years, 8 months ago (2015-03-12 09:04:20 UTC) #27
Jens Widell
On 2015/03/12 08:50:52, haraken wrote: > On 2015/03/12 08:43:44, Jens Widell wrote: > > On ...
2 years, 8 months ago (2015-03-12 09:10:50 UTC) #28
Sanghyun Park
https://codereview.chromium.org/993893002/diff/80001/Source/modules/notifications/Notification.cpp File Source/modules/notifications/Notification.cpp (right): https://codereview.chromium.org/993893002/diff/80001/Source/modules/notifications/Notification.cpp#newcode169 Source/modules/notifications/Notification.cpp:169: WebNotificationData notificationData(m_title, dir, m_lang, m_body, m_tag, m_iconUrl, m_silent, m_data.get()); ...
2 years, 8 months ago (2015-03-12 11:52:58 UTC) #29
Sanghyun Park
https://codereview.chromium.org/993893002/diff/80001/Source/modules/notifications/Notification.cpp File Source/modules/notifications/Notification.cpp (right): https://codereview.chromium.org/993893002/diff/80001/Source/modules/notifications/Notification.cpp#newcode169 Source/modules/notifications/Notification.cpp:169: WebNotificationData notificationData(m_title, dir, m_lang, m_body, m_tag, m_iconUrl, m_silent, m_data.get()); ...
2 years, 8 months ago (2015-03-12 12:39:36 UTC) #31
Peter Beverloo
Mostly lg, but we have a layering violation we need to deal with first. You ...
2 years, 8 months ago (2015-03-12 20:30:46 UTC) #32
Sanghyun Park
Thank you for review. I have one doubt about layering (WebNotificationData.h). Please refer my comment ...
2 years, 8 months ago (2015-03-13 02:57:07 UTC) #33
Sanghyun Park
On 2015/03/12 20:30:46, Peter Beverloo wrote: > Mostly lg, but we have a layering violation ...
2 years, 8 months ago (2015-03-13 04:13:05 UTC) #34
vivekg
https://codereview.chromium.org/993893002/diff/120001/LayoutTests/http/tests/notifications/notification-data-property.html File LayoutTests/http/tests/notifications/notification-data-property.html (right): https://codereview.chromium.org/993893002/diff/120001/LayoutTests/http/tests/notifications/notification-data-property.html#newcode1 LayoutTests/http/tests/notifications/notification-data-property.html:1: <!doctype html> nit: We prefer <!DOCTYPE html> https://codereview.chromium.org/993893002/diff/120001/LayoutTests/http/tests/notifications/notification-data-property.html#newcode21 LayoutTests/http/tests/notifications/notification-data-property.html:21: ...
2 years, 8 months ago (2015-03-13 04:21:49 UTC) #36
Sanghyun Park
Dear Vivekg, Thanks for review. https://codereview.chromium.org/993893002/diff/120001/LayoutTests/http/tests/notifications/notification-data-property.html File LayoutTests/http/tests/notifications/notification-data-property.html (right): https://codereview.chromium.org/993893002/diff/120001/LayoutTests/http/tests/notifications/notification-data-property.html#newcode1 LayoutTests/http/tests/notifications/notification-data-property.html:1: <!doctype html> On 2015/03/13 ...
2 years, 8 months ago (2015-03-13 05:03:08 UTC) #37
vivekg
https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifications/Notification.h File Source/modules/notifications/Notification.h (right): https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifications/Notification.h#newcode34 Source/modules/notifications/Notification.h:34: #include "bindings/core/v8/SerializedScriptValue.h" On 2015/03/13 05:03:08, sh919.park wrote: > On ...
2 years, 8 months ago (2015-03-13 07:13:15 UTC) #38
Sanghyun Park
On 2015/03/13 07:13:15, vivekg_ wrote: > https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifications/Notification.h > File Source/modules/notifications/Notification.h (right): > > https://codereview.chromium.org/993893002/diff/120001/Source/modules/notifications/Notification.h#newcode34 > ...
2 years, 8 months ago (2015-03-13 07:40:08 UTC) #39
Sanghyun Park
https://codereview.chromium.org/993893002/diff/120001/public/platform/modules/notifications/WebNotificationData.h File public/platform/modules/notifications/WebNotificationData.h (right): https://codereview.chromium.org/993893002/diff/120001/public/platform/modules/notifications/WebNotificationData.h#newcode10 public/platform/modules/notifications/WebNotificationData.h:10: #include "public/web/WebSerializedScriptValue.h" On 2015/03/13 02:57:07, sh919.park wrote: > On ...
2 years, 8 months ago (2015-03-13 07:58:08 UTC) #40
Peter Beverloo
notifications lgtm, thank you!! You'll need another OWNER to approve RuntimeEnabledFeatures.in. https://codereview.chromium.org/993893002/diff/140001/LayoutTests/http/tests/notifications/notification-data-property.html File LayoutTests/http/tests/notifications/notification-data-property.html (right): ...
2 years, 8 months ago (2015-03-13 12:12:07 UTC) #41
haraken
LGTM
2 years, 8 months ago (2015-03-13 12:36:34 UTC) #42
Sanghyun Park
Thank you so much. I cannot complete this patch without your help. :) I'll upload ...
2 years, 8 months ago (2015-03-13 17:47:31 UTC) #43
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/993893002/160001
2 years, 8 months ago (2015-03-13 18:57:51 UTC) #46
Sanghyun Park
Dear Peter. Layouttest had fails. So I fix about these. Could you review about this? ...
2 years, 8 months ago (2015-03-14 03:22:37 UTC) #48
Peter Beverloo
On 2015/03/14 03:22:37, Sanghyun Park wrote: > Dear Peter. > > Layouttest had fails. > ...
2 years, 8 months ago (2015-03-14 15:20:21 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/993893002/180001
2 years, 8 months ago (2015-03-14 15:20:43 UTC) #52
commit-bot: I haz the power
2 years, 8 months ago (2015-03-14 16:45:06 UTC) #53
Message was sent while issue was closed.
Committed patchset #6 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191882

Powered by Google App Engine
This is Rietveld efc10ee0f