Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Brave Today: optimize fetching #7353

Merged
merged 1 commit into from
Dec 7, 2020
Merged

Brave Today: optimize fetching #7353

merged 1 commit into from
Dec 7, 2020

Conversation

petemill
Copy link
Member

@petemill petemill commented Dec 5, 2020

Submitter Checklist:

  • There is a ticket for my issue.
  • Used Github auto-closing keywords in the commit message.
  • Wrote a good PR/commit description
  • 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, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed).
  • Requested a security/privacy review as needed.

Reviewer Checklist:

  • 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:

Test Plan:

@petemill petemill requested review from bsclifton and ryanml December 5, 2020 05:26
@petemill petemill self-assigned this Dec 5, 2020
@petemill petemill force-pushed the today-optimize-fetch branch from 54a74f1 to 4aacfd6 Compare December 5, 2020 05:34
}).catch((e) => console.error(e))
// Refreshing of content after prefs changed is throttled, so wait
// a while before seeing if we have new content yet.
// This doens't have to be exact since we often check for update when
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/doens't/doesn't

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested changes, works good! Code changes look good too. Async startup/shutdown when Today enabled/disabled, debounced re-fetch for publisher pref changes. Very nice 😄

Copy link
Contributor

@ryanml ryanml left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great!

@petemill
Copy link
Member Author

petemill commented Dec 7, 2020

CI passed apart from Windows but that was "unstable" not "fail" so 👍

- Do not fetch any data from remote until user has interacted with brave today once
- Fix double-fetching due to feed fetch also fetching sources which notifies feed there is an update which then performs feed fetch...
- Debounce fetch feed update when user is toggling publishers
@petemill petemill force-pushed the today-optimize-fetch branch from 4aacfd6 to d174f90 Compare December 7, 2020 22:15
@petemill
Copy link
Member Author

petemill commented Dec 7, 2020

Pushed changes that were only comment updates

@petemill petemill merged commit 5bd8338 into master Dec 7, 2020
@petemill petemill deleted the today-optimize-fetch branch December 7, 2020 22:16
petemill added a commit that referenced this pull request Dec 7, 2020
@kjozwiak kjozwiak added this to the 1.20.x - Nightly milestone Dec 8, 2020
@kjozwiak
Copy link
Member

kjozwiak commented Dec 9, 2020

Verification PASSED on Win 10 x64 using the following build:

Brave | 1.20.17 Chromium: 87.0.4280.88 (Official Build) nightly (64-bit)
-- | --
Revision | 89e2380a3e36c3464b5dd1302349b1382549290d-refs/branch-heads/4280@{#1761}
OS | Windows 10 OS Version 2009 (Build 19042.630)

Followed the cases/summary motioned via both brave/brave-browser#13059 & brave/brave-browser#13058 and ran through the following cases:

  • ensured that both https://brave-today-cdn.brave.com & https://pcdn.brave.com are not being contacted when:
  • launching Brave for the first time
  • opening a NTP without interacting/scrolling through Brave Today
  • ensured that both https://brave-today-cdn.brave.com & https://pcdn.brave.com are only initially contacted when:
    • user starts scrolling through Brave Today via a NTP
    • open the NTP Customize modal and clicking on Brave Today (will load sources.json from https://brave-today-cdn.brave.com
    • https://brave-today-cdn.brave.com --> downloads feed.json & sources.json
    • https://pcdn.brave.com --> pulls down the images
  • ensured that when a change is made/publisher is added/removed, Brave contacts https://brave-today-cdn.brave.com and pulls a new feed.json & sources.json
  • ensured that scrolling through the Brave Today feed, only https://pcdn.brave.com is contacted while pulling down images

Scrolling through Brave Today for the first time:

Example 1 Example 2
BraveTodayInitial1 BraveTodayInitial2

Scrolling through Brave Today feed:

Example 1 Example 2
TodayScrolling1 TodayScrolling2

Open Brave Today section via Customize for the first time:

source

petemill added a commit that referenced this pull request Dec 9, 2020
kjozwiak added a commit that referenced this pull request Dec 9, 2020
1.19.x - Uplift #7353 (today-optimize-fetch)
kjozwiak added a commit that referenced this pull request Dec 9, 2020
1.18.x - Uplift #7353 (today-optimize-fetch)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants