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

Remove unsupported components from brave://components #7331

Merged
merged 8 commits into from
Dec 14, 2020
Merged

Remove unsupported components from brave://components #7331

merged 8 commits into from
Dec 14, 2020

Conversation

mherrmann
Copy link
Collaborator

@mherrmann mherrmann commented Dec 3, 2020 •
edited
Loading

Resolves brave/brave-browser#8709

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:

Open brave://components on Windows, Mac, Linux and Android and check that none of the components described in brave/brave-browser#8709 (comment) are visible. The only exception is CRLSet on Android which @pj said should actually be there.

@mherrmann mherrmann added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS labels Dec 3, 2020
@mherrmann mherrmann requested a review from a team as a code owner December 3, 2020 18:26
@mherrmann mherrmann self-assigned this Dec 3, 2020
@mherrmann
Copy link
Collaborator Author

The implementation I've committed works on Linux. Screenshot:

image

On the left is Brave Nightly 1.20.9, on the right my build. Note how exactly the components with version 0.0.0.0 are removed on the right.

Will check for other OSs next.

@mherrmann mherrmann removed the CI/skip-android Do not run CI builds for Android label Dec 4, 2020
@mherrmann
Copy link
Collaborator Author

Checked the list of components on the other OSs Windows, macOS and Android, updated the list of components to be removed and committed a fix that removes them.

@mherrmann mherrmann force-pushed the mherrmann-remove-unsupported-components branch 4 times, most recently from d06ce68 to a52403c Compare December 7, 2020 07:41
@mherrmann
Copy link
Collaborator Author

Tested my build on Linux and Android. Everything seems to work. I think this is ready to merge.

@jumde can you please review? My reasoning for removing the components I removed is given here.

@jumde jumde added the CI/run-network-audit Run network-audit label Dec 8, 2020
@jumde
Copy link
Contributor

jumde commented Dec 8, 2020

PR lgtm, added minor comments. @mherrmann - please re-run this PR with network-audit label to ensure we are not reaching out to any unknown hosts. @mkarolin @bridiver - can you please review. thanks!

@jumde jumde self-requested a review December 8, 2020 03:44
@mherrmann mherrmann force-pushed the mherrmann-remove-unsupported-components branch from a52403c to 18d4691 Compare December 8, 2020 05:14
@mihaiplesa mihaiplesa requested review from mkarolin, bridiver and bsclifton and removed request for bridiver and mkarolin December 8, 2020 06:21
@mherrmann
Remove unsupported components from brave://components
b9808d4 
 * Legacy TLS Deprecation Config
 * Federated Learning of Cohorts
 * Autofill States Data
 * Subresource Filter Rules
 * Crowd Deny
 * Certificate Error Assistant
 * Safety Tips
 * Origin Trials
 * Zxcvbn Data Dictionaries
@mherrmann
Correct copyright notice
6dcff90
@mherrmann
Reformat code to fix linter warnings
a6117ca
@mherrmann mherrmann force-pushed the mherrmann-remove-unsupported-components branch from 18d4691 to e8671fe Compare December 11, 2020 16:13
mkarolin
mkarolin reviewed Dec 11, 2020
View reviewed changes
chromium_src/components/component_updater/component_installer.cc Outdated
std::vector<uint8_t> hash;
installer_policy_->GetHash(&hash);
const std::string id = update_client::GetCrxIdFromPublicKeyHash(hash);
if (base::Contains(blacklisted_components, id.c_str())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just needs #include "base/containers/contains.h" now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean #include "base/stl_util.h" right?

Copy link
Collaborator Author

@mherrmann mherrmann Dec 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

(Did that in 78db7e9.)

mkarolin
mkarolin approved these changes Dec 11, 2020
View reviewed changes
Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

LGTM

@mihaiplesa mihaiplesa merged commit fe18335 into master Dec 14, 2020
@mihaiplesa mihaiplesa deleted the mherrmann-remove-unsupported-components branch December 14, 2020 08:38
@bsclifton
Copy link
Member

bsclifton commented Dec 14, 2020 •
edited
Loading

Nice work, @mherrmann 😄 The only feedback I'd have would be to squash commits into logical units (ex: squash the lint fix/missing include/etc with the actual fix associated)

edit: set missing milestone 😄

@bsclifton bsclifton added this to the 1.20.x - Nightly milestone Dec 14, 2020
@mherrmann
Copy link
Collaborator Author

Thank you Brian! I was already working on this but @mihaiplesa beat me to the merge ;-)

This was referenced Dec 23, 2020
Merged
Merged
@mihaiplesa mihaiplesa mentioned this pull request Dec 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jumde jumde jumde approved these changes

@mkarolin mkarolin mkarolin approved these changes

@bsclifton bsclifton Awaiting requested review from bsclifton

Assignees

@mherrmann mherrmann

Labels
CI/run-network-audit Run network-audit CI/skip-ios Do not run CI builds for iOS
Projects
None yet
Milestone
1.20.x - Release
Development

Successfully merging this pull request may close these issues.

Remove unsupported components in brave://components
5 participants
@mherrmann @jumde @bsclifton @mkarolin @mihaiplesa