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

Added 'Manifest V2 extensions' section in settings. #16983

Merged
merged 6 commits into from
Mar 10, 2023
Merged

Conversation

boocmp
Copy link
Contributor

@boocmp boocmp commented Feb 2, 2023

Resolves brave/brave-browser#28367

extensions_v2.mp4

image

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • 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 lint, 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:

  1. Launch Brave with ExtensionsManifestV2 feature enabled:
brave-browser --enable-features=ExtensionsManifestV2
  1. Go to brave://settings/extensions
  2. Click to Manifest V2 Extensions section
  3. You should see 4 extensions
  4. Enable/Disable toggles will install/uninstall corresponding extensions
@boocmp boocmp requested a review from iefremov February 2, 2023 12:06
@boocmp boocmp requested a review from a team as a code owner February 2, 2023 12:06
@github-actions github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false labels Feb 2, 2023
@brave-builds
Copy link
Collaborator

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

@iefremov
Copy link
Contributor

iefremov commented Feb 7, 2023

pls file a ticket with a link to the spec

@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

@brave-builds
Copy link
Collaborator

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

Copy link
Collaborator

@antonok-edm antonok-edm left a comment

Choose a reason for hiding this comment

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

We probably want another review for the strings, but I added my own suggestions.

Manifest V2 extensions
</message>
<message name="IDS_SETTINGS_MANAGE_EXTENSIONS_V2_SUBLABEL" desc="The sub label of manage v2 extensions link in settings">
The following Manifest V2 extensions are no longer available in the Chrome Web Store, but still supported in Brave. Click to download and install.
Copy link
Collaborator

Choose a reason for hiding this comment

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

They are technically still available in the Chrome Web Store and should be for much longer, but will be increasingly difficult to find there over time. I suggest rewording to something like:

The following Manifest V2 extensions will be removed from the Chrome Web Store, but are still supported in Brave. Click to download and install.

Close
</message>
<message name="IDS_SETTINGS_MANAGE_EXTENSIONS_V2_NO_SCRIPT_NAME" desc="Manifest V2 extension toggle's label">
Enable NoScript&#x2122;
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 not an expert on this kind of thing, just raising a potential concern:

Is it appropriate to be using for all of these? AdGuard maybe (although they use © on their website), but the others are released under GPLv3 and don't appear to have any registered trademark.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rebron asked me to add trademarks. Rafael what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, confirmed with @bradleyrichter.

Enable uBlock Origin&#x2122;
</message>
<message name="IDS_SETTINGS_MANAGE_EXTENSIONS_V2_UBLOCK_ORIGIN_DESC" desc="Manifest V2 extension toggle's sub label">
uBlock Origin is a Manifest V2 extension that provides ad blocking features and controls specific to uBlock.
Copy link
Collaborator

Choose a reason for hiding this comment

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

uBlock Origin is a Manifest V2 extension that provides powerful content blocking tools with lots of options for advanced users.

Enable uMatrix&#x2122;
</message>
<message name="IDS_SETTINGS_MANAGE_EXTENSIONS_V2_UMATRIX_DESC" desc="Manifest V2 extension toggle's sub label">
uMatrix is a Manifest V2 extension that provides point-and-click matrix-based firewall, with many privacy-enhancing tools.
Copy link
Collaborator

Choose a reason for hiding this comment

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

uMatrix is a Manifest V2 extension that provides point-and-click matrix-based firewall, with many privacy-enhancing tools.

uMatrix is a Manifest V2 extension that provides point-and-click matrix-based firewall, with many privacy-enhancing tools.
</message>
<message name="IDS_SETTINGS_MANAGE_EXTENSIONS_V2_ADGUARD_NAME" desc="Manifest V2 extension toggle's label">
Enable AdBlock&#x2122;
Copy link
Collaborator

Choose a reason for hiding this comment

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

AdGuard

Enable AdBlock&#x2122;
</message>
<message name="IDS_SETTINGS_MANAGE_EXTENSIONS_V2_ADGUARD_DESC" desc="Manifest V2 extension toggle's sub label">
AdGuard is a Manifest V2 extension that effectively blocks all types of ads on all web pages, even on Facebook, YouTube and others.
Copy link
Collaborator

Choose a reason for hiding this comment

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

AdGuard is a Manifest V2 extension that provides ad blocking, parental control options, and integration with other AdGuard service offerings.

Copy link
Contributor

Choose a reason for hiding this comment

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

TM is useful to identify these extension names as "not Brave's thing" and TM has loose rules for usage. Any company can claim a TM but it carries little value without official registration which then earns you an ®.

browser_context != web_ui()->GetWebContents()->GetBrowserContext()) {
return;
}
auto fnd = std::find_if(extensions_.begin(), extensions_.end(),
Copy link
Contributor

Choose a reason for hiding this comment

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

base::ranges::find

Copy link
Contributor

@iefremov iefremov left a comment

Choose a reason for hiding this comment

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

patches/* and the C++ part lgtm

@brave-builds
Copy link
Collaborator

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

Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

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

The WebUI Polymer code looks good!

Manifest V2 extensions
</message>
<message name="IDS_SETTINGS_MANAGE_EXTENSIONS_V2_SUBLABEL" desc="The sub label of manage v2 extensions link in settings">
The following Manifest V2 extensions will be removed from the Chrome Web Store, but are still supported in Brave. Click to download and install.
Copy link
Member

@petemill petemill Mar 7, 2023

Choose a reason for hiding this comment

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

Should this be "toggle to download and install, or to uninstall" (not focusing on the word "click" and also conveying that it will be uninstalled when untoggled)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

cc: @rmcfadden3 Any suggestions on the text. Also, @boocmp We can finalize text with a follow-up. I wouldn't consider this blocking.

Choose a reason for hiding this comment

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

Per Slack convo with @rebron , let's go with this:

The following Manifest V2 extensions will be removed from the Chrome Web Store, but still be supported in Brave. Toggle on an individual extension to download / install it, or toggle off to uninstall / remove.

@@ -0,0 +1,56 @@
<style include="cr-shared-style">
#errorMessage {
align-items: center;
Copy link
Member

Choose a reason for hiding this comment

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

nit: align-items should be colocated with display: flex, flex-direction, and gap (or anything which affects children's layout

display: flex;
flex-direction: row;
gap: 16px;
left: 50%;
Copy link
Member

Choose a reason for hiding this comment

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

nit: left should be colocated with the other position items

@brave-builds
Copy link
Collaborator

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

@boocmp boocmp enabled auto-merge (squash) March 8, 2023 10:45
@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

@boocmp boocmp merged commit db91beb into master Mar 10, 2023
@boocmp boocmp deleted the ext_manifest_v2 branch March 10, 2023 04:17
@github-actions github-actions bot added this to the 1.51.x - Nightly milestone Mar 10, 2023
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 potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false
8 participants