-
Notifications
You must be signed in to change notification settings - Fork 920
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
Conversation
3b79ae5
to
44b9c35
Compare
A Storybook has been deployed to preview UI for the latest push |
pls file a ticket with a link to the spec |
44b9c35
to
3dae27c
Compare
A Storybook has been deployed to preview UI for the latest push |
3dae27c
to
491d9fe
Compare
A Storybook has been deployed to preview UI for the latest push |
491d9fe
to
62e0cc7
Compare
A Storybook has been deployed to preview UI for the latest push |
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.
We probably want another review for the strings, but I added my own suggestions.
app/brave_settings_strings.grdp
Outdated
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. |
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.
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™ |
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.
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.
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.
@rebron asked me to add trademarks. Rafael what do you think?
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.
Yes, confirmed with @bradleyrichter.
app/brave_settings_strings.grdp
Outdated
Enable uBlock Origin™ | ||
</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. |
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.
uBlock Origin is a Manifest V2 extension that provides powerful content blocking tools with lots of options for advanced users.
Enable uMatrix™ | ||
</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. |
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.
uMatrix is a Manifest V2 extension that provides point-and-click matrix-based firewall, with many privacy-enhancing tools.
app/brave_settings_strings.grdp
Outdated
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™ |
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.
AdGuard
app/brave_settings_strings.grdp
Outdated
Enable AdBlock™ | ||
</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. |
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.
AdGuard is a Manifest V2 extension that provides ad blocking, parental control options, and integration with other AdGuard service offerings.
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.
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 ®.
62e0cc7
to
86e42c3
Compare
browser_context != web_ui()->GetWebContents()->GetBrowserContext()) { | ||
return; | ||
} | ||
auto fnd = std::find_if(extensions_.begin(), extensions_.end(), |
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.
base::ranges::find
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.
patches/* and the C++ part lgtm
A Storybook has been deployed to preview UI for the latest push |
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.
The WebUI Polymer code looks good!
app/brave_settings_strings.grdp
Outdated
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. |
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.
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)?
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.
cc: @rmcfadden3 Any suggestions on the text. Also, @boocmp We can finalize text with a follow-up. I wouldn't consider this blocking.
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.
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; |
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: 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%; |
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: left
should be colocated with the other position
items
A Storybook has been deployed to preview UI for the latest push |
A Storybook has been deployed to preview UI for the latest push |
A Storybook has been deployed to preview UI for the latest push |
Resolves brave/brave-browser#28367
extensions_v2.mp4
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
wikinpm run lint
,npm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
ExtensionsManifestV2
feature enabled:Manifest V2 Extensions
section