New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Notification.timestamp and NotificationOptions.timestamp #54

Merged
merged 1 commit into from Oct 14, 2015

Conversation

3 participants
@beverloo
Member

beverloo commented Oct 2, 2015

Fixes #20.

The timestamp of a notification is the time, in milliseconds since
the epoch, of the event for which the notification was created. Web
developers can use this if the time of this event does not match the
time at which the notification is being shown, which can be the case
when notifying the user of upcoming calendar events.

Note that the Boilerplate change in the meta-data is Tab's
suggestion in tabatkins/bikeshed#470.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Oct 2, 2015

Member

@sicking, could you review?

Member

annevk commented Oct 2, 2015

@sicking, could you review?

@sicking

This comment has been minimized.

Show comment
Hide comment
@sicking

sicking Oct 2, 2015

Just two things:

I think we should have an informative not that says that the timestamp can both be in the future, for example for notifications about a meeting starting in 10 minutes, or in the past, when the phone regains internet connectivity and a messaging app detects that a message was sent to the user an hour ago.

I think we should also default the timestamp to the current time if no timestamp is provided?

sicking commented Oct 2, 2015

Just two things:

I think we should have an informative not that says that the timestamp can both be in the future, for example for notifications about a meeting starting in 10 minutes, or in the past, when the phone regains internet connectivity and a messaging app detects that a message was sent to the user an hour ago.

I think we should also default the timestamp to the current time if no timestamp is provided?

@beverloo

This comment has been minimized.

Show comment
Hide comment
@beverloo

beverloo Oct 2, 2015

Member

I have added a

in the list of definitions. Does this describe the intention well enough?

The timestamp member in the NotificationOptions dictionary has no default value in the IDL. The behavior is explained in the "create a notification" steps:

If options’s timestamp is present, set notification’s timestamp to the value. Otherwise,
set notification’s timestamp to the number of milliseconds that passed since 00:00:00
UTC on 1 January 1970.

Member

beverloo commented Oct 2, 2015

I have added a

in the list of definitions. Does this describe the intention well enough?

The timestamp member in the NotificationOptions dictionary has no default value in the IDL. The behavior is explained in the "create a notification" steps:

If options’s timestamp is present, set notification’s timestamp to the value. Otherwise,
set notification’s timestamp to the number of milliseconds that passed since 00:00:00
UTC on 1 January 1970.

@beverloo

This comment has been minimized.

Show comment
Hide comment
@beverloo

beverloo Oct 9, 2015

Member

@sicking Friendly ping :) WDYT?

Member

beverloo commented Oct 9, 2015

@sicking Friendly ping :) WDYT?

@sicking

This comment has been minimized.

Show comment
Hide comment
@sicking

sicking Oct 13, 2015

Sorry, didn't realize this was waiting on me. Looks good to me. Only minor thing might be to clarify the point in time at which the number of milliseconds is measured. I.e. something like:

Otherwise, set notification’s timestamp to the number of milliseconds that passed between 00:00:00
UTC on 1 January 1970 and Notification constructor was called.

But I'm really fine either way.

sicking commented Oct 13, 2015

Sorry, didn't realize this was waiting on me. Looks good to me. Only minor thing might be to clarify the point in time at which the number of milliseconds is measured. I.e. something like:

Otherwise, set notification’s timestamp to the number of milliseconds that passed between 00:00:00
UTC on 1 January 1970 and Notification constructor was called.

But I'm really fine either way.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Oct 14, 2015

Member

One nit I have is to remove the trailing dot from the first line of the commit message. We're treating that as a title of sorts these days and those don't have dots.

Member

annevk commented Oct 14, 2015

One nit I have is to remove the trailing dot from the first line of the commit message. We're treating that as a title of sorts these days and those don't have dots.

Add Notification.timestamp and NotificationOptions.timestamp
Fixes #20.

The `timestamp` of a notification is the time, in milliseconds since
the epoch, of the event for which the notification was created. Web
developers can use this if the time of this event does not match the
time at which the notification is being shown, which can be the case
when notifying the user of upcoming calendar events.
@beverloo

This comment has been minimized.

Show comment
Hide comment
@beverloo

beverloo Oct 14, 2015

Member

Both done. Thank you!

Member

beverloo commented Oct 14, 2015

Both done. Thank you!

@beverloo beverloo changed the title from Add Notification.timestamp and NotificationOptions.timestamp. to Add Notification.timestamp and NotificationOptions.timestamp Oct 14, 2015

@annevk annevk merged commit f72a041 into whatwg:master Oct 14, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment