Implement the Notification `timestamp` property

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}

[Peter Beverloo, 2015-10-05 10:05:48]

Patchset #2 (id:20001) has been deleted

[Peter Beverloo, 2015-10-20 15:59:22]

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

[Peter Beverloo, 2015-10-20 15:59:22]

--> johnme, PTAL

[Peter Beverloo, 2015-10-20 16:00:24]

Description was changed from

==========
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
 
BUG=538720
==========

to

==========
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/discus...
 
BUG=538720
==========

[johnme, 2015-10-20 17:12:21]

Generally looks good. Some nits about overflow etc.

https://codereview.chromium.org/1388483002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java
(right):

https://codereview.chromium.org/1388483002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java:171:
assertTrue((System.currentTimeMillis() - notification.when) < 30 * 1000);
Is it worth throwing in a Math.abs just in case the renderer's clock is running
fast (e.g. if the machine synced to a NTP server in between)?

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

https://codereview.chromium.org/1388483002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_ui_manager_android.cc:155:
notification.timestamp().ToJavaTime(),
FYI ToJavaTime can underflow for times in between 1601 and 1970, but I think
you're ok here since your base::Time is after 1970.

https://codereview.chromium.org/1388483002/diff/40001/content/browser/notific...
File content/browser/notifications/notification_database_data.proto (right):

https://codereview.chromium.org/1388483002/diff/40001/content/browser/notific...
content/browser/notifications/notification_database_data.proto:47: optional
double timestamp = 12;
Why not an int64, to match the base::Time in PlatformNotificationData? You can
use ToInternalValue/FromInternalValue to serialize/deserialize.

(the double you get from ToDoubleT is in microseconds, so it's not consistent
with the Blink one either)

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

https://codereview.chromium.org/1388483002/diff/40001/content/child/notificat...
content/child/notifications/notification_data_conversions.cc:44:
platform_data.timestamp = base::Time::FromJsTime(web_data.timestamp);
If JS passes MAX_ULL, it'll overflow base::Time, since:
a) base::Time uses an earlier epoch (1601)
b) base::Time is signed so has half the range
It looks like operator+ handles that sensibly, but it won't round-trip to the
same value, so please add a test.

https://codereview.chromium.org/1388483002/diff/40001/content/child/notificat...
content/child/notifications/notification_data_conversions.cc:80:
web_data.timestamp = platform_data.timestamp.ToJsTime();
I don't think underflow is possible, and I think zero will round-trip correctly,
but perhaps add a test for the latter anyway?

https://codereview.chromium.org/1388483002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/NotificationOptions.idl
(right):

https://codereview.chromium.org/1388483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationOptions.idl:21:
unsigned long long timestamp;
I realise this matches the spec, but I notice that Date.now() is an ECMAScript
Number[1], and performance.now() is a DOMHighResTimeStamp == double[2] (though
that's monotonic time ticks, which is somewhat different). There's also
https://heycam.github.io/webidl/#idl-Date. Is ULL definitely the right type
here? Seems weird that you can't represent times before 1970 (unlike Java signed
long dates, which can go both ways), even though that's not very relevant for
this particular feature.

If ULL is correct, please use the
https://heycam.github.io/webidl/#common-DOMTimeStamp typedef (and ditto in the
spec).

[1]:
http://www.ecma-international.org/ecma-262/6.0/#sec-time-values-and-time-range
[2]: http://www.w3.org/TR/hr-time/#sec-extenstions-performance-interface

https://codereview.chromium.org/1388483002/diff/40001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/notifications/WebNotificationData.h
(right):

https://codereview.chromium.org/1388483002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/notifications/WebNotificationData.h:30:
double timestamp = 0.;
Micro-nit: no '.'
https://www.chromium.org/blink/coding-style#TOC-Floating-point-literals

[johnme, 2015-10-20 18:32:25]

You probably also want to update
LayoutTests/webexposed/global-interface-listing-expected.txt and friends?

[Peter Beverloo, 2016-01-28 17:17:46]

Patchset #3 (id:60001) has been deleted

[Peter Beverloo, 2016-01-28 17:18:30]

PTAL

https://codereview.chromium.org/1388483002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java
(right):

https://codereview.chromium.org/1388483002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationUIManagerTest.java:171:
assertTrue((System.currentTimeMillis() - notification.when) < 30 * 1000);
On 2015/10/20 17:12:20, johnme wrote:
> Is it worth throwing in a Math.abs just in case the renderer's clock is
running
> fast (e.g. if the machine synced to a NTP server in between)?

Done.

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

https://codereview.chromium.org/1388483002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_ui_manager_android.cc:155:
notification.timestamp().ToJavaTime(),
On 2015/10/20 17:12:21, johnme wrote:
> FYI ToJavaTime can underflow for times in between 1601 and 1970, but I think
> you're ok here since your base::Time is after 1970.

Acknowledged.

https://codereview.chromium.org/1388483002/diff/40001/content/browser/notific...
File content/browser/notifications/notification_database_data.proto (right):

https://codereview.chromium.org/1388483002/diff/40001/content/browser/notific...
content/browser/notifications/notification_database_data.proto:47: optional
double timestamp = 12;
On 2015/10/20 17:12:21, johnme wrote:
> Why not an int64, to match the base::Time in PlatformNotificationData? You can
> use ToInternalValue/FromInternalValue to serialize/deserialize.
> 
> (the double you get from ToDoubleT is in microseconds, so it's not consistent
> with the Blink one either)

Done.

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

https://codereview.chromium.org/1388483002/diff/40001/content/child/notificat...
content/child/notifications/notification_data_conversions.cc:44:
platform_data.timestamp = base::Time::FromJsTime(web_data.timestamp);
On 2015/10/20 17:12:21, johnme wrote:
> If JS passes MAX_ULL, it'll overflow base::Time, since:
> a) base::Time uses an earlier epoch (1601)
> b) base::Time is signed so has half the range
> It looks like operator+ handles that sensibly, but it won't round-trip to the
> same value, so please add a test.

Done.

https://codereview.chromium.org/1388483002/diff/40001/content/child/notificat...
content/child/notifications/notification_data_conversions.cc:80:
web_data.timestamp = platform_data.timestamp.ToJsTime();
On 2015/10/20 17:12:21, johnme wrote:
> I don't think underflow is possible, and I think zero will round-trip
correctly,
> but perhaps add a test for the latter anyway?

Done.

https://codereview.chromium.org/1388483002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/NotificationOptions.idl
(right):

https://codereview.chromium.org/1388483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationOptions.idl:21:
unsigned long long timestamp;
Changed to DOMTimeStamp.

https://codereview.chromium.org/1388483002/diff/40001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/notifications/WebNotificationData.h
(right):

https://codereview.chromium.org/1388483002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/notifications/WebNotificationData.h:30:
double timestamp = 0.;
On 2015/10/20 17:12:21, johnme wrote:
> Micro-nit: no '.'
> https://www.chromium.org/blink/coding-style#TOC-Floating-point-literals

Done.

[johnme, 2016-01-28 18:16:57]

Looks good, just one question.

https://codereview.chromium.org/1388483002/diff/80001/content/child/notificat...
File content/child/notifications/notification_data_conversions_unittest.cc
(right):

https://codereview.chromium.org/1388483002/diff/80001/content/child/notificat...
content/child/notifications/notification_data_conversions_unittest.cc:175:
EXPECT_NE(web_data.timestamp, copied_data.timestamp);
Thinking about this more, is it really ok that this doesn't round-trip? Whilst
there isn't a strong use case for timestamps in the distant future, according to
the spec they should still roundtrip.

[Peter Beverloo, 2016-01-28 18:28:34]

https://codereview.chromium.org/1388483002/diff/80001/content/child/notificat...
File content/child/notifications/notification_data_conversions_unittest.cc
(right):

https://codereview.chromium.org/1388483002/diff/80001/content/child/notificat...
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 about this more, is it really ok that this doesn't round-trip? Whilst
> there isn't a strong use case for timestamps in the distant future, according
to
> the spec they should still roundtrip.

Our implementation will support timestamps between 1970 (as defined by
JavaScript) and some point in year 294441. We'll round-trip properly until
approximately 294071.

This honestly does not concern me, would you agree?

[johnme, 2016-01-28 18:50:21]

lgtm

https://codereview.chromium.org/1388483002/diff/80001/content/child/notificat...
File content/child/notifications/notification_data_conversions_unittest.cc
(right):

https://codereview.chromium.org/1388483002/diff/80001/content/child/notificat...
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:
> On 2016/01/28 18:16:57, johnme wrote:
> > Thinking about this more, is it really ok that this doesn't round-trip?
Whilst
> > there isn't a strong use case for timestamps in the distant future,
according
> to
> > the spec they should still roundtrip.
> 
> Our implementation will support timestamps between 1970 (as defined by
> JavaScript) and some point in year 294441. We'll round-trip properly until
> approximately 294071.
> 
> This honestly does not concern me, would you agree?

Some websites might pass MAX_ULL as a hack to make sure their notification
appears first (I'm not saying we should condone that, but it's not unlikely).

But I guess this is fine. Can you add a test that Jan 1st 294071 round-trips
properly?

[Peter Beverloo, 2016-01-28 20:52:06]

peter@chromium.org changed reviewers:
+ dglazkov@chromium.org, nasko@chromium.org

[Peter Beverloo, 2016-01-28 20:52:07]

+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/notificat...
File content/child/notifications/notification_data_conversions_unittest.cc
(right):

https://codereview.chromium.org/1388483002/diff/80001/content/child/notificat...
content/child/notifications/notification_data_conversions_unittest.cc:175:
EXPECT_NE(web_data.timestamp, copied_data.timestamp);
On 2016/01/28 18:50:21, johnme wrote:
> On 2016/01/28 18:28:34, Peter Beverloo wrote:
> > On 2016/01/28 18:16:57, johnme wrote:
> > > Thinking about this more, is it really ok that this doesn't round-trip?
> Whilst
> > > there isn't a strong use case for timestamps in the distant future,
> according
> > to
> > > the spec they should still roundtrip.
> > 
> > Our implementation will support timestamps between 1970 (as defined by
> > JavaScript) and some point in year 294441. We'll round-trip properly until
> > approximately 294071.
> > 
> > This honestly does not concern me, would you agree?
> 
> Some websites might pass MAX_ULL as a hack to make sure their notification
> appears first (I'm not saying we should condone that, but it's not unlikely).
> 
> But I guess this is fine. Can you add a test that Jan 1st 294071 round-trips
> properly?

Done.

[nasko, 2016-01-28 23:51:30]

LGTM

[dglazkov, 2016-01-28 23:54:56]

lgtm

[Peter Beverloo, 2016-02-01 10:11:12]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2016-02-01 10:11:12]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org
Link to the patchset: https://codereview.chromium.org/1388483002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2016-02-01 10:11:49]

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

[commit-bot: I haz the power, 2016-02-01 14:01:21]

Description was changed from

==========
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/discus...
 
BUG=538720
==========

to

==========
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/discus...
 
BUG=538720
==========

[commit-bot: I haz the power, 2016-02-01 14:01:22]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-02-01 14:02:47]

Description was changed from

==========
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/discus...
 
BUG=538720
==========

to

==========
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/discus...

BUG=538720

Committed: https://crrev.com/b8afbbe4213c087653171cd2dd7a11c84c183a11
Cr-Commit-Position: refs/heads/master@{#372655}
==========

[commit-bot: I haz the power, 2016-02-01 14:02:48]

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