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

Issue 1099093003: Push API: Forced notifications should use Notifications database (Closed)

Created:
2 years, 7 months ago by johnme
Modified:
2 years, 6 months ago
CC:
chromium-reviews, mvanouwerkerk+watch_chromium.org, peter+watch_chromium.org, johnme+watch_chromium.org, Miguel Garcia
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Push API: Forced notifications should use Notifications database 1. Writes to the Notifications database when creating forced notifications. 2. Uses Notifications database when counting notifications, instead of the unreliable NotificationUIManager APIs. This gives more reliable values after browser restarts. BUG=437277 TBR=avi@chromium.org Committed: https://crrev.com/830ecef7ea5e4fb162fb7cfdc6be2cbe4fd1d52b Cr-Commit-Position: refs/heads/master@{#327716}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Fix threading #

Patch Set 3 : Also write forced notifications to database #

Patch Set 4 : Don't use BindToCurrentLoop yet #

Total comments: 10

Patch Set 5 : Address Peter's review nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+160 lines, -33 lines) Patch
M chrome/browser/push_messaging/push_messaging_service_impl.h View 1 2 3 4 5 chunks +43 lines, -5 lines 0 comments Download
M chrome/browser/push_messaging/push_messaging_service_impl.cc View 1 2 3 4 6 chunks +108 lines, -20 lines 0 comments Download
M content/browser/notifications/platform_notification_context_impl.h View 1 2 chunks +1 line, -7 lines 0 comments Download
M content/public/browser/platform_notification_context.h View 1 3 chunks +8 lines, -1 line 0 comments Download

Messages

Total messages: 18 (6 generated)
johnme
2 years, 7 months ago (2015-04-21 17:26:39 UTC) #2
Peter Beverloo
Should this affect tests? https://codereview.chromium.org/1099093003/diff/1/chrome/browser/push_messaging/push_messaging_service_impl.cc File chrome/browser/push_messaging/push_messaging_service_impl.cc (right): https://codereview.chromium.org/1099093003/diff/1/chrome/browser/push_messaging/push_messaging_service_impl.cc#newcode282 chrome/browser/push_messaging/push_messaging_service_impl.cc:282: notification_context->ReadAllNotificationDataForServiceWorkerRegistration( We're on the UI ...
2 years, 7 months ago (2015-04-21 18:06:17 UTC) #3
Peter Beverloo
On further thought, could you please consider factoring the forced notification code out of this ...
2 years, 7 months ago (2015-04-22 17:53:51 UTC) #4
johnme
> On further thought, could you please consider factoring the forced notification > code out ...
2 years, 6 months ago (2015-04-23 16:51:17 UTC) #6
johnme
On 2015/04/23 16:51:17, johnme wrote: > > On further thought, could you please consider factoring ...
2 years, 6 months ago (2015-04-29 21:02:09 UTC) #8
Peter Beverloo
lgtm % a few nits. Please update the CLs description, since at least the dependency ...
2 years, 6 months ago (2015-04-30 11:11:42 UTC) #9
johnme
> Please update the CLs description, since at least the dependency mentions are not relevant ...
2 years, 6 months ago (2015-04-30 13:30:13 UTC) #10
johnme
TBR=avi since he already l-g-t-m'd this content/ change in https://codereview.chromium.org/1071773003/ - I'm just bringing it ...
2 years, 6 months ago (2015-04-30 13:32:41 UTC) #11
johnme
2 years, 6 months ago (2015-04-30 13:33:06 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1099093003/80001
2 years, 6 months ago (2015-04-30 13:33:32 UTC) #16
commit-bot: I haz the power
Committed patchset #5 (id:80001)
2 years, 6 months ago (2015-04-30 15:23:03 UTC) #17
commit-bot: I haz the power
2 years, 6 months ago (2015-04-30 15:24:00 UTC) #18
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/830ecef7ea5e4fb162fb7cfdc6be2cbe4fd1d52b
Cr-Commit-Position: refs/heads/master@{#327716}

Powered by Google App Engine
This is Rietveld efc10ee0f