New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 768695 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 769400



Sign in to add a comment

Invalid timestamp is set for "This site has been updated in the background" notification.

Project Member Reported by tetsui@chromium.org, Sep 26 2017

Issue description

Chrome Version: 63.0.3225.0
OS: Chrome OS

What steps will reproduce the problem?
(1) Enable push notification in a website.
(2) Wait for notification to arrive.

What is the expected result?
The notification header should be "now".

What happens instead?
The notification header says "418y" (418 years ago)
 
Screenshot 2017-09-26 at 1.16.01 PM.png
21.2 KB View Download

Comment 1 by peter@chromium.org, Sep 26 2017

That's because it defaults to zero. We should add the following to:

https://cs.chromium.org/chromium/src/chrome/browser/push_messaging/push_messaging_notification_manager.cc?q=push_messaging_notification_manager&sq=package:chromium&dr=C&l=69

notification_data.timestamp = base::Time::Now();


I'm heading to lunch now - happy to write the fix or review yours if you beat me to it.

Comment 2 by tetsui@chromium.org, Sep 26 2017

Status: Started (was: Assigned)
Thank you! I wrote the CL https://crrev.com/c/683914 (just pasted your fix :)
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f1b84062d58fd80832fccfb828b2d5c25b6b7388

commit f1b84062d58fd80832fccfb828b2d5c25b6b7388
Author: Tetsui Ohkubo <tetsui@chromium.org>
Date: Tue Sep 26 06:08:16 2017

Set timestamp to push messaging default notification.

This CL sets current timestamp to the default notification of push
messaging.
Previously, invalid time was shown when new style notification
chrome://flags/#enable-message-center-new-style-notification
is Enabled (which is ToT default).

TEST=manual
BUG= 768695 

Change-Id: Id253c990d39a8f8df71472c47137b8252a9186d4
Reviewed-on: https://chromium-review.googlesource.com/683914
Commit-Queue: Tetsui Ohkubo <tetsui@chromium.org>
Commit-Queue: Peter Beverloo <peter@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504296}
[modify] https://crrev.com/f1b84062d58fd80832fccfb828b2d5c25b6b7388/chrome/browser/push_messaging/push_messaging_notification_manager.cc

Comment 4 by tetsui@chromium.org, Sep 26 2017

Status: Fixed (was: Started)

Comment 5 by tetsui@chromium.org, Sep 28 2017

Blocking: 769400

Sign in to add a comment