use mojo types for PublisherInfo #2432
Conversation
bridiver
force-pushed
the
publisher-info-mojom
branch
4 times, most recently
from
440a856
to
7f01dcc
Compare
bridiver
force-pushed
the
publisher-info-mojom
branch
from
7f01dcc
to
674187c
Compare
| } | ||
| // TODO(bridiver) - this doesn't belong here | ||
| std::sort(site_list.begin(), site_list.end()); |
There was a problem hiding this comment.
To solve this one I think we need to emit event to UI that list was normalized so that UI pulls it again. This way we don't need to sort it here, but UI decides if they want to pull. Down side is that data that we already have will need to be pulled from db again
There was a problem hiding this comment.
actually I think the list normalization itself is a problem. @tmancey and I were discussing because calculated fields shouldn't be persisted in the db
There was a problem hiding this comment.
the percentage, etc... should be calculated at query time
There was a problem hiding this comment.
why would we like to do that on query time? then every time that we load rewards page we would nee to recalculate it even though that nothing changed
There was a problem hiding this comment.
It is normalized every time publisher info is saved which happens very frequently. We are actually wasting far more cycles writing things that are never read. It can also easily lead to incorrect data or complicated data migrations when formulas change
There was a problem hiding this comment.
also what we're doing in the synopsis normalizer is trivial for the DB to do and doesn't require pulling all the records into memory (a problem for mobile). It is both more efficient and more reliable to do it at query time in the DB
bridiver
force-pushed
the
publisher-info-mojom
branch
3 times, most recently
from
6b6f474
to
a78418f
Compare
| using PublisherInfoPtr = mojom::PublisherInfoPtr; | ||
| using PublisherInfoList = std::vector<PublisherInfoPtr>; | ||
| // TODO - remove this | ||
| using PublisherInfoListStruct = PublisherInfoList; |
There was a problem hiding this comment.
I'm not sure if I removed all references to it
| ContentSiteList site_list; | ||
| for (auto& publisher : *list) { | ||
| if (publisher.percent >= 1) { | ||
| site_list.push_back(PublisherInfoToContentSite(publisher)); |
There was a problem hiding this comment.
are these two lines supposed to be removed by this PR, or we moved this logic somewhere else?
There was a problem hiding this comment.
There was a problem hiding this comment.
As we discussed offline, this logic (the if (publisher.percent >= 1) part) was unintentionally removed when rebasing, let's put it back for now and move this UI logic to a suitable place later.
bridiver
force-pushed
the
publisher-info-mojom
branch
from
a78418f
to
bbc3e7d
Compare
|
builds completed successfully, just an intermittent security check error not related to this PR |
Resolves brave/brave-browser#5130 Minor addition to #2432
Resolves brave/brave-browser#4425
Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security) onnpm run lint)git rebase master(if needed).git rebase -ito squash commits (if needed).Test Plan:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.