-
Notifications
You must be signed in to change notification settings - Fork 888
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
Include web discovery bundles in the brave extension #9381
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the bundles seem to be missing (they are not injected directly from manifest.json but are needed to initialize instances of Worker
dynamically):
rusha.bundle.js
which can be found in the output folder atbuild/modules/human-web/rusha.bundle.js
. It is needed to initialize aWorker
dynamically here: https://github.com/brave/humanweb/blob/main/modules/human-web/sources/human-web.es#L5399worker.wasm.bundle.js
andworker.asmjs.bundle.js
which can both be found inbuild/modules/hpnv2/
and are needed to initialize aWorker
here: https://github.com/brave/humanweb/blob/main/modules/hpnv2/sources/group-signer.es#L56
I think we might need to figure out the value of this config.baseURL
which is needed to find the path to the bundle at runtime (and enable the workers).
Optionally, these bundles come with sourcemaps, but maybe we can ignore that until we do some more build system work and move to the webpack stack.
components/brave_extension/extension/brave_extension/_locales/en_US/messages.json
Outdated
Show resolved
Hide resolved
c049e5f
to
7e34e60
Compare
components/brave_extension/extension/brave_extension/manifest.json
Outdated
Show resolved
Hide resolved
7e34e60
to
fc4ad3b
Compare
fc4ad3b
to
1353245
Compare
components/brave_extension/extension/brave_extension/manifest.json
Outdated
Show resolved
Hide resolved
components/brave_extension/extension/brave_extension/manifest.json
Outdated
Show resolved
Hide resolved
components/brave_extension/extension/brave_extension/background/humanWeb.ts
Outdated
Show resolved
Hide resolved
Should we at least put a more unobfuscated build / bundle output from the current humanweb source here? |
Given the amount of work for v0 and unless there is strong opposition to this idea, I'd ideally prefer to keep the code in a separate repo for simplicity. I think there are also few advantages in having it separated for now (and open-source, of course):
I'm sure all of these can be solved but there are many other blocking items on the list of things to do. Wdyt? |
That is fine as long as people can verify that the bundled code is the same as the code in the repo, like via a reproducible build process that is documented in the open source repo. |
Instead of including the bundled code in the git tree, you could have a build time script that pulls the open source code from the separate repo and bundles it, for instance. |
I guess we can publish releases from the open-source repository and include them in package.json/package-lock.json right? Then people know which tag is currently in use in Brave. |
196c901
to
ad01a71
Compare
@diracdeltas did this in the newest push |
components/brave_extension/extension/brave_extension/manifest.json
Outdated
Show resolved
Hide resolved
components/brave_extension/extension/brave_extension/background/webDiscoveryProject.ts
Outdated
Show resolved
Hide resolved
@remusao I added the copywriter's text and the entry to the settings page, all tied up to extension enable/disable. |
const pref = prefs.find(p => p.key === WEB_DISCOVERY_PREF_KEY) | ||
if (pref) { | ||
toggleWebDiscovery(pref) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit. Since you already check if (pref ...)
in the toggleWebDiscovery
function maybe this can be simplified with:
const pref = prefs.find(p => p.key === WEB_DISCOVERY_PREF_KEY) | |
if (pref) { | |
toggleWebDiscovery(pref) | |
} | |
toggleWebDiscovery(prefs.find(p => p.key === WEB_DISCOVERY_PREF_KEY)) |
components/brave_extension/extension/brave_extension/background/webDiscoveryProject.ts
Outdated
Show resolved
Hide resolved
components/brave_extension/extension/brave_extension/background/webDiscoveryProject.ts
Outdated
Show resolved
Hide resolved
This way users that don't have the feature enabled won't incur the performance cost of loading the script. Includes webRequest optimizations in the web-discovery codebase.
8436c59
to
5991191
Compare
components/brave_extension/extension/brave_extension/background/webDiscoveryProject.ts
Outdated
Show resolved
Hide resolved
We inject if the following conditions are met: 1. WDP pref is enabled 2. Not loading a chrome:// resource 3. Page is not loaded from bfcache (avoids double inject)
23f8c99
to
9a90817
Compare
I've pushed a patch from @remusao in the slack chat. |
33db218
to
13c89fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chromium_src
++
const url = tab.url || tab.pendingUrl || '' | ||
return url ? !/chrome:\/\//.test(url) : false | ||
} | ||
// We inject on `complete` since multiple `loading` events can fire. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually very late in the process. As discussed on slack I think we want webRequest.onCommitted instead
Testing guidelines for QA:
WDP.modules['web-discovery-project'].background.webDiscoveryProject.patternsLoader.resourceWatcher.forceUpdate()
WDP.modules['web-discovery-project'].background.webDiscoveryProject.strictQueries.map(x=>x.tDiff=0)
The important bit here is the first call to similar URL you visited in the tab (double-fetch), then one or more calls to
|
Verification PASSED on
Went through and verified the STR/Cases outlined via #9381 (comment) as per the following:
|
We'll vendor the code at sync time until we can port their build system
Resolves brave/brave-browser#18166
Test Plan
See following comment: #9381 (comment)
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-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 tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: