Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1861683002:
Background service worker push message processing (Closed)
|
Created:
4 years, 8 months ago by harkness Modified:
4 years, 8 months ago CC:
chromium-reviews, Peter Beverloo, johnme+watch_chromium.org, jam, mvanouwerkerk+watch_chromium.org, darin-cc_chromium.org, harkness+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBackground service worker push message processing
This is the first part of a many-part project to add a framework for
service workers to perform background processing on behalf of the user
in response to a push message but without always showing a visible
notification to the user.
In the inital implementation, chrome's 1 in 10 grace period will be
replaced with a heuristic based budget derrived from the Site Engagement
Score.
This patch begins the road to that goal. We have created a new class,
the BackgroundBudgetService which lives in chrome/browser/push_messaging.
The implementation of the class is just to provide the grace period
tracking that was previously provided in
content/public/browser/push_messaging_service.cc. The backing storage has
also been changed to the user prefs instead of the service_worker_context
storage.
The next stage of implementation will replace the grace period tracking
with budget tracking and tie into the SiteEngagementService.
BUG=576837, 602244
Committed: https://crrev.com/373eb22ed62aaacd9d9e77c3843601eb88de7bf7
Cr-Commit-Position: refs/heads/master@{#386683}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Switched the key to origin and added tests. #
Total comments: 30
Patch Set 3 : Integrated code review comments #
Total comments: 8
Patch Set 4 : Made service static and synchronous #
Total comments: 4
Patch Set 5 : Cleaned up unit test #Patch Set 6 : Removed RunLoop #
Total comments: 12
Patch Set 7 : Integrate code review comments #
Total comments: 1
Patch Set 8 : dcheck ordering #Messages
Total messages: 41 (15 generated)
harkness@chromium.org changed reviewers: + peter@chromium.org
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1861683002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1861683002/1
https://codereview.chromium.org/1861683002/diff/1/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/1861683002/diff/1/chrome/common/pref_names.cc... chrome/common/pref_names.cc:1204: // Maps from service worker id to background budget information. I think service worker registration ids are unique per storage partition, not per profile. But origin+separator+sw id would be.
https://codereview.chromium.org/1861683002/diff/1/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/1861683002/diff/1/chrome/common/pref_names.cc... chrome/common/pref_names.cc:1204: // Maps from service worker id to background budget information. On 2016/04/05 14:05:52, Michael van Ouwerkerk wrote: > I think service worker registration ids are unique per storage partition, not > per profile. But origin+separator+sw id would be. Discussed with Michael in person. I'm going to switch the key to origin only.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Switched the key for the prefs info over to origin. Also added some unit tests.
Updated for real now.
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1861683002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1861683002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Cool, great to see the progress here :-).
Because this CL already moves storage of the notification history bitset from
per-Service Worker to per-origin, this fixes Issue 576837.
A few higher-level comments first:
- KeyedServices have associated KeyedServiceFactories. I'm fairly sure we
want this to be a KeyedService(+Factory), but in this CL it makes no sense
because (1) there is no factory, (2) nothing instantiates it, and (3) as
a consequence of those the KeyedService contract is voided.
- Taking one step further— why are the methods in the BBS static? I think
that you're basing this off the SES, but I don't understand that either :)
And then some higher-level nits:
- Please cap the CL description to ~74 characters. That way it reads
well in all the tools, including local `git log`.
- This really needs to have an associated bug. You probably also want to
include Issue 576837.
https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/background_budget_service.cc (right):
https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/background_budget_service.cc:15: namespace chrome
{
Please don't put things in the chrome namespace.
https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/background_budget_service.cc:19: std::string
GetBudgetStringForServiceWorker(Profile* profile,
nit (1): s/ForServiceWorker/ForOrigin/
nit (2): All callers store this in |*_value|, method says |*_string|.
Consistency++? :)
https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/background_budget_service.cc:25:
map->GetStringWithoutPathExpansion(origin.spec(), &map_value);
What happens when |origin| is not an origin, but a full URL? :-)
I would suggest adding the following DCHECK:
DCHECK_EQ(origin.GetOrigin(), origin);
https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/background_budget_service.cc:28: } // namespace
micro nit: blank line above this one
https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/background_budget_service.cc:41: // service.
micro nit: This comment doesn't add much value. Would be better when phrased as
a TODO.
https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/background_budget_service.cc:45:
callback.Run(map_value);
Do you expect this to be asynchronous in the future?
If it will be, I would recommend posting a task rather than invoking
immediately. That avoids writing test code that inadvertently relies on
synchronous behaviour.
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::Bind(callback, map_value));
https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/background_budget_service.h (right):
https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/background_budget_service.h:25: // implementation
it is just using the old grace period logic.
Comment nits:
- s/chrome/Chrome/.
- "... will be ..., ... initial implementation ..." - the comment should
reflect what the code does *today*. Refer to a BUG in a TODO for functionality
that is yet to be added.
https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/background_budget_service.h:26: // TODO(harkness)
do I need a CHROME_EXPORT?
No.
The *_EXPORT macros are used when building Chrome in component build, i.e. many
.so/.dll files rather than few. In these cases the linker needs to be told
whether functionality should be imported to or exported from the current build
target.
https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/background_budget_service.h:34: using
BudgetCallback = base::Callback<void(bool success)>;
Where would we use the success/failure callback of StoreBudget? How would we
behave differently when it fails?
https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/background_budget_service.h:36: using
StringCallback = base::Callback<void(const std::string& data)>;
nit: I'd call this BudgetCallback.
https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/background_budget_service.h:39: static void
GetBudget(Profile* profile,
Get/StoreBudget need to be documented. Today they store and retrieve whatever
string data you give them, but I'd hope that they get defined values in the
future. (E.g. a number?)
https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/background_budget_service_unittest.cc
(right):
https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/background_budget_service_unittest.cc:7: #include
<string>
micro nit: no new line above here
https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/background_budget_service_unittest.cc:23: class
BackgroundBudgetTestingProfile : public TestingProfile {
What is the benefit of this class today? Can you just use TestingProfile
directly until we need more?
https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/background_budget_service_unittest.cc:47: void
IgnoreResult() {}
nit: unused.
Description was changed from ========== Background service worker push message processing This is the first part of a many-part project to add a framework for service workers to perform background processing on behalf of the user in response to a push message but without always showing a visible notification to the user. In the inital implementation, chrome's 1 in 10 grace period will be replaced with a heuristic based budget derrived from the Site Engagement Score. This patch begins the road to that goal. We have created a new class, the BackgroundBudgetService which lives in chrome/browser/push_messaging. The implementation of the class is just to provide the grace period tracking that was previously provided in content/public/browser/push_messaging_service.cc. The backing storage has also been changed to the user prefs instead of the service_worker_context storage. The next stage of implementation will replace the grace period tracking with budget tracking and tie into the SiteEngagementService. BUG= ========== to ========== Background service worker push message processing This is the first part of a many-part project to add a framework for service workers to perform background processing on behalf of the user in response to a push message but without always showing a visible notification to the user. In the inital implementation, chrome's 1 in 10 grace period will be replaced with a heuristic based budget derrived from the Site Engagement Score. This patch begins the road to that goal. We have created a new class, the BackgroundBudgetService which lives in chrome/browser/push_messaging. The implementation of the class is just to provide the grace period tracking that was previously provided in content/public/browser/push_messaging_service.cc. The backing storage has also been changed to the user prefs instead of the service_worker_context storage. The next stage of implementation will replace the grace period tracking with budget tracking and tie into the SiteEngagementService. BUG=576837 ==========
High level comments: - My first start on the implementation was doing the SES and not the prefs move. Then I decided to do the storage first, because it was an easier/smaller patch to pull out from the main implementation. KeyedService was part of the first start, which I missed when I shifted focus. I've removed it now. - I'm not actually sure whether I want this to be a KeyedService long term or not. It doesn't really feel like a fully fledged service at this point, just a collection of implementations. That's the reason I've got the methods as static right now. The only data the class would own if they weren't static would be the profile, and it feels wrong to have all the overhead of an instantiated class and factory just to avoid passing the profile into the two calls. We can discuss it more in the next code review, where I'm actually implementing more of these things. - CL description updated for formatting, and I'll be filing a new bug which I'll add shortly. https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.cc (right): https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:15: namespace chrome { On 2016/04/08 10:41:10, Peter Beverloo wrote: > Please don't put things in the chrome namespace. Done. https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:19: std::string GetBudgetStringForServiceWorker(Profile* profile, On 2016/04/08 10:41:10, Peter Beverloo wrote: > nit (1): s/ForServiceWorker/ForOrigin/ > nit (2): All callers store this in |*_value|, method says |*_string|. > Consistency++? :) Done. https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:25: map->GetStringWithoutPathExpansion(origin.spec(), &map_value); On 2016/04/08 10:41:10, Peter Beverloo wrote: > What happens when |origin| is not an origin, but a full URL? :-) > > I would suggest adding the following DCHECK: > > DCHECK_EQ(origin.GetOrigin(), origin); Done. https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:28: } // namespace On 2016/04/08 10:41:10, Peter Beverloo wrote: > micro nit: blank line above this one Done. https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:41: // service. On 2016/04/08 10:41:11, Peter Beverloo wrote: > micro nit: This comment doesn't add much value. Would be better when phrased as > a TODO. I just removed it. https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:45: callback.Run(map_value); On 2016/04/08 10:41:10, Peter Beverloo wrote: > Do you expect this to be asynchronous in the future? > > If it will be, I would recommend posting a task rather than invoking > immediately. That avoids writing test code that inadvertently relies on > synchronous behaviour. > > base::ThreadTaskRunnerHandle::Get()->PostTask( > FROM_HERE, base::Bind(callback, map_value)); I did a bit of investigation into the prefs storage, and it looks like as long as we're not storing a significant amount of information there, it's a good long term place for our storage. Because that's in-memory, there doesn't seem to be a need for the lookup to be on a separate thread. https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.h (right): https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:25: // implementation it is just using the old grace period logic. On 2016/04/08 10:41:11, Peter Beverloo wrote: > Comment nits: > - s/chrome/Chrome/. > - "... will be ..., ... initial implementation ..." - the comment should > reflect what the code does *today*. Refer to a BUG in a TODO for functionality > that is yet to be added. Done. https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:26: // TODO(harkness) do I need a CHROME_EXPORT? On 2016/04/08 10:41:11, Peter Beverloo wrote: > No. > > The *_EXPORT macros are used when building Chrome in component build, i.e. many > .so/.dll files rather than few. In these cases the linker needs to be told > whether functionality should be imported to or exported from the current build > target. Acknowledged. https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:34: using BudgetCallback = base::Callback<void(bool success)>; On 2016/04/08 10:41:11, Peter Beverloo wrote: > Where would we use the success/failure callback of StoreBudget? How would we > behave differently when it fails? Currently, this isn't used at all. I had it in this code because this was a minimal change to the original code, but I think I'll pull out the callback entirely. https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:36: using StringCallback = base::Callback<void(const std::string& data)>; On 2016/04/08 10:41:11, Peter Beverloo wrote: > nit: I'd call this BudgetCallback. I'd actually already changed that in my next patch, but I'll move that change to this one. https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:39: static void GetBudget(Profile* profile, On 2016/04/08 10:41:11, Peter Beverloo wrote: > Get/StoreBudget need to be documented. Today they store and retrieve whatever > string data you give them, but I'd hope that they get defined values in the > future. (E.g. a number?) Done. https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service_unittest.cc (right): https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service_unittest.cc:7: #include <string> On 2016/04/08 10:41:11, Peter Beverloo wrote: > micro nit: no new line above here Done. https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service_unittest.cc:23: class BackgroundBudgetTestingProfile : public TestingProfile { On 2016/04/08 10:41:11, Peter Beverloo wrote: > What is the benefit of this class today? Can you just use TestingProfile > directly until we need more? My next patch will use it, but I can remove it for now. https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service_unittest.cc:47: void IgnoreResult() {} On 2016/04/08 10:41:11, Peter Beverloo wrote: > nit: unused. Done.
https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.cc (right): https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:45: callback.Run(map_value); On 2016/04/08 14:11:43, harkness wrote: > On 2016/04/08 10:41:10, Peter Beverloo wrote: > > Do you expect this to be asynchronous in the future? > > > > If it will be, I would recommend posting a task rather than invoking > > immediately. That avoids writing test code that inadvertently relies on > > synchronous behaviour. > > > > base::ThreadTaskRunnerHandle::Get()->PostTask( > > FROM_HERE, base::Bind(callback, map_value)); > > I did a bit of investigation into the prefs storage, and it looks like as long > as we're not storing a significant amount of information there, it's a good long > term place for our storage. Because that's in-memory, there doesn't seem to be a > need for the lookup to be on a separate thread. It's not about whether you're on the same thread or not, it's about whether |callback| will be called synchronously or asynchronously :-). Right now your tests would pass without pumping the message loops as well— that's fine, but it will allow subtle mistakes where code assumes it's not necessary to pump the loop. That's why I suggested the PostTask call: it posts to the *same thread*, which has the effect of making the response asynchronous. If the code will by synchronous, let's not bother with callbacks at all? https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.h (right): https://codereview.chromium.org/1861683002/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:26: // TODO(harkness) do I need a CHROME_EXPORT? On 2016/04/08 14:11:43, harkness wrote: > On 2016/04/08 10:41:11, Peter Beverloo wrote: > > No. > > > > The *_EXPORT macros are used when building Chrome in component build, i.e. > many > > .so/.dll files rather than few. In these cases the linker needs to be told > > whether functionality should be imported to or exported from the current build > > target. > > Acknowledged. So remove the TODO? :) https://codereview.chromium.org/1861683002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.cc (right): https://codereview.chromium.org/1861683002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:8: #include "base/strings/string_number_conversions.h" nit: unused https://codereview.chromium.org/1861683002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.h (right): https://codereview.chromium.org/1861683002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:22: // TODO(harkness) do I need a CHROME_EXPORT? Note: Personally I like using class headers as a TODO list by having a million of them, when building a system from the ground up. https://codereview.chromium.org/1861683002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:25: BackgroundBudgetService() {} If you're going the all static route (we need to talk about keyed services), please remove the non-static methods altogether and add DISALLOW_IMPLICIT_CONSTRUCTORS() from base/macros.h. https://codereview.chromium.org/1861683002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:26: ~BackgroundBudgetService() override {} Note: This (and line 28) will cause a compile failure because they don't override anything.
On 2016/04/08 14:29:38, Peter Beverloo wrote: > If the code will by synchronous, let's not bother with callbacks at all? Done! https://codereview.chromium.org/1861683002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.cc (right): https://codereview.chromium.org/1861683002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:8: #include "base/strings/string_number_conversions.h" On 2016/04/08 14:29:38, Peter Beverloo wrote: > nit: unused Done. https://codereview.chromium.org/1861683002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.h (right): https://codereview.chromium.org/1861683002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:22: // TODO(harkness) do I need a CHROME_EXPORT? On 2016/04/08 14:29:38, Peter Beverloo wrote: > Note: Personally I like using class headers as a TODO list by having a million > of them, when building a system from the ground up. I'll consider doing that in my next patch, but it feels weird to write TODOs here for code that's already written, just waiting for this patch to clear before I upload it. https://codereview.chromium.org/1861683002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:25: BackgroundBudgetService() {} On 2016/04/08 14:29:38, Peter Beverloo wrote: > If you're going the all static route (we need to talk about keyed services), > please remove the non-static methods altogether and add > DISALLOW_IMPLICIT_CONSTRUCTORS() from base/macros.h. For this patch, I'll go ahead and do that. https://codereview.chromium.org/1861683002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:26: ~BackgroundBudgetService() override {} On 2016/04/08 14:29:38, Peter Beverloo wrote: > Note: This (and line 28) will cause a compile failure because they don't > override anything. Done.
lgtm, but please file and associate a bug for the larger project. https://codereview.chromium.org/1861683002/diff/60001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service_unittest.cc (right): https://codereview.chromium.org/1861683002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service_unittest.cc:52: base::RunLoop().RunUntilIdle(); Please remove this. (Also on line 64 and 71, and the include on line 8.) https://codereview.chromium.org/1861683002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service_unittest.cc:55: EXPECT_EQ(data_, std::string()); nit: you'll get a slightly nicer message if you write it as: EXPECT_FALSE(success_); EXPECT_TRUE(data_.empty()); (dito on line 73)
Description was changed from ========== Background service worker push message processing This is the first part of a many-part project to add a framework for service workers to perform background processing on behalf of the user in response to a push message but without always showing a visible notification to the user. In the inital implementation, chrome's 1 in 10 grace period will be replaced with a heuristic based budget derrived from the Site Engagement Score. This patch begins the road to that goal. We have created a new class, the BackgroundBudgetService which lives in chrome/browser/push_messaging. The implementation of the class is just to provide the grace period tracking that was previously provided in content/public/browser/push_messaging_service.cc. The backing storage has also been changed to the user prefs instead of the service_worker_context storage. The next stage of implementation will replace the grace period tracking with budget tracking and tie into the SiteEngagementService. BUG=576837 ========== to ========== Background service worker push message processing This is the first part of a many-part project to add a framework for service workers to perform background processing on behalf of the user in response to a push message but without always showing a visible notification to the user. In the inital implementation, chrome's 1 in 10 grace period will be replaced with a heuristic based budget derrived from the Site Engagement Score. This patch begins the road to that goal. We have created a new class, the BackgroundBudgetService which lives in chrome/browser/push_messaging. The implementation of the class is just to provide the grace period tracking that was previously provided in content/public/browser/push_messaging_service.cc. The backing storage has also been changed to the user prefs instead of the service_worker_context storage. The next stage of implementation will replace the grace period tracking with budget tracking and tie into the SiteEngagementService. BUG=576837,602244 ==========
https://codereview.chromium.org/1861683002/diff/60001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service_unittest.cc (right): https://codereview.chromium.org/1861683002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service_unittest.cc:52: base::RunLoop().RunUntilIdle(); On 2016/04/11 10:55:59, Peter Beverloo wrote: > Please remove this. (Also on line 64 and 71, and the include on line 8.) Done. https://codereview.chromium.org/1861683002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service_unittest.cc:55: EXPECT_EQ(data_, std::string()); On 2016/04/11 10:55:59, Peter Beverloo wrote: > nit: you'll get a slightly nicer message if you write it as: > > EXPECT_FALSE(success_); > EXPECT_TRUE(data_.empty()); > > (dito on line 73) Done.
harkness@chromium.org changed reviewers: + bauerb@chromium.org
bauerb@chromium.org: Please review changes in chrome/browser/prefs/browser_prefs.cc
https://codereview.chromium.org/1861683002/diff/100001/chrome/browser/push_me... File chrome/browser/push_messaging/background_budget_service.h (right): https://codereview.chromium.org/1861683002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/background_budget_service.h:19: // A budget service to help chrome decide how much background work a service Nit: "Chrome" capitalized. https://codereview.chromium.org/1861683002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/background_budget_service.h:32: // expected to be a sting encoding whether the last 10 push messages triggered Nit: "string" :) https://codereview.chromium.org/1861683002/diff/100001/chrome/browser/push_me... File chrome/browser/push_messaging/background_budget_service_unittest.cc (right): https://codereview.chromium.org/1861683002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/background_budget_service_unittest.cc:13: namespace { Just FTR: You don't need the anonymous namespace if you only have constants in there, as they get internal linkage automatically. https://codereview.chromium.org/1861683002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/background_budget_service_unittest.cc:20: class BackgroundBudgetServiceTest : public ::testing::Test { The initial :: isn't necessary if you are not inside a namespace. https://codereview.chromium.org/1861683002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/background_budget_service_unittest.cc:36: BackgroundBudgetService::GetBudget(profile(), GURL(kTestOrigin)); That's what |origin| is for, no? :) https://codereview.chromium.org/1861683002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/background_budget_service_unittest.cc:38: EXPECT_TRUE(budget_string.empty()); Nit: If you compare this to an empty string with EXPECT_EQ, you'll get the actual value in case of a failure.
https://codereview.chromium.org/1861683002/diff/100001/chrome/browser/push_me... File chrome/browser/push_messaging/background_budget_service.h (right): https://codereview.chromium.org/1861683002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/background_budget_service.h:19: // A budget service to help chrome decide how much background work a service On 2016/04/11 13:58:42, Bernhard Bauer wrote: > Nit: "Chrome" capitalized. Done. https://codereview.chromium.org/1861683002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/background_budget_service.h:32: // expected to be a sting encoding whether the last 10 push messages triggered On 2016/04/11 13:58:41, Bernhard Bauer wrote: > Nit: "string" :) Done. https://codereview.chromium.org/1861683002/diff/100001/chrome/browser/push_me... File chrome/browser/push_messaging/background_budget_service_unittest.cc (right): https://codereview.chromium.org/1861683002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/background_budget_service_unittest.cc:13: namespace { On 2016/04/11 13:58:42, Bernhard Bauer wrote: > Just FTR: You don't need the anonymous namespace if you only have constants in > there, as they get internal linkage automatically. Good to know, although if you don't mind, I'll leave it here for now. My next patch adds a lot to that anonymous namespace. https://codereview.chromium.org/1861683002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/background_budget_service_unittest.cc:20: class BackgroundBudgetServiceTest : public ::testing::Test { On 2016/04/11 13:58:42, Bernhard Bauer wrote: > The initial :: isn't necessary if you are not inside a namespace. Done. https://codereview.chromium.org/1861683002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/background_budget_service_unittest.cc:36: BackgroundBudgetService::GetBudget(profile(), GURL(kTestOrigin)); On 2016/04/11 13:58:42, Bernhard Bauer wrote: > That's what |origin| is for, no? :) good point! https://codereview.chromium.org/1861683002/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/background_budget_service_unittest.cc:38: EXPECT_TRUE(budget_string.empty()); On 2016/04/11 13:58:42, Bernhard Bauer wrote: > Nit: If you compare this to an empty string with EXPECT_EQ, you'll get the > actual value in case of a failure. Done.
LGTM https://codereview.chromium.org/1861683002/diff/120001/chrome/browser/push_me... File chrome/browser/push_messaging/background_budget_service_unittest.cc (right): https://codereview.chromium.org/1861683002/diff/120001/chrome/browser/push_me... chrome/browser/push_messaging/background_budget_service_unittest.cc:38: EXPECT_EQ(budget_string, std::string()); And expected value first, please :)
The CQ bit was checked by harkness@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/1861683002/#ps140001 (title: "dcheck ordering")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1861683002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1861683002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
harkness@chromium.org changed reviewers: + avi@chromium.org
avi@chromium.org: Please review changes in content/public/browser/push_messaging_service.*
content lgtm
The CQ bit was checked by harkness@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1861683002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1861683002/140001
Message was sent while issue was closed.
Description was changed from ========== Background service worker push message processing This is the first part of a many-part project to add a framework for service workers to perform background processing on behalf of the user in response to a push message but without always showing a visible notification to the user. In the inital implementation, chrome's 1 in 10 grace period will be replaced with a heuristic based budget derrived from the Site Engagement Score. This patch begins the road to that goal. We have created a new class, the BackgroundBudgetService which lives in chrome/browser/push_messaging. The implementation of the class is just to provide the grace period tracking that was previously provided in content/public/browser/push_messaging_service.cc. The backing storage has also been changed to the user prefs instead of the service_worker_context storage. The next stage of implementation will replace the grace period tracking with budget tracking and tie into the SiteEngagementService. BUG=576837,602244 ========== to ========== Background service worker push message processing This is the first part of a many-part project to add a framework for service workers to perform background processing on behalf of the user in response to a push message but without always showing a visible notification to the user. In the inital implementation, chrome's 1 in 10 grace period will be replaced with a heuristic based budget derrived from the Site Engagement Score. This patch begins the road to that goal. We have created a new class, the BackgroundBudgetService which lives in chrome/browser/push_messaging. The implementation of the class is just to provide the grace period tracking that was previously provided in content/public/browser/push_messaging_service.cc. The backing storage has also been changed to the user prefs instead of the service_worker_context storage. The next stage of implementation will replace the grace period tracking with budget tracking and tie into the SiteEngagementService. BUG=576837,602244 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Background service worker push message processing This is the first part of a many-part project to add a framework for service workers to perform background processing on behalf of the user in response to a push message but without always showing a visible notification to the user. In the inital implementation, chrome's 1 in 10 grace period will be replaced with a heuristic based budget derrived from the Site Engagement Score. This patch begins the road to that goal. We have created a new class, the BackgroundBudgetService which lives in chrome/browser/push_messaging. The implementation of the class is just to provide the grace period tracking that was previously provided in content/public/browser/push_messaging_service.cc. The backing storage has also been changed to the user prefs instead of the service_worker_context storage. The next stage of implementation will replace the grace period tracking with budget tracking and tie into the SiteEngagementService. BUG=576837,602244 ========== to ========== Background service worker push message processing This is the first part of a many-part project to add a framework for service workers to perform background processing on behalf of the user in response to a push message but without always showing a visible notification to the user. In the inital implementation, chrome's 1 in 10 grace period will be replaced with a heuristic based budget derrived from the Site Engagement Score. This patch begins the road to that goal. We have created a new class, the BackgroundBudgetService which lives in chrome/browser/push_messaging. The implementation of the class is just to provide the grace period tracking that was previously provided in content/public/browser/push_messaging_service.cc. The backing storage has also been changed to the user prefs instead of the service_worker_context storage. The next stage of implementation will replace the grace period tracking with budget tracking and tie into the SiteEngagementService. BUG=576837,602244 Committed: https://crrev.com/373eb22ed62aaacd9d9e77c3843601eb88de7bf7 Cr-Commit-Position: refs/heads/master@{#386683} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/373eb22ed62aaacd9d9e77c3843601eb88de7bf7 Cr-Commit-Position: refs/heads/master@{#386683} |
