Brave Today on New Tab Page #6192
Conversation
petemill
force-pushed
the
brave-today
branch
3 times, most recently
from
78230bb
to
7f1754c
Compare
components/brave_extension/extension/brave_extension/background/today/feed.ts
Show resolved
Hide resolved
| const pages: BraveToday.Page[] = [] | ||
| let canGenerateAnotherPage = true | ||
| // Sanity check: arbitrary max pages so we don't end up | ||
| // in infinit loop. |
|
|
||
| export async function getUnpaddedAsDataUrl(buffer: ArrayBuffer, mimeType = 'image/jpg') { | ||
| const data = new DataView(buffer) | ||
| const contentLength = data.getUint32(0, false) |
There was a problem hiding this comment.
Might be helpful to use getUint32(0, false /* big endian */) to document that this is done correctly :)
There was a problem hiding this comment.
I added that, thanks for the tip
| // you can obtain one at http://mozilla.org/MPL/2.0/. | ||
|
|
||
| export const URLS = { | ||
| braveTodayFeed: 'https://pcdn.brave.software/brave-today/feed.json', |
There was a problem hiding this comment.
This needs to be brave-today-cdn.brave.com.
There was a problem hiding this comment.
Still waiting on this to be pushed to *.brave.com
| "permissions": [ "contentSettings", "settingsPrivate", "management", "tabs", "storage", "webNavigation", "contextMenus", "cookies", "*://*/*", "chrome://favicon/*" ], | ||
| "content_security_policy": "default-src 'self'; font-src 'self' data:; script-src 'self'; style-src 'unsafe-inline'; img-src 'self' data: chrome://favicon/;", | ||
| "permissions": [ "contentSettings", "settingsPrivate", "management", "tabs", "history", "storage", "webNavigation", "contextMenus", "cookies", "*://*/*", "chrome://favicon/*", "https://pcdn.brave.software/*", "https://brave-today-cdn.brave.software/*" ], | ||
| "content_security_policy": "default-src 'self' https://pcdn.brave.software https://brave-today-cdn.brave.software; font-src 'self' data:; script-src 'self'; style-src 'unsafe-inline'; img-src 'self' data: chrome://favicon/ https://pcdn.brave.software; connect-src https://pcdn.brave.software https://brave-today-cdn.brave.software;", |
There was a problem hiding this comment.
We should be able to avoid putting pcdn.brave.com and brave-today-cdn.brave.software in default-src and instead just put them in connect-src and img-src as needed.
There was a problem hiding this comment.
are we going to use brave.software instead of brave.com for this service? if so we might need to do key pinning for it and also add it to the network audit whitelist if it's not there already
There was a problem hiding this comment.
Actually I think brave-today-cdn.brave.software will also change to brave-today-cdn.brave.com before this is merged.
There was a problem hiding this comment.
Still waiting for this to be deployed. It's coming soon as far as I can tell.
There was a problem hiding this comment.
Just to recap, once the files have been uploaded to the right places, we can:
- put
pcdn.brave.comandbrave-today-cdn.brave.cominconnect-srcandimg-src - leave
default-srcset to'self'
There was a problem hiding this comment.
@fmarier I've put the staging URLS in connect-src and removed any additions to img-src and permissions. PTAL.
petemill
force-pushed
the
brave-today
branch
3 times, most recently
from
0f56cf0
to
2930db5
Compare
components/brave_extension/extension/brave_extension/background/today/feedToData.ts
Outdated
Show resolved
Hide resolved
components/brave_extension/extension/brave_extension/background/today/feedToData.ts
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| export default async function getBraveTodayData ( |
There was a problem hiding this comment.
Lots of logic in here - could be useful to break specific parts out to a new method(s) if we need to unit test specific parts. That could always be done at a later time (follow up) 😄
There was a problem hiding this comment.
Yeah I was thinking I'd unit test this whole function. Already broken out take<T>, generateNextPage and the weighting, but can also break out all the category calculations.
components/brave_extension/extension/brave_extension/background/today/feedToData.ts
Show resolved
Hide resolved
components/brave_extension/extension/brave_extension/background/today/index.ts
Show resolved
Hide resolved
components/brave_extension/extension/brave_extension/background/today/publishers.ts
Outdated
Show resolved
Hide resolved
petemill
force-pushed
the
brave-today
branch
3 times, most recently
from
3f1b695
to
928b901
Compare
Feed is once per hour and sources is once per day Removed a unit test that was not doing anything useful (copying functionality from function to test and comparing output).
This shouldn't happen in production since the feed parser verifies that a publisher exists. Although it could happen if the sources list changes to remove a publisher, but the feed cache has not been updated yet. Also can occur in testing and storybook.
…drop-filter Also, tighten style for indicator. Overcomes a bug where a backdrop-filter element's ancestor which has a filter must also have a background. When this bug is fixed then this element won't need the background. Before this commit, the Widget title backgrounds would not correctly have backdrop filter because their ancestor had a blur and not also a background (it came from a higher up ancestor).
Deep links to correct Settings tab. Clean up how we do deep links and refactor the "Add Cards" link too. In fact, cleaned up all tab index checking with an enum.
…fied More Cards behavior and look should be the same
Creates a trusted types policy which is required now that we have strict trusted types enabled for scripts.
…hange has applied
Prevents a bug where cmd-click an item would navigate to the article as well as the browser opening another tab.
- When Brave Today is interacted with (scrolled down to), get the current cached feed - When NTP is opened and Today is enabled, make sure we have either loaded data from cache or fetched the first data feed (and publishers list) - Whilst Brave Today is open, every 1 minutes, check if the backend has an updated feed, or if it knows that remote has updated feed. The backend will only check for remote update every 10 mins (whilst Brave Today is visible), via a HEAD request. - Every 2 hours, the backend will do an actual feed refresh regardless of whether Brave Today is open or not
|
Pushed a very minor amend to fix a missing string and a bug that was preventing some deals from showing ...and also another rebase on master... |
|
There was one browser-test failure on Linux, but it appears to be intermittent as it passed great on prior runs: |
| @@ -0,0 +1,46 @@ | |||
| // Copyright (c) 2020 The Brave Authors. All rights reserved. | |||
There was a problem hiding this comment.
common has a specific meaning in chromium and this should not be at the top level of components like this
Resolves brave/brave-browser#12574
TODO
Follow-up
Description
Implementing Brave Today on Desktop:
Implementing a communication channel between WebUI (chrome://newtab) and the Brave Extension background page.
This is being used on this project for 2 reasons:
Because
SharedWorkeris not (easily) supported in WebUI and I did not want to perform feed and image data manipulation on the front-end JS thread and for every single NTP instance (which are frequent).To perform a fetch of the feed, since NTP front-end is not able to make requests on the network.
Certainly, this could be performed in c++, but timing and the need to iterate meant that the background JS "worker" was a great place for this. This background fetching and prefs may well be ported to c++ in the future, most likely when working on bringing this feature to Android.
Storage / prefs
Aside from the regular usage of chromium profile prefs to determine if the user has opted-out if Language is set to English or opted-in for all other Languages, we are using extension (local) storage for user preferences as to which publisher sources to enable or disable for the Brave Today feed.
Architecture
Notable areas to consider:
Extension background process
Sets up listening to messages from the NTP WebUI (via
chrome.runtime.onMessageExternal)Fetches and caches (both in-memory and in local storage) remote data from private CDN.
Fetches and caches (both in-memory and in local storage) remote publisher list from private CDN.
Sets and gets prefs about which publishers are explicitly enabled or disabled by the user. Stores in
chrome.storage.local.Content selection algorithm. Breaks the articles in to categories and pages. Somewhat randomizes and adds weight based on the user's recent browsing history.
Expresses URLs to fetch content from, and helps "unpad" consistent-byte-length images.
NTP WebUI renderer process
Handles UI actions (init, refresh, preference-changed, etc) and communicates with extension background process, receiving feed data.
Communicates with extension background process, receiving image data via padded content fetched from the Private CDN.
Submitter Checklist:
npm 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.