Fix BraveBrowserView::OnThemeChanged is called infinitely #924
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In the below situation, BraveBrowserView::OnThemeChanged() is called infinitely on linux. * Open normal window and private window and change theme * Open normal window and guest/tor window This might be the reason of unresponsiveness of windows icon and cpu spike.
|
Maybe I couldn't response review comment because of holidays. |
|
@petemill do you have any other review comments on this? |
petemill
approved these changes
Nov 20, 2018
|
Please indicate merge flag |
|
Approving uplift to @simonhong next time when requesting approval to the |
kjozwiak
added
uplift-approved/0.57.x-Beta
and removed
uplift-request/0.57.x-Beta
labels
bbondy
pushed a commit
that referenced
this pull request
Nov 27, 2018
Fix BraveBrowserView::OnThemeChanged is called infinitely
bbondy
pushed a commit
that referenced
this pull request
Nov 27, 2018
Fix BraveBrowserView::OnThemeChanged is called infinitely
18 tasks
bbondy
added a commit
that referenced
this pull request
Nov 30, 2018
bbondy [4:58 PM] trying to get tests working on 0.58.x and noticed this is failing for tests: #924 I noticed this is in 0.57.x and 0.58.x but it needs a dependency for which was added with this f929cbc now I'm just going to add the dep for now ,but I want to make sure this is not a problem and check in case we needed the other PR in those branches too or to revert 924 from them petemill [5:04 PM] hmm good spot. I don't think there is anything else that makes 924 depend on 774. We probably don't need 774 on 0.57 as it's not urgent. I don't know how urgent 924 is but it sounds like it could be if it was causing an infinite loop. So safest is probably to add the dep. bbondy [5:06 PM] great thx for confirming, added the dep
bbondy
added a commit
that referenced
this pull request
Dec 1, 2018
bbondy [4:58 PM] trying to get tests working on 0.58.x and noticed this is failing for tests: #924 I noticed this is in 0.57.x and 0.58.x but it needs a dependency for which was added with this f929cbc now I'm just going to add the dep for now ,but I want to make sure this is not a problem and check in case we needed the other PR in those branches too or to revert 924 from them petemill [5:04 PM] hmm good spot. I don't think there is anything else that makes 924 depend on 774. We probably don't need 774 on 0.57 as it's not urgent. I don't know how urgent 924 is but it sounds like it could be if it was causing an infinite loop. So safest is probably to add the dep. bbondy [5:06 PM] great thx for confirming, added the dep
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix brave/brave-browser#1643
Fix brave/brave-browser#1791
In the below situation, BraveBrowserView::OnThemeChanged() is called
infinitely on linux.
This might be the reason of unresponsiveness of windows icon and
cpu spike.
Instead of notifying to native theme observer in
BraveThemeService::OnThemeChanged(),BraveThemeService::OnPreferenceChanged()would be much better to prevent infiniteOnThemeChanged()calling.Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests) ongit rebase master(if needed).git rebase -ito squash commits (if needed).Test Plan:
yarn test brave_browser_tests --filter=BraveThemeServiceTest.NativeThemeObserverTestReviewer Checklist: