Fixed brave shiels panel in incognito. #29237
Merged
Conversation
This file contains hidden or 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
DJAndries
reviewed
Jun 2, 2025
| primary_pattern, ContentSettingsPattern::Wildcard(), | ||
| ContentSettingsType::BRAVE_SHIELDS, setting); | ||
|
|
||
| RecordShieldsToggled(local_state); |
There was a problem hiding this comment.
please ensure RecordShieldsToggled is not called for OTR profiles
There was a problem hiding this comment.
PTAL this commit, may be we can use the same approach for other components
DJAndries
approved these changes
Jun 2, 2025
boocmp
force-pushed
the
shields_panel
branch
2 times, most recently
from
bc90289 to
97d497a
Compare
|
I'm going to move the P3A changes into the separate PR. |
DJAndries
reviewed
Jun 4, 2025
| #define P3A_LAZY_CALLS(stream, is_enabled) \ | ||
| !(is_enabled) ? (void)0 : ::brave_shields::P3ACallStream::Voidify() & (stream) | ||
|
|
||
| #define P3A(context) \ |
There was a problem hiding this comment.
very clever, but imo this may be overkill given the relatively small amount of call sites. wdyt about just passing the map into the relevant "Record" functions and performing a simple OTR check inside each function?
There was a problem hiding this comment.
I've moved this to a separate Draft PR. This PR only adds an explicit check for RecordShieldsToggled.
|
Released in v1.81.45 |
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.
Resolves brave/brave-browser#46186
There were two functions,
SetBraveShieldsEnabledandResetBraveShieldsEnabled. The second function setCONTENT_SETTING_DEFAULTfor the content settings when shields were enabled on the page, whileSetBraveShieldsEnabledwould setCONTENT_SETTING_ALLOWin the same case. I've merged the logic ofResetBraveShieldsEnabledintoSetBraveShieldsEnabled.Now:
SetBraveShieldsEnabledsetsCONTENT_SETTING_DEFAULTwhen the user enables shields on the page in a regular profileSetBraveShieldsEnabledsetsCONTENT_SETTING_ALLOWwhen the user enables shields on the page in an off-the-record profile, becauseBRAVE_SHIELDScontent settings are registered to inherit values from the regular profile. Without explicitly settingALLOWin off-the-record profiles, the getter would fall back to the regular profile's settings, which cause incorrect behavior described in the issue.Other functions were also checked for similar issues. In cases where a problem arises, it has been confirmed that such a state cannot be reached from the UI, and
DCHECKs have been added for values that are unused (and should not be used).