-
Notifications
You must be signed in to change notification settings - Fork 868
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
Remove brave vpn toolbar button flashes when reconnecting multiple times #20386
Conversation
4ca830d
to
2ca4313
Compare
952e45e
to
d234440
Compare
@@ -221,12 +227,14 @@ std::unique_ptr<views::Border> BraveVPNButton::GetBorder( | |||
} | |||
|
|||
void BraveVPNButton::UpdateColorsAndInsets() { | |||
const bool is_connect_error = IsConnectError(); | |||
is_error_state_ = is_connect_error; |
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 think UpdateColorsAndInsets()
is not good place to set is_error_state_
.
Could we do it in OnConnectionStateChanged()
?
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.
UpdateColorsAndInsets
is also called from OnPurchasedStateChanged
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.
How about having another method only for it and call it from both places?
As UpdateColorsAndInsets()
is for updating UI based on current state,
updating error state from it seems not appropriate.
And it should be called when purchased state is changed?
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.
done
bd6fe05
to
9ab0041
Compare
9ab0041
to
11c8aae
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.
chromium_src lgtm
Remove brave vpn toolbar button flashes when reconnecting multiple times
Resolves brave/brave-browser#33283
BraveVPNOSConnectionAPI
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: