_{reported by [reviewdog](https://github.com/reviewdog/reviewdog) :dog:}<br>[semgrep] A function returns the address of a stack variable, which will cause unintended program behavior, typically in the form of a crash.<br><br>Source: https://github.com/0xdea/semgrep-rules/blob/main/c/ret-stack-address.yaml<br><br><!-- Category: security --><br>Cc @thypon @kdenhartog

_{reported by [reviewdog](https://github.com/reviewdog/reviewdog) :dog:}<br>[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.<br/>Consider swapping Unretained for a weak reference.<br/>base::Unretained usage may be acceptable when a callback owner is guaranteed<br/>to be destroyed with the object base::Unretained is pointing to, for example:<br/><br/>- PrefChangeRegistrar<br/>- base::*Timer<br/>- mojo::Receiver<br/>- any other class member destroyed when the class is deallocated<br/><br><br>Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/chromium-uaf.yaml<br><br><!-- Category: security --><br>Cc @thypon @goodov @iefremov

this lazy caching of the value seems problematic to me as mentioned in the comment for the test, I think it would be better to do it in `RegisterLocalStatePrefs` so it always happens at browser startup and there is no way for the pref to change before reading it

this assertion has side effects that can affect the rest of the test because calling `IsWindowsRecallDisabled` does a lazy cache of the value

`block` -> `disabled` for consistency in the code. We can use "block" in the text for the user if we want, but it's easier to search the code if we have consistent naming there. Alternatively you could change it to block everywhere, but we're mostly using disabled right now

`kWindowsRecallDisabled` for naming consistency

please add comments to explain the difference between available (based on windows version) and current state below for `IsWindowsRecallDisabled`

_{reported by [reviewdog](https://github.com/reviewdog/reviewdog) :dog:}<br>[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.<br/>Consider swapping Unretained for a weak reference.<br/>base::Unretained usage may be acceptable when a callback owner is guaranteed<br/>to be destroyed with the object base::Unretained is pointing to, for example:<br/><br/>- PrefChangeRegistrar<br/>- base::*Timer<br/>- mojo::Receiver<br/>- any other class member destroyed when the class is deallocated<br/><br><br>Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/chromium-uaf.yaml<br><br><!-- Category: security --><br>Cc @thypon @goodov @iefremov <!-- __reviewdog__:ChBlMWZkMmRlOTZhYjBiNmZm -->

same as mentioned below, pick either enabled or blocked, do not use both in different contexts. That is very confusing and error prone

why do we need this check for OTR? This is already handled by the chromium code when the original method returns false https://github.com/brave/brave-core/pull/29251/files

this makes things unnecessarily confusing. Either use blocked or enabled, not both.

why are you using an out param here? https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs `Prefer using return values over output parameters` but also in particular this seems like a very odd usage of out params

why are you adding this method to `BraveContentBrowserClient` that provides yet another different way (optional bool) to check the same thing as mentioned here https://github.com/brave/brave-core/pull/29251/files#r2129771095. Why can't you just use the optional bool everywhere?

why do we have this method and also an enum with three states? That seems redundant?

the result of the test should not be dependent on the version of windows this test runs on. You should mock the version to different values so you can test it correctly across a range of versions

browser tests are substantially slower and use far more resources than unit tests. Please do not use them for things that can be unit tested. Even the "restart" test here is not correct and doesn't require a browser test because it's attached to a Profile, not the browser process. Closing the last window for a given profile will destroy it and the associated UserData. There really is no good way to cache it per-profile for the lifetime of the browser, but it seems like this is maybe unnecessary anyway

do you really need this service? Seems like it only exists to store the pref value at startup. I wonder if `BrowserContext::Set/GetUserData` can be used for simplicity.

`static_library`