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.

Sign up for GitHub

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

Jump to bottom

Load WDP on demand #22756

Merged
merged 1 commit into from
Apr 2, 2024
Merged

Load WDP on demand #22756

merged 1 commit into from
Apr 2, 2024

Conversation

atuchin-m
Copy link
Collaborator

@atuchin-m atuchin-m commented Mar 25, 2024 •
edited
Loading

For brave/brave-browser#35645

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:

Check that WDP can load correctly:

  • enable Web Discovery Project in brave://settings
  • go to brave://inspect/#extensions and inspect Brave Extension Background page
  • reload the page via Ctrl + Shift (Cmd for Mac) + R.
  • check Network tabs, should be requests config?.. to https://collector.wdp.brave.com

@github-actions github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Mar 25, 2024
remusao
remusao reviewed Mar 25, 2024
View reviewed changes
components/brave_extension/extension/brave_extension/background/webDiscoveryProject.ts
@@ -2,13 +2,33 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// you can obtain one at https://mozilla.org/MPL/2.0/.
export {}
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 needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Otherwise declare let window: any; hack doesn't work

components/brave_extension/extension/brave_extension/background/webDiscoveryProject.ts Outdated
// after user has opted in to WDP. This will be especially relevant
// when all shields functionality is removed from this extension.
import { App } from 'gen/brave/web-discovery-project'
declare let window: any;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn’t this type exist already? We can extend its type to account for optional WDP attribute if needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not my code ;)
I suppose it's a general hack to redeclare window as any. Of course, better to use https://www.jamestharpe.com/typescript-extend-window/, but it's out of the PR's scope.

My bad, I've occasionally added ;

Copy link
Collaborator

Choose a reason for hiding this comment

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

I missed that, all good then :)

@brave-builds
Copy link
Collaborator

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

remusao
remusao approved these changes Mar 26, 2024
View reviewed changes
components/brave_extension/extension/brave_extension/background/webDiscoveryProject.ts Outdated
// after user has opted in to WDP. This will be especially relevant
// when all shields functionality is removed from this extension.
import { App } from 'gen/brave/web-discovery-project'
declare let window: any;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I missed that, all good then :)

@atuchin-m atuchin-m requested a review from petemill March 26, 2024 17:25
@atuchin-m atuchin-m marked this pull request as ready for review March 26, 2024 17:25
@brave-builds
Copy link
Collaborator

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

@atuchin-m atuchin-m merged commit 7fe7415 into master Apr 2, 2024
22 checks passed
@atuchin-m atuchin-m deleted the load-wdp-on-demand branch April 2, 2024 21:08
@atuchin-m atuchin-m added this to the 1.66.x - Nightly milestone Apr 3, 2024
@kjozwiak kjozwiak mentioned this pull request May 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@remusao remusao remusao approved these changes

@petemill petemill Awaiting requested review from petemill

Assignees

@atuchin-m atuchin-m

Labels
CI/storybook-url Deploy storybook and provide a unique URL for each build unverified-commits
Projects
None yet
Milestone
1.66.x - Release
Development

Successfully merging this pull request may close these issues.
3 participants
@atuchin-m @brave-builds @remusao