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

Issue 1388483002: Implement the Notification `timestamp` property (Closed)

Created:
3 years, 2 months ago by Peter Beverloo
Modified:
2 years, 10 months ago
Reviewers:
johnme, dglazkov, nasko
CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, jam, mlamouri+watch-notifications_chromium.org, peter+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Implement the Notification `timestamp` property. This CL implements the new Notification.timestamp property that can be used by developers to indicate the time at which a notification is actual. For example, a notification for an upcoming meeting could be set in the future, whereas missed messages because the device was offline can be set in the past. On desktop this only affects ordering of the notification for now, whereas the notification's time is rendered on the toast on Android. https://notifications.spec.whatwg.org/#api This feature was approved to be implemented and to ship by the following Intent to Implement and Ship on blink-dev: https://groups.google.com/a/chromium.org/d/topic/blink-dev/gCvPWn0rb_c/discussion BUG=538720 Committed: https://crrev.com/b8afbbe4213c087653171cd2dd7a11c84c183a11 Cr-Commit-Position: refs/heads/master@{#372655}

Patch Set 1 #

Patch Set 2 : #

Total comments: 14

Patch Set 3 : rebase + layout tests #

Total comments: 4

Patch Set 4 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+136 lines, -3 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java View 1 2 2 chunks +9 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java View 1 2 3 chunks +3 lines, -1 line 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilder.java View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/notifications/notification_ui_manager_android.cc View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/notifications/platform_notification_service_browsertest.cc View 1 2 4 chunks +10 lines, -0 lines 0 comments Download
M chrome/browser/notifications/platform_notification_service_impl.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/test/data/notifications/platform_notification_service.html View 1 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/notifications/notification_database_data.proto View 1 2 2 chunks +2 lines, -1 line 0 comments Download
M content/browser/notifications/notification_database_data_conversions.cc View 1 2 3 chunks +4 lines, -0 lines 0 comments Download
M content/browser/notifications/notification_database_data_unittest.cc View 1 2 4 chunks +4 lines, -0 lines 0 comments Download
M content/child/notifications/notification_data_conversions.cc View 1 2 3 chunks +3 lines, -0 lines 0 comments Download
M content/child/notifications/notification_data_conversions_unittest.cc View 1 2 3 6 chunks +42 lines, -0 lines 0 comments Download
M content/common/platform_notification_messages.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/platform_notification_data.h View 1 2 2 chunks +4 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/notifications/notification-properties.html View 1 2 3 chunks +5 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-dedicated-worker-expected.txt View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-shared-worker-expected.txt View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-dedicated-worker-expected.txt View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-shared-worker-expected.txt View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/notifications/Notification.h View 1 2 2 chunks +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/notifications/Notification.cpp View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/notifications/Notification.idl View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/notifications/NotificationData.cpp View 1 2 2 chunks +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/notifications/NotificationDataTest.cpp View 1 2 5 chunks +17 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/notifications/NotificationOptions.idl View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/public/platform/modules/notifications/WebNotificationData.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 22 (9 generated)
Peter Beverloo
--> johnme, PTAL
3 years, 2 months ago (2015-10-20 15:59:22 UTC) #3
johnme
Generally looks good. Some nits about overflow etc. https://codereview.chromium.org/1388483002/diff/40001/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java (right): https://codereview.chromium.org/1388483002/diff/40001/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java#newcode171 chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java:171: assertTrue((System.currentTimeMillis() ...
3 years, 2 months ago (2015-10-20 17:12:21 UTC) #5
johnme
You probably also want to update LayoutTests/webexposed/global-interface-listing-expected.txt and friends?
3 years, 2 months ago (2015-10-20 18:32:25 UTC) #6
Peter Beverloo
PTAL https://codereview.chromium.org/1388483002/diff/40001/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java (right): https://codereview.chromium.org/1388483002/diff/40001/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java#newcode171 chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java:171: assertTrue((System.currentTimeMillis() - notification.when) < 30 * 1000); On ...
2 years, 10 months ago (2016-01-28 17:18:30 UTC) #8
johnme
Looks good, just one question. https://codereview.chromium.org/1388483002/diff/80001/content/child/notifications/notification_data_conversions_unittest.cc File content/child/notifications/notification_data_conversions_unittest.cc (right): https://codereview.chromium.org/1388483002/diff/80001/content/child/notifications/notification_data_conversions_unittest.cc#newcode175 content/child/notifications/notification_data_conversions_unittest.cc:175: EXPECT_NE(web_data.timestamp, copied_data.timestamp); Thinking about ...
2 years, 10 months ago (2016-01-28 18:16:57 UTC) #9
Peter Beverloo
https://codereview.chromium.org/1388483002/diff/80001/content/child/notifications/notification_data_conversions_unittest.cc File content/child/notifications/notification_data_conversions_unittest.cc (right): https://codereview.chromium.org/1388483002/diff/80001/content/child/notifications/notification_data_conversions_unittest.cc#newcode175 content/child/notifications/notification_data_conversions_unittest.cc:175: EXPECT_NE(web_data.timestamp, copied_data.timestamp); On 2016/01/28 18:16:57, johnme wrote: > Thinking ...
2 years, 10 months ago (2016-01-28 18:28:34 UTC) #10
johnme
lgtm https://codereview.chromium.org/1388483002/diff/80001/content/child/notifications/notification_data_conversions_unittest.cc File content/child/notifications/notification_data_conversions_unittest.cc (right): https://codereview.chromium.org/1388483002/diff/80001/content/child/notifications/notification_data_conversions_unittest.cc#newcode175 content/child/notifications/notification_data_conversions_unittest.cc:175: EXPECT_NE(web_data.timestamp, copied_data.timestamp); On 2016/01/28 18:28:34, Peter Beverloo wrote: ...
2 years, 10 months ago (2016-01-28 18:50:21 UTC) #11
Peter Beverloo
+dglazkov for LayoutTests/virtual/stable/ +nasko for content/public/common/platform_notification_data.h and platform_notification_messages.h https://codereview.chromium.org/1388483002/diff/80001/content/child/notifications/notification_data_conversions_unittest.cc File content/child/notifications/notification_data_conversions_unittest.cc (right): https://codereview.chromium.org/1388483002/diff/80001/content/child/notifications/notification_data_conversions_unittest.cc#newcode175 content/child/notifications/notification_data_conversions_unittest.cc:175: EXPECT_NE(web_data.timestamp, ...
2 years, 10 months ago (2016-01-28 20:52:07 UTC) #13
nasko
LGTM
2 years, 10 months ago (2016-01-28 23:51:30 UTC) #14
dglazkov
lgtm
2 years, 10 months ago (2016-01-28 23:54:56 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1388483002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1388483002/100001
2 years, 10 months ago (2016-02-01 10:11:49 UTC) #18
commit-bot: I haz the power
Committed patchset #4 (id:100001)
2 years, 10 months ago (2016-02-01 14:01:22 UTC) #20
commit-bot: I haz the power
2 years, 10 months ago (2016-02-01 14:02:48 UTC) #22
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/b8afbbe4213c087653171cd2dd7a11c84c183a11
Cr-Commit-Position: refs/heads/master@{#372655}

Powered by Google App Engine
This is Rietveld 408576698