Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1887623002:
Replace the 1 in 10 grace period with an accumulating budget based on SES. (Closed)
|
Created:
4 years, 8 months ago by harkness Modified:
4 years, 7 months ago Reviewers:
Peter Beverloo CC:
chromium-reviews, Peter Beverloo, mvanouwerkerk+watch_chromium.org, johnme+watch_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. |
DescriptionThis code replaces the existing 1 in 10 grace period with an accumulating
budget based on the site engagemene score for the origin.
This also allowed extensive simplification in the
PushMessagingNotificationManager, since we no longer need to update the
stored information when a notification is displayed.
This is the second 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.
BUG=602244
Committed: https://crrev.com/a942309be50c300eb6b18142e3feacf04d66649d
Cr-Commit-Position: refs/heads/master@{#393538}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Rebase to master after Keyed Service and add a clock parameter to BackgroundBudgetService construct… #Patch Set 3 : Integrated code review comments #
Total comments: 38
Patch Set 4 : Integrated code review comments and refactored service constructors. #
Total comments: 14
Patch Set 5 : Fixed nits #Patch Set 6 : Missed a file in update #Patch Set 7 : Added failure case for parsing error on prefs string. Also fixed memory leak in unit test. #
Total comments: 2
Patch Set 8 : Using Remove* instead of Set* #
Messages
Total messages: 24 (9 generated)
harkness@chromium.org changed reviewers: + peter@chromium.org
Description was changed from ========== This is the second 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. This code replaces the existing 1 in 10 grace period with an accumulating budget based on the site engagemene score for the origin. This also allowed extensive simplification in the PushMessagingNotificationManager, since we no longer need to update the stored information when a notification is displayed. BUG=602244 ========== to ========== This code replaces the existing 1 in 10 grace period with an accumulating budget based on the site engagemene score for the origin. This also allowed extensive simplification in the PushMessagingNotificationManager, since we no longer need to update the stored information when a notification is displayed. This is the second 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. BUG=602244 ==========
This is very exciting— our first user-visible steps towards silent push! Could you detail a little bit about how you plan to determine whether this is safe to do? For example, would a decrease in the number of push messages that end up showing our default notifications be a good thing or a concern? https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/background_budget_service.cc (right): https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service.cc:48: base::DoubleToString(budget) + kSeparator + base::DoubleToString(ses); Please check out base/strings/stringprintf.h. If anything, avoids many possible reallocations. https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service.cc:51: bool GetBudgetDataFromPrefValue(const std::string& pref_value, Can we combine this with GetBudgetDataFromPrefs? I'd rather keep the parsing/combining logic as self-contained as possible. Dito re: SetBudgetDataInPrefs and MakePrefValueFromBudgetData. https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service.cc:55: std::vector<std::string> parts = Please use base::SplitStringPiece() instead. String pieces are very similar to C++17s std::string_view, i.e. they refer to part of |pref_value|s memory and avoid the need to allocate for and copy each chunk in the variable. In this case we're not modifying |pref_value|, so that will be a much more elegant solution. https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service.cc:119: budget = budget_carryover + ses_component; So we have a linear decrease of budget over a period of ten days, then calculate the new budget by multiplying the average between the previous and current site engagement scores and multiplying that by the |ses_ratio|. Does this match your spreadsheet? :) https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/background_budget_service.h (right): https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service.h:28: static void GetBudget(Profile* profile, const GURL& origin, double& budget); Can we return the double instead? If you do decide to keep it as a parameter, could you make it a pointer? While a reference like this technically is allowed nowadays, there is an important difference in readability. Consider this example: { double budget = 42.0; // .... BudgetService::GetBudget(profile, origin, budget); // ... // What value does |budget| have? } By s/budget/&budget/ is already becomes much more obvious that the variable may be modified. https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service.h:30: // Store the budget associated with the origin. This method, as well as GetBudget(), should document what the expected values of |budget| are. Can I pass in a budget of 1.798E+308? We should then also have DCHECKs to make sure the budget stays within that contract. https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/background_budget_service_unittest.cc (right): https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service_unittest.cc:45: EXPECT_EQ(budget, 0.0); EXPECT_DOUBLE_EQ(), elsewhere too Comparisons are evil for doubles. https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service_unittest.cc:68: EXPECT_EQ(budget, kTestBudget); You probably want to talk to Sami about a thing we have called "virtual time". I don't know how usable it will be for your case, or if it has been implemented at all, but as I understand it, it would be a fantastic way to power your unit tests. I do feel we need more coverage for the calculations you're introducing. An easy option could be to store "historic" values in the preferences manually, getting the budget and seeing what happens. https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/push_messaging_notification_manager.h (right): https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_notification_manager.h:79: const double budget); nit: we don't usually put "const" in front of scalar arguments.
This includes the rebase to post-KeyedService master and also adding a clock on BackgroundBudgetService and testing for the budget algorithm. https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/background_budget_service.cc (right): https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service.cc:48: base::DoubleToString(budget) + kSeparator + base::DoubleToString(ses); On 2016/04/14 18:02:09, Peter Beverloo wrote: > Please check out base/strings/stringprintf.h. If anything, avoids many possible > reallocations. Done. https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service.cc:51: bool GetBudgetDataFromPrefValue(const std::string& pref_value, On 2016/04/14 18:02:09, Peter Beverloo wrote: > Can we combine this with GetBudgetDataFromPrefs? I'd rather keep the > parsing/combining logic as self-contained as possible. > > Dito re: SetBudgetDataInPrefs and MakePrefValueFromBudgetData. Done. https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service.cc:55: std::vector<std::string> parts = On 2016/04/14 18:02:08, Peter Beverloo wrote: > Please use base::SplitStringPiece() instead. > > String pieces are very similar to C++17s std::string_view, i.e. they refer to > part of |pref_value|s memory and avoid the need to allocate for and copy each > chunk in the variable. In this case we're not modifying |pref_value|, so that > will be a much more elegant solution. As discussed in person, there's currently no StringToDouble that takes a StringPiece, so while I'm using SplitStringPiece, I'm still doing the allocations using as_string(). I'll fix that in a future patch/CL. https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service.cc:119: budget = budget_carryover + ses_component; On 2016/04/14 18:02:09, Peter Beverloo wrote: > So we have a linear decrease of budget over a period of ten days, then calculate > the new budget by multiplying the average between the previous and current site > engagement scores and multiplying that by the |ses_ratio|. Does this match your > spreadsheet? :) The original spreadsheet assumed that everything was only being calculated once a day, so this algorithm is a slight change from the original, to include gaps in when it's calculated. There are two new pages to the spreadsheet titled "Time based..." which show the way the algorithm works with the time based variant, where I've varied how often the budget is computed. https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/background_budget_service.h (right): https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service.h:28: static void GetBudget(Profile* profile, const GURL& origin, double& budget); On 2016/04/14 18:02:09, Peter Beverloo wrote: > Can we return the double instead? If you do decide to keep it as a parameter, > could you make it a pointer? > > While a reference like this technically is allowed nowadays, there is an > important difference in readability. Consider this example: > > { > double budget = 42.0; > // .... > BudgetService::GetBudget(profile, origin, budget); > // ... > // What value does |budget| have? > } > > By s/budget/&budget/ is already becomes much more obvious that the variable may > be modified. Done. https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service.h:30: // Store the budget associated with the origin. On 2016/04/14 18:02:09, Peter Beverloo wrote: > This method, as well as GetBudget(), should document what the expected values of > |budget| are. Can I pass in a budget of 1.798E+308? We should then also have > DCHECKs to make sure the budget stays within that contract. Done. I capped it at 0.0 to the max value of SES. In the future we may consider going negative on budget, but for now that shouldn't happen. https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/background_budget_service_unittest.cc (right): https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service_unittest.cc:45: EXPECT_EQ(budget, 0.0); On 2016/04/14 18:02:09, Peter Beverloo wrote: > EXPECT_DOUBLE_EQ(), elsewhere too > > Comparisons are evil for doubles. Done, and I changed a lot of them to EXPECT_NEAR with appropriate bounds. https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/background_budget_service_unittest.cc:68: EXPECT_EQ(budget, kTestBudget); On 2016/04/14 18:02:09, Peter Beverloo wrote: > You probably want to talk to Sami about a thing we have called "virtual time". I > don't know how usable it will be for your case, or if it has been implemented at > all, but as I understand it, it would be a fantastic way to power your unit > tests. > > I do feel we need more coverage for the calculations you're introducing. An easy > option could be to store "historic" values in the preferences manually, getting > the budget and seeing what happens. Many more tests added, now that I have a clock in the BackgroundBudgetService. https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/push_messaging_notification_manager.h (right): https://codereview.chromium.org/1887623002/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_notification_manager.h:79: const double budget); On 2016/04/14 18:02:09, Peter Beverloo wrote: > nit: we don't usually put "const" in front of scalar arguments. Done.
Fantastic, thank you! Summarizing the impact on Push Notifications, we would grant up to five free silent notifications per day, one per ~2 navigations on the site. Have you considered requiring the user to be "moderately engaged" with a site prior to allowing this at all? Not sure if it's necessary. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.cc (right): https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:33: double* last_updated) { Something to consider for the longer term: store the budget in a structure that has a static FromString method, and a non-static ToString method. It would allow you to separate out storage from this file, as this is largely plumbing while there's some important logic in here. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:41: base::SplitStringPiece(map_string, std::string(1, kSeparator), nit: StringPieceSplit takes a StringPiece as the separator, so you can avoid this allocation by using StringPiece(&kSeparator, 1) instead. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:94: double BackgroundBudgetService::GetBudget(const GURL& origin) { DCHECK_EQ(origin, origin.GetOrigin()); (also in StoreBudget) This will guard against people accidentally passing a random URL to these methods. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:103: &last_updated_msec)) { Is it OK to silently ignore parsing errors? (I.e. the "return false" statements on lines 44, 47, 50 and 53.) Your comment on line 104 doesn't mention that case. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:113: // component, which gives extra budget to sites that have a high ses score. This would really benefit from an explanation of the algorithm in English :) https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:122: DCHECK(budget >= 0.0); DCHECK_GE https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:127: DCHECK(budget >= 0.0); DCHECK_GE https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:128: DCHECK(budget <= SiteEngagementScore::kMaxPoints); DCHECK_LE https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.h (right): https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:8: #include <string> IWYU: #include <memory> https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:27: // doesn't show a notification when it needed to. I've noticed that you're very diligent in keeping comments up-to-date, but in the longer term it may not be sustainable to keep mentioning the consumers. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:51: BackgroundBudgetService(Profile* profile, std::unique_ptr<base::Clock> clock); You could move some of the complexity from the BackgroundBudgetService to tests by doing the following: - Only have one BBS constructor: (Profile*, std::unique_ptr<base::Clock>) - BackgroundBudgetServiceFactory::BuildServiceInstanceFor() to create an instance with base::DefaultClock. - BackgroundBudgetServiceTest to override the BBSF's factory (BBSF::SetTestingFactory()) to create an instance with a custom clock, to which the test keeps holding a weak reference. This has the following benefits: (1) We can remove the second constructor. (2) You don't need the FRIEND_TEST_ALL_PREFIXES declarations above. (3) The BackgroundBudgetServiceTest can just *always* use a custom clock, avoiding the need for second instances in the new tests that you're introducing. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service_unittest.cc (right): https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service_unittest.cc:72: // Manually construct a BackgroundBudgetServie with a clock that the test nit: s/BackgroundBudgetServie/BackgroundBudgetService/g (3 occurrences) https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service_unittest.cc:104: EXPECT_NEAR(budget, kTestBudget, kTestSES * 1.0 / kSecondsToAccumulate); Heh, you're using EXPECT_NEAR as sort of a EXPECT_INCREASE :) While I'm not against this, one thing to keep in mind is that this would pass on *decreases* as well. (Although the check on line 123 would then fail.) https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service_unittest.cc:121: clock->SetNow(starting_time + base::TimeDelta::FromDays(11)); Why 11 days instead of 10? (kSecondsToAccumulate) https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service_unittest.cc:186: // than the starting budget. We should definitely measure the site's budget for *every* incoming message. Do you think we can still get that done in time for M52? (~May 20) https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_browsertest.cc:762: SetSiteEngagementScore(web_contents->GetURL(), 2.0); "Setting it to 3 means..." "...->GetURL(), 2.0);" You say three, but use two? https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/push_messaging_notification_manager.cc (right): https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_notification_manager.cc:257: double cost = 1.0; nit: this should be a constant at the top of this file https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_notification_manager.cc:265: content::PUSH_USER_VISIBLE_STATUS_REQUIRED_BUT_NOT_SHOWN_USED_GRACE); I think we should introduce a new enum value for budget, so that we can distinguish patterns more easily from grace uses.
The number of allowed silent pushes will really depend on how engaged the user is with the site. If the SES is 100, then per day, the webapp will get an extra 10 budget, so they could suppress 10 notifications per day. If they didn't use all of those, the budget would ramp up over time. Theoretically, if the site doesn't use any budget, they could eventually send 100 silent messages in a day, although they would only be able to do that on one day, and the next day they would be limited to 10 again. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.cc (right): https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:33: double* last_updated) { On 2016/05/04 15:41:51, Peter Beverloo wrote: > Something to consider for the longer term: store the budget in a structure that > has a static FromString method, and a non-static ToString method. It would allow > you to separate out storage from this file, as this is largely plumbing while > there's some important logic in here. Acknowledged. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:41: base::SplitStringPiece(map_string, std::string(1, kSeparator), On 2016/05/04 15:41:52, Peter Beverloo wrote: > nit: StringPieceSplit takes a StringPiece as the separator, so you can avoid > this allocation by using StringPiece(&kSeparator, 1) instead. Done. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:94: double BackgroundBudgetService::GetBudget(const GURL& origin) { On 2016/05/04 15:41:52, Peter Beverloo wrote: > DCHECK_EQ(origin, origin.GetOrigin()); > (also in StoreBudget) > > This will guard against people accidentally passing a random URL to these > methods. Done. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:103: &last_updated_msec)) { On 2016/05/04 15:41:52, Peter Beverloo wrote: > Is it OK to silently ignore parsing errors? (I.e. the "return false" statements > on lines 44, 47, 50 and 53.) Your comment on line 104 doesn't mention that case. I've updated the comment. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:113: // component, which gives extra budget to sites that have a high ses score. On 2016/05/04 15:41:51, Peter Beverloo wrote: > This would really benefit from an explanation of the algorithm in English :) I've updated it, which involved moving a few things around. Let me know if you think it reads better. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:122: DCHECK(budget >= 0.0); On 2016/05/04 15:41:52, Peter Beverloo wrote: > DCHECK_GE Done. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:127: DCHECK(budget >= 0.0); On 2016/05/04 15:41:51, Peter Beverloo wrote: > DCHECK_GE Done. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:128: DCHECK(budget <= SiteEngagementScore::kMaxPoints); On 2016/05/04 15:41:52, Peter Beverloo wrote: > DCHECK_LE Done. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.h (right): https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:8: #include <string> On 2016/05/04 15:41:52, Peter Beverloo wrote: > IWYU: #include <memory> Done. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:27: // doesn't show a notification when it needed to. On 2016/05/04 15:41:52, Peter Beverloo wrote: > I've noticed that you're very diligent in keeping comments up-to-date, but in > the longer term it may not be sustainable to keep mentioning the consumers. I'll generify the comment. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:51: BackgroundBudgetService(Profile* profile, std::unique_ptr<base::Clock> clock); On 2016/05/04 15:41:52, Peter Beverloo wrote: > You could move some of the complexity from the BackgroundBudgetService to tests > by doing the following: > > - Only have one BBS constructor: (Profile*, std::unique_ptr<base::Clock>) > - BackgroundBudgetServiceFactory::BuildServiceInstanceFor() to create an > instance with base::DefaultClock. > - BackgroundBudgetServiceTest to override the BBSF's factory > (BBSF::SetTestingFactory()) to create an instance with a custom clock, to which > the test keeps holding a weak reference. > > This has the following benefits: > (1) We can remove the second constructor. > (2) You don't need the FRIEND_TEST_ALL_PREFIXES declarations above. > (3) The BackgroundBudgetServiceTest can just *always* use a custom clock, > avoiding the need for second instances in the new tests that you're introducing. As discussed in person, I've combined the two constructors, but instead of using the SetTestingFactory, I've created a private method on BackgroundBudgetService that allows the test to override the default assigned clock. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service_unittest.cc (right): https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service_unittest.cc:72: // Manually construct a BackgroundBudgetServie with a clock that the test On 2016/05/04 15:41:52, Peter Beverloo wrote: > nit: s/BackgroundBudgetServie/BackgroundBudgetService/g (3 occurrences) Done. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service_unittest.cc:104: EXPECT_NEAR(budget, kTestBudget, kTestSES * 1.0 / kSecondsToAccumulate); On 2016/05/04 15:41:52, Peter Beverloo wrote: > Heh, you're using EXPECT_NEAR as sort of a EXPECT_INCREASE :) > > While I'm not against this, one thing to keep in mind is that this would pass on > *decreases* as well. (Although the check on line 123 would then fail.) I added extra GT checks to avoid that case. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service_unittest.cc:121: clock->SetNow(starting_time + base::TimeDelta::FromDays(11)); On 2016/05/04 15:41:52, Peter Beverloo wrote: > Why 11 days instead of 10? (kSecondsToAccumulate) Originally, I was using Advance() instead of SetNow() so I advanced 10 days, which put me around 11. Now that I'm using SetNow(), I reduced the days down to 10, which should be enough to stabilize the budget. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service_unittest.cc:186: // than the starting budget. On 2016/05/04 15:41:52, Peter Beverloo wrote: > We should definitely measure the site's budget for *every* incoming message. Do > you think we can still get that done in time for M52? (~May 20) Do you want to measure for every message, or only for when the budget is consumed? Right now, the algorithm has the very nice property that we only query SES and calculate the budget if a notification was needed but not shown. That's one of the changes that let me clean up PushMessagingNotificationManager so much. As for the UMA, I've got a branch I'm working on that now, I think we should be able to get it for 52. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_browsertest.cc:762: SetSiteEngagementScore(web_contents->GetURL(), 2.0); On 2016/05/04 15:41:52, Peter Beverloo wrote: > "Setting it to 3 means..." "...->GetURL(), 2.0);" > > You say three, but use two? oops, comment update fail. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/push_messaging_notification_manager.cc (right): https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_notification_manager.cc:257: double cost = 1.0; On 2016/05/04 15:41:52, Peter Beverloo wrote: > nit: this should be a constant at the top of this file Done. https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_notification_manager.cc:265: content::PUSH_USER_VISIBLE_STATUS_REQUIRED_BUT_NOT_SHOWN_USED_GRACE); On 2016/05/04 15:41:52, Peter Beverloo wrote: > I think we should introduce a new enum value for budget, so that we can > distinguish patterns more easily from grace uses. Do you mind if I do that in my next CL, when I do all my uma changes, since I'll be touching all those files for that already?
lgtm, thanks! https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service_unittest.cc (right): https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service_unittest.cc:186: // than the starting budget. On 2016/05/09 15:33:27, harkness wrote: > On 2016/05/04 15:41:52, Peter Beverloo wrote: > > We should definitely measure the site's budget for *every* incoming message. > Do > > you think we can still get that done in time for M52? (~May 20) > > Do you want to measure for every message, or only for when the budget is > consumed? > > Right now, the algorithm has the very nice property that we only query SES and > calculate the budget if a notification was needed but not shown. That's one of > the changes that let me clean up PushMessagingNotificationManager so much. > > As for the UMA, I've got a branch I'm working on that now, I think we should be > able to get it for 52. Since it's all synchronous, I think we can just query the budget whenever we want without introducing new methods or thread jumps, minimizing overhead? https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... File chrome/browser/push_messaging/push_messaging_notification_manager.cc (right): https://codereview.chromium.org/1887623002/diff/40001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_notification_manager.cc:265: content::PUSH_USER_VISIBLE_STATUS_REQUIRED_BUT_NOT_SHOWN_USED_GRACE); On 2016/05/09 15:33:27, harkness wrote: > On 2016/05/04 15:41:52, Peter Beverloo wrote: > > I think we should introduce a new enum value for budget, so that we can > > distinguish patterns more easily from grace uses. > > Do you mind if I do that in my next CL, when I do all my uma changes, since I'll > be touching all those files for that already? OK. https://codereview.chromium.org/1887623002/diff/60001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.cc (right): https://codereview.chromium.org/1887623002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:29: const double kSecondsToAccumulate = 864000.0; nit: you can use constexpr instead of const now if you want https://codereview.chromium.org/1887623002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:103: // SES. Sure, but is it *OK* to silently ignore parsing errors? This either means (a) a format update occurred and we drop budget on the floor, or (b) the user's profile preferences have been corrupted. https://codereview.chromium.org/1887623002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:110: // For each time period that elapses, we calculate the carryover ratio as the nit: Try to avoid the use of "we" in comments. https://codereview.chromium.org/1887623002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:129: double budget = budget_carryover + ses_component; Love the explanation, thank you :) https://codereview.chromium.org/1887623002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:143: base::Time t = clock_->Now(); nit: s/t/time/ https://codereview.chromium.org/1887623002/diff/60001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.h (right): https://codereview.chromium.org/1887623002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:48: void setClock(std::unique_ptr<base::Clock> clock); nit: Blank line after the friend declaration. Since it's called [className]Test I don't think the comment adds much value? Your choice :) https://codereview.chromium.org/1887623002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:48: void setClock(std::unique_ptr<base::Clock> clock); nit: (Override|Set)ClockForTesting() to further clarify the intended usage? Definitely begin the name with a capital.
The CQ bit was checked by harkness@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/1887623002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887623002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1887623002/diff/60001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.cc (right): https://codereview.chromium.org/1887623002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:29: const double kSecondsToAccumulate = 864000.0; On 2016/05/09 17:12:06, Peter Beverloo wrote: > nit: you can use constexpr instead of const now if you want Done. https://codereview.chromium.org/1887623002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:103: // SES. On 2016/05/09 17:12:06, Peter Beverloo wrote: > Sure, but is it *OK* to silently ignore parsing errors? This either means (a) a > format update occurred and we drop budget on the floor, or (b) the user's > profile preferences have been corrupted. As discussed offline, I've added an error log and removed the invalid prefs data if it is found. https://codereview.chromium.org/1887623002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:110: // For each time period that elapses, we calculate the carryover ratio as the On 2016/05/09 17:12:06, Peter Beverloo wrote: > nit: Try to avoid the use of "we" in comments. Done. https://codereview.chromium.org/1887623002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:129: double budget = budget_carryover + ses_component; On 2016/05/09 17:12:06, Peter Beverloo wrote: > Love the explanation, thank you :) Acknowledged. https://codereview.chromium.org/1887623002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.cc:143: base::Time t = clock_->Now(); On 2016/05/09 17:12:06, Peter Beverloo wrote: > nit: s/t/time/ Done. https://codereview.chromium.org/1887623002/diff/60001/chrome/browser/push_mes... File chrome/browser/push_messaging/background_budget_service.h (right): https://codereview.chromium.org/1887623002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:48: void setClock(std::unique_ptr<base::Clock> clock); On 2016/05/09 17:12:06, Peter Beverloo wrote: > nit: Blank line after the friend declaration. Since it's called [className]Test > I don't think the comment adds much value? Your choice :) moved/updated. https://codereview.chromium.org/1887623002/diff/60001/chrome/browser/push_mes... chrome/browser/push_messaging/background_budget_service.h:48: void setClock(std::unique_ptr<base::Clock> clock); On 2016/05/09 17:12:06, Peter Beverloo wrote: > nit: (Override|Set)ClockForTesting() to further clarify the intended usage? > Definitely begin the name with a capital. Done.
The CQ bit was checked by harkness@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/1887623002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887623002/120001
Still lgtm % one question https://codereview.chromium.org/1887623002/diff/120001/chrome/browser/push_me... File chrome/browser/push_messaging/background_budget_service.cc (right): https://codereview.chromium.org/1887623002/diff/120001/chrome/browser/push_me... chrome/browser/push_messaging/background_budget_service.cc:61: update_map->SetStringWithoutPathExpansion(origin.spec(), ""); Why not RemoveWithoutPathExpansion()?
https://codereview.chromium.org/1887623002/diff/120001/chrome/browser/push_me... File chrome/browser/push_messaging/background_budget_service.cc (right): https://codereview.chromium.org/1887623002/diff/120001/chrome/browser/push_me... chrome/browser/push_messaging/background_budget_service.cc:61: update_map->SetStringWithoutPathExpansion(origin.spec(), ""); On 2016/05/13 15:16:49, Peter Beverloo wrote: > Why not RemoveWithoutPathExpansion()? Because I hadn't seen that when I was looking through my options! updated it.
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 Link to the patchset: https://codereview.chromium.org/1887623002/#ps140001 (title: "Using Remove* instead of Set*")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1887623002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887623002/140001
Message was sent while issue was closed.
Description was changed from ========== This code replaces the existing 1 in 10 grace period with an accumulating budget based on the site engagemene score for the origin. This also allowed extensive simplification in the PushMessagingNotificationManager, since we no longer need to update the stored information when a notification is displayed. This is the second 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. BUG=602244 ========== to ========== This code replaces the existing 1 in 10 grace period with an accumulating budget based on the site engagemene score for the origin. This also allowed extensive simplification in the PushMessagingNotificationManager, since we no longer need to update the stored information when a notification is displayed. This is the second 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. BUG=602244 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== This code replaces the existing 1 in 10 grace period with an accumulating budget based on the site engagemene score for the origin. This also allowed extensive simplification in the PushMessagingNotificationManager, since we no longer need to update the stored information when a notification is displayed. This is the second 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. BUG=602244 ========== to ========== This code replaces the existing 1 in 10 grace period with an accumulating budget based on the site engagemene score for the origin. This also allowed extensive simplification in the PushMessagingNotificationManager, since we no longer need to update the stored information when a notification is displayed. This is the second 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. BUG=602244 Committed: https://crrev.com/a942309be50c300eb6b18142e3feacf04d66649d Cr-Commit-Position: refs/heads/master@{#393538} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/a942309be50c300eb6b18142e3feacf04d66649d Cr-Commit-Position: refs/heads/master@{#393538} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
