Add News custom RSS feed P3A questions, refactor News P3A #12518
Conversation
|
Two more questions! 🎉 Will look tomorrow. NB it's much easier to review refactoring if you make is a separate commit with just the refactor and then commit the functional changes on top of that. One less thing to keep track up looking for regressions. |
DJAndries
force-pushed
the
news-p3a
branch
2 times, most recently
from
96557f1 to
5eddb44
Compare
convinced, and done! |
| if (delta == 0) | ||
| break; |
| uint64_t day_delta = std::min(daily_value.value, delta); | ||
| daily_value.value -= day_delta; | ||
| delta -= day_delta; | ||
| } |
There was a problem hiding this comment.
So this distributes the subtraction across as many available daily values as possible, but it's best-effort because the weekly sum is still clamped to be non-negative? Worth documenting that in a comment I think.
Also, please add a unit test for the new WeeklyStorage method.
There was a problem hiding this comment.
your interpretation is correct. good call on the test, will add
| @@ -5,6 +5,7 @@ | |||
|
|
|||
| #include "brave/components/weekly_storage/weekly_storage.h" | |||
|
|
|||
| #include <algorithm> | |||
There was a problem hiding this comment.
Should this be in the later commit?
| template <std::size_t N> | ||
| void RecordToHistogramBucket(const char* histogram_name, | ||
| const int (&buckets)[N], |
There was a problem hiding this comment.
Does this make std::end() faster? I've had reviewers complain about templating before, because of code bloat, although it's probably fine in file-scope as here.
There was a problem hiding this comment.
I'm not sure if it makes it faster, the main objective is to let std::end find the end of the array via the non-type param.
DJAndries
force-pushed
the
news-p3a
branch
2 times, most recently
from
1522b2a to
fc4b592
Compare
| state_->SubDelta(4000); | ||
| EXPECT_EQ(state_->GetWeeklySum(), 4500U); | ||
| state_->SubDelta(5000); | ||
| EXPECT_EQ(state_->GetWeeklySum(), 0U); |
There was a problem hiding this comment.
Please add a comment here about how the overall sum can't go negative and further. Also please advance far enough for an earlier AddDelta value to expire and show that it still saturates properly.
components/p3a/brave_p3a_service.cc
Outdated
| @@ -99,6 +99,8 @@ constexpr const char* kCollectedHistograms[] = { | |||
| "Brave.Today.WeeklyMaxCardViewsCount", | |||
| "Brave.Today.WeeklyMaxCardVisitsCount", | |||
| "Brave.Today.WeeklySessionCount", | |||
| "Brave.Today.WeeklyAddedDirectFeedsCount", | |||
| "Brave.Today.DirectFeedsTotal", | |||
There was a problem hiding this comment.
let's keep the list sorted
Resolves brave/brave-browser#21519
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
brave://local-statepage, verify that theBrave.Today.DirectFeedsTotalmetric has a value of 2.Brave.Today.DirectFeedsTotalandBrave.Today.WeeklyAddedDirectFeedsCountmetrics have a value of 6.Brave.Today.DirectFeedsTotalmetric still has a value of 6 andBrave.Today.DirectFeedsTotalmetric has a value of 5.Brave.Today.DirectFeedsTotalmetric still has a value of 6 andBrave.Today.DirectFeedsTotalmetric has a value of 0.