-
Notifications
You must be signed in to change notification settings - Fork 908
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
[HangoutsExtension] Remove hangouts extension #24594
[HangoutsExtension] Remove hangouts extension #24594
Conversation
9524281
to
73ed47d
Compare
6b1f4ac
to
d22460a
Compare
d22460a
to
8393fce
Compare
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 Could you please confirm whether we need to show a InfoBar message or similar to convey the users for removing this option. |
f5cb0a4
to
772901c
Compare
@jagadeshjai I checked with @rebron - no infobar needed! 😄 |
Everything here looks good to go! @goodov it's ready for your re-review after @jagadeshjai added the test case 😄👍 |
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.
lgtm with few nits
@@ -61,6 +61,7 @@ inline constexpr char kShieldsStatsBadgeVisible[] = | |||
inline constexpr char kAdControlType[] = "brave.ad_default"; | |||
inline constexpr char kGoogleLoginControlType[] = "brave.google_login_default"; | |||
inline constexpr char kWebTorrentEnabled[] = "brave.webtorrent_enabled"; | |||
// Deprecated |
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 don't think this comment is needed here. right now it looks like everything below is deprecated.
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.
@goodov How about adding right side of the definition?
// Deprecated | |
inline constexpr char kWebTorrentEnabled[] = "brave.webtorrent_enabled"; // Deprecated |
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.
Webtorrent is OK; it's the one below that is deprecated
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 yes, I just wanted to show it with example 😆
Resolves brave/brave-browser#39660
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 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:
Upgrade scenario
Snippet courtesy of https://fedi.simonwillison.net/@simon/112757810519145581
Hangouts
should be enabled by default. Toggle it to OFF. Then back to ON.Hangouts
is no longer in brave://settings/extensionsnkeimhogjdpnpccoofpliimaahmaaome
is no longer thereFresh profile
Same as
Upgrade scenario
- but skip to step 5