[Brave News]: FeedV2Builder refactor

[fallaciousreasoning]

Resolves https://github.com/brave/brave-browser/issues/38180

This PR refactors the FeedV2Builder to split out a FeedGenerationInfo struct and keep track of the number of articles available in each content group. This allows us to avoid attempting to generate from a ContentGroup with no available articles (which terminates the feed generation).

Additionally, this logic is reused for generating Channel blocks in the feed.

@LorenzoMinto to you remember what the issue for this was?

TODO

• [x] Add test
• [x] Rename ArticleWeight to ArticleMeta

Submitter Checklist:

• [ ] I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
• [ ] There is a ticket for my issue
• [ ] Used Github auto-closing keywords in the PR description above
• [ ] Wrote a good PR/commit description
• [ ] Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
• [ ] Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
• [ ] Checked the PR locally:
  • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
  • npm run presubmit wiki, npm run gn_check, npm run tslint
• [ ] Ran git rebase master (if needed)

Reviewer Checklist:

• [ ] A security review is not needed, or a link to one is included in the PR description
• [ ] New files have MPL-2.0 license header
• [ ] Adequate test coverage exists to prevent regressions
• [ ] Major classes, functions and non-trivial code blocks are well-commented
• [ ] Changes in component dependencies are properly reflected in gn
• [ ] Code follows the style guide
• [ ] Test plan is specified in PR before merging

After-merge Checklist:

• [ ] The associated issue milestone is set to the smallest version that the changes has landed on
• [ ] All relevant documentation has been updated, for instance:
  • [ ] https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
  • [ ] https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
  • [ ] https://github.com/brave/brave-browser/wiki/Fingerprinting-Protections
  • [ ] https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
  • [ ] https://github.com/brave/brave-browser/wiki/Web-Compatibility-Exceptions-in-Brave
  • [ ] https://github.com/brave/brave-browser/wiki/QA-Guide
  • [ ] https://github.com/brave/brave-browser/wiki/P3A

Test Plan:

[LorenzoMinto]

This would be help fix https://github.com/brave/brave-browser/issues/36137. We can filter out articles older than X days and confidently sample if a content group runs out of eligible content. Wdyt? I guess we could fix it here or as a follow up

[fallaciousreasoning]

Lets do it as a followup :smile: I'd like to try and get this merged

[fallaciousreasoning]

@LorenzoMinto mind taking a look?

[github-actions[bot]]

[puLL-Merge] - brave/brave-core@23492

Here is my review of the PR:

Description

This PR adds a new FeedGenerationInfo class that encapsulates all the data needed to generate a personalized news feed. It moves some feed generation logic out of FeedV2Builder into this new class. The main motivation seems to be to simplify the feed generation code by centralizing the data and logic in one place.

Changes

Changes

feed_generation_info.h, feed_generation_info.cc:

• Added new FeedGenerationInfo class that stores locale, feed items, publishers, channels, signals, suggested publisher IDs and topics
• Provides methods to get article infos, eligible content groups, pick and consume articles based on a picking strategy
• Lazily initializes article infos and content groups when first accessed
• Reduces publisher/channel counts when articles are consumed to maintain content group eligibility

feed_sampling.h, feed_sampling.cc:

• Renamed ArticleWeight struct to ArticleMetadata
• Moved some article metadata generation logic from free functions into FeedGenerationInfo
• Added PickChannelRoulette function to pick articles from a specific channel

feed_v2_builder.cc:

• Refactored feed generation methods to use FeedGenerationInfo instead of separate parameters
• Removed some duplicate logic that is now handled in FeedGenerationInfo

feed_generation_info_unittest.cc, feed_sampling_unittest.cc:

• Added unit tests for the new FeedGenerationInfo class
• Moved some unit tests from feed_sampling_unittest.cc to feed_generation_info_unittest.cc

Overall, the changes look good and well-structured. Encapsulating the feed generation data and logic in the FeedGenerationInfo class seems to simplify and clean up the code nicely. The unit tests provide decent coverage of the new functionality.

Let me know if you have any other questions!

[fallaciousreasoning]

@LorenzoMinto & @simonhong thanks for the review

What do you think about renaming FeedGenerationInfo to FeedGenerator as otherwise it reads more like a data class, but it's doing quite a bit that it's not just representational

Yeah, I agree the name isn't quite right, but I think FeedGenerator is equally misleading, as it doesn't actually generate the feed. I might stick with the current name for now.

And also, what do you think about moving the generation functions GenerateBlock, GenerateCluster/Special/ChannelBlock etc. to the the generator and passing in the relevant arguments?

It's on my TODO list, but it's not quite that simple yet, as I'm not 100% sure the best way to set up the generation method from the FeedV2Builder. I'm keen to leave it as is for now until I get the chance to think some more.

Mind both having another look?