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

Bootstrap Web Discovery Native, add credential manager and server config loading #24969

Open
wants to merge 9 commits into
base: wdp-native-anon-creds
Choose a base branch
from

Conversation

DJAndries
Copy link
Collaborator

Resolves brave/brave-browser#39439

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:

Test Plan:

@DJAndries DJAndries requested review from deeppandya, fmarier and a team as code owners August 1, 2024 23:08
@github-actions github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Aug 1, 2024
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

Copy link
Member

@fmarier fmarier left a comment

Choose a reason for hiding this comment

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

script/brave_license_helper.py change looks good

@@ -682,6 +696,16 @@ private void updateBravePreferences() {
getPreferenceScreen().removePreference(mSendP3A);
}

if (mSendWebDiscovery != null) {
mSendWebDiscovery.setTitle(
Copy link
Contributor

Choose a reason for hiding this comment

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

we can remove title/summary here as we already have those mentioned in .xml

Copy link
Contributor

@deeppandya deeppandya left a comment

Choose a reason for hiding this comment

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

Android changes LGTM with minor comment.

android/BUILD.gn Outdated
@@ -5,6 +5,7 @@

import("//brave/components/ai_chat/core/common/buildflags/buildflags.gni")
import("//brave/components/p3a/buildflags.gni")
import("//brave/components/web_discovery/common/buildflags/buildflags.gni")
Copy link
Member

Choose a reason for hiding this comment

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

please store it in //brave/components/web_discovery/buildflags/buildflags.gni

Copy link
Collaborator

Choose a reason for hiding this comment

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

to be clear that means that everything in browser should also move into the root of components/web_discovery. https://github.com/brave/brave-core/pull/24970/files adds some renderer code, but I'm not sure we actually need that as there are existing mechanisms that could possibly be used https://github.com/brave/brave-core/pull/24970/files

Copy link
Member

Choose a reason for hiding this comment

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

to be clear that means that everything in browser should also move into the root of components/web_discovery.

I don't get why browser should be moved to the root of a component. buildflags is unconditional target, other targets should be conditional, including common, browser, renderer, etc., and they should stay in their own directories.

There's other change we should do (after discussing with @atuchin-m): browser should actually be renamed into core if we're aiming to use most of the parts on iOS.

Copy link
Collaborator

@bridiver bridiver Aug 8, 2024

Choose a reason for hiding this comment

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

actually looks like upstream code is putting buildflags in the root even when there are process/common/core directories. Not sure if that changed at some point or if we've just always been doing it incorrectly when there are process specific directories (which there will be in the next PR). There are only a few examples of buildflags in components in upstream and it doesn't specify in the component docs

browser/ui/BUILD.gn Outdated Show resolved Hide resolved
browser/ui/webui/brave_settings_ui.cc Outdated Show resolved Hide resolved
browser/sources.gni Outdated Show resolved Hide resolved
browser/web_discovery/BUILD.gn Outdated Show resolved Hide resolved
return map;
}

std::optional<std::string> GunzipContents(std::string gzipped_contents) {
Copy link
Member

Choose a reason for hiding this comment

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

why this machinery is required? Is server missing Content-Encoding: gzip header?

URLLoader should handle this automatically. There's a test: SimpleURLLoaderTest, GzipBody.

Copy link
Collaborator Author

@DJAndries DJAndries Aug 8, 2024

Choose a reason for hiding this comment

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

Is server missing Content-Encoding: gzip header?

Yes, the server does not provide the header because it's serving a pre-gzipped file.

Copy link
Member

Choose a reason for hiding this comment

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

Is server missing Content-Encoding: gzip header?

Yes, the server does not provide the header because it's serving a pre-gzipped file.

Sorry, but that's not an excuse. Is it served from AWS/Cloudflare? Just set "Content-Encoding: gzip" on server, do not perform manual unzipping.

example how to set during aws s3 cp: https://github.com/brave/brave-variations/blob/55500374116ea45c0c1cd9e3cb2416326c80a285/.github/workflows/generate-test-seed.yml#L82

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that modification may have ramifications for the original extension, since it also performs manual gunzipping. will investigate

components/web_discovery/browser/util.cc Outdated Show resolved Hide resolved
server_config_loader_ = nullptr;
credential_manager_ = nullptr;

profile_prefs_->ClearPref(kWebDiscoveryNativeEnabled);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think Stop should clear kWebDiscoveryNativeEnabled. Other prefs cleanup is understandable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

using Start and Stop seems confusing to me. Shouldn't this be Enable and Disable? As mentioned above having separate prefs for native and extension seems unnecessarily complex and confusing.

Copy link
Collaborator Author

@DJAndries DJAndries Aug 8, 2024

Choose a reason for hiding this comment

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

i think the current names are appropriate. Start also gets called on profile startup, so the name Enable doesn't seem correct.

components/web_discovery/common/features.cc Show resolved Hide resolved
@@ -85,7 +85,8 @@ inline constexpr char kBraveShieldsSettingsVersion[] =
inline constexpr char kDefaultBrowserPromptEnabled[] =
"brave.default_browser_prompt_enabled";

inline constexpr char kWebDiscoveryEnabled[] = "brave.web_discovery_enabled";
inline constexpr char kWebDiscoveryExtensionEnabled[] =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit confused here, usages of kWebDiscoveryExtensionEnabled are guarded by extensions enabled (in some places and missing in others), but this is not so why are there guards for the pref itself?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems there's no reason to have a guard around the kWebDiscoveryExtensionEnabled pref and the guards are also inconsistent and sometimes include the native pref so I think we should just drop them. Guards should generally be consistent between gn and cpp so if neither this file nor the pref declaration itself is guarded by enable extensions then access to it shouldn't be guarded by extensions enabled either

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i added the missing guards in two places. in general, if there is a guard around both the extension pref and the native pref, it's for pref migration

Copy link
Collaborator

Choose a reason for hiding this comment

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

but there's no reason for this added complexity and there are still missing guards. Also this pref should move out of components/constants and into web_discovery.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added more guards. also decided to keep the pref here since it's shared between the extension and the native component. since the native component is behind a separate flag, it didn't seem like the best place for the shared pref.

# You can obtain one at https://mozilla.org/MPL/2.0/.

declare_args() {
enable_web_discovery_native = is_win || is_mac || is_linux || is_android
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not ios?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ios will not be supported initially


void WebDiscoveryService::RegisterLocalStatePrefs(
PrefRegistrySimple* registry) {
registry->RegisterTimePref(kPatternsRetrievalTime, {});
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this a pref? Is it configurable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the value of the pref is managed by the ServerConfigLoader. it refers to the last retrieval time for the patterns file, and is used for scheduling the next patterns retrieval request.

@DJAndries DJAndries force-pushed the wdp-native-cred-mgr-srv-config branch from 4f11211 to 40d9461 Compare August 8, 2024 06:48
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

components/web_discovery/browser/credential_manager.h Outdated Show resolved Hide resolved
components/web_discovery/browser/patterns.cc Outdated Show resolved Hide resolved
components/web_discovery/browser/patterns.cc Show resolved Hide resolved
components/web_discovery/browser/patterns.cc Show resolved Hide resolved
return map;
}

std::optional<std::string> GunzipContents(std::string gzipped_contents) {
Copy link
Member

Choose a reason for hiding this comment

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

Is server missing Content-Encoding: gzip header?

Yes, the server does not provide the header because it's serving a pre-gzipped file.

Sorry, but that's not an excuse. Is it served from AWS/Cloudflare? Just set "Content-Encoding: gzip" on server, do not perform manual unzipping.

example how to set during aws s3 cp: https://github.com/brave/brave-variations/blob/55500374116ea45c0c1cd9e3cb2416326c80a285/.github/workflows/generate-test-seed.yml#L82

components/web_discovery/browser/server_config_loader.cc Outdated Show resolved Hide resolved
components/web_discovery/browser/server_config_loader.cc Outdated Show resolved Hide resolved
components/web_discovery/browser/server_config_loader.cc Outdated Show resolved Hide resolved
components/web_discovery/browser/server_config_loader.cc Outdated Show resolved Hide resolved
components/web_discovery/browser/credential_manager.cc Outdated Show resolved Hide resolved
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@DJAndries DJAndries force-pushed the wdp-native-cred-mgr-srv-config branch from b0de36a to 8b00999 Compare October 30, 2024 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/storybook-url Deploy storybook and provide a unique URL for each build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants