[PSST] Show infobar when the feature availability is detected for the website#34174
[PSST] Show infobar when the feature availability is detected for the website#34174vadimstruts wants to merge 22 commits intomasterfrom
Conversation
463f997 to
96efa72
Compare
📋 Code Owners Summary21 file(s) changed, 2 with assigned owners 2 team(s) affected: Owners and Their Files
|
|
A Storybook has been deployed to preview UI for the latest push |
| base::fixed_flat_set<std::string_view, 0>{}; | ||
| #endif | ||
|
|
||
| inline constexpr char kBravePsstHost[] = "psst"; |
There was a problem hiding this comment.
Move theses constants into the component that uses them. Changes in this file are deprecated and useless :)
| void AddLocalizedStrings(content::WebUIDataSource* source) { | ||
| DCHECK(source); | ||
| static constexpr webui::LocalizedString kLocalizedStrings[] = { | ||
| {"bravePsstDialogTitle", IDS_PSST_CONSENT_DIALOG_TITLE}, |
There was a problem hiding this comment.
Lets use the new strings literals generation system formatter_data="webui=<webui name>" in grdp?
Doc ref: https://sourcegraph.com/github.com/brave/brave-core/-/blob/docs/webui_strings_explainer.md?L1-2
browser/psst/psst_ui_presenter.cc
Outdated
| return GURL(kBraveUIPsstURL); | ||
| } | ||
|
|
||
| void PsstWebDialogDelegate::OnDialogClosed( |
There was a problem hiding this comment.
Looks like unused. If you plan to use this in the future, then use RegisterOnDialogClosedCallback instead.
browser/psst/psst_ui_presenter.cc
Outdated
|
|
||
| void PsstWebDialogDelegate::OnCloseContents(content::WebContents* /* source */, | ||
| bool* out_close_dialog) { | ||
| *out_close_dialog = true; |
There was a problem hiding this comment.
You can remove this method and use set_can_close(true) in the c-tor instead.
|
|
||
| namespace { | ||
|
|
||
| // inline constexpr char kUserScriptResultTaskItemUrlPropName[] = "url"; |
| void BravePsstDialogUI::CreatePsstConsentHandler( | ||
| ::mojo::PendingReceiver<psst::mojom::PsstConsentHelper> psst_consent_helper, | ||
| ::mojo::PendingRemote<psst::mojom::PsstConsentDialog> psst_consent_dialog) { | ||
| BrowserWindowInterface* const bwi = |
There was a problem hiding this comment.
uff, it's always a pain to bind webui to the browser or tab. I don't like getting any active browser, because it should be hard link between the dialog and the tab.
browser/ui/BUILD.gn
Outdated
| ] | ||
| } | ||
|
|
||
| if (enable_psst) { |
There was a problem hiding this comment.
lets move it into the separated build.gn ?
| #endif | ||
| #endif | ||
|
|
||
| #if BUILDFLAG(ENABLE_PSST) |
There was a problem hiding this comment.
lets use ForWebUI approach ?
|
Chromium major version is behind target branch (145.0.7632.120 vs 146.0.7680.32). Please rebase. |
#34174 (comment) #34174 (comment) Signed-off-by: Vadym Struts <vstruts@brave.com>
#34174 (comment) Signed-off-by: Vadym Struts <vstruts@brave.com>
#34174 (comment) #34174 (comment) Signed-off-by: Vadym Struts <vstruts@brave.com>
#34174 (comment) #34174 (comment) Signed-off-by: Vadym Struts <vstruts@brave.com>
#34174 (comment) #34174 (comment) Signed-off-by: Vadym Struts <vstruts@brave.com>
#34174 (comment) Signed-off-by: Vadym Struts <vstruts@brave.com>
#34174 (comment) Signed-off-by: Vadym Struts <vstruts@brave.com>
Signed-off-by: Vadym Struts <vstruts@brave.com>
Signed-off-by: Vadym Struts <vstruts@brave.com>
Signed-off-by: Vadym Struts <vstruts@brave.com>
…ct to the next one Signed-off-by: Vadym Struts <vstruts@brave.com>
Signed-off-by: Vadym Struts <vstruts@brave.com>
Signed-off-by: Vadym Struts <vstruts@brave.com>
Signed-off-by: Vadym Struts <vstruts@brave.com>
Signed-off-by: Vadym Struts <vstruts@brave.com>
Signed-off-by: Vadym Struts <vstruts@brave.com>
Signed-off-by: Vadym Struts <vstruts@brave.com>
#34174 (comment) #34174 (comment) Signed-off-by: Vadym Struts <vstruts@brave.com>
#34174 (comment) Signed-off-by: Vadym Struts <vstruts@brave.com>
#34174 (comment) #34174 (comment) Signed-off-by: Vadym Struts <vstruts@brave.com>
#34174 (comment) #34174 (comment) Signed-off-by: Vadym Struts <vstruts@brave.com>
#34174 (comment) #34174 (comment) Signed-off-by: Vadym Struts <vstruts@brave.com>
#34174 (comment) Signed-off-by: Vadym Struts <vstruts@brave.com>
#34174 (comment) Signed-off-by: Vadym Struts <vstruts@brave.com>
369893a to
f458ade
Compare
fallaciousreasoning
left a comment
There was a problem hiding this comment.
sorry for the drive by review!
|
|
||
| function initialize() { | ||
| render( | ||
| <ThemeProvider theme={Theme}> |
There was a problem hiding this comment.
nit: we shouldn't need this ThemeProvider as this shouldn't be using any brave-ui
There was a problem hiding this comment.
I will fix it in follow up, as I removed consent dialog from the current PR
| // License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
| // You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
|
||
| import * as mojom from 'gen/brave/components/psst/common/psst_ui_common.mojom.m.js' |
There was a problem hiding this comment.
There's a new pattern for binding mojo handlers from Brave frontend code. PTAL at https://github.com/brave/brave-core/blob/5e7242311c1bf3478dffaeacb1224f021309638c/components/common/api/readme.md
There's also
#32899
if you want an example of an existing WebUI migrating to this new state manager
There was a problem hiding this comment.
I will fix it in follow up, as I removed consent dialog from the current PR
| <SettingGrid> | ||
| <SettingGridHeaderRow> | ||
| <div style={iconContainerStyle}> | ||
| {/* <img |
There was a problem hiding this comment.
nit: remove commented code
There was a problem hiding this comment.
I will fix it in follow up, as I removed consent dialog from the current PR
There was a problem hiding this comment.
I think this file can be removed now this is using the WebUI strings. Optionally you can add a strings.ts file like
S.MY_STRING instead of typing the full enum nameThere was a problem hiding this comment.
I will fix it in follow up, as I removed consent dialog from the current PR
| someProp: string | ||
| } | ||
|
|
||
| export default class PsstDlgContainer extends React.Component<Props, {}> { |
There was a problem hiding this comment.
nit: don't use class style components
There was a problem hiding this comment.
I will fix it in follow up, as I removed consent dialog from the current PR
| import styled from 'styled-components' | ||
| // import Button from '$web-components/button' | ||
|
|
||
| export const TextSection = styled('div')<{}>` |
There was a problem hiding this comment.
nit: don't need to specify empty args - additionally, if you're styling a built in element you can write
const Thing = styled.div`
/* styles */
`There was a problem hiding this comment.
I will fix it in follow up, as I removed consent dialog from the current PR
| ` | ||
| export const SettingGridRow = styled(LeftAlignedItem)` | ||
| padding: 16px; | ||
| border-bottom: 1px solid #eee; |
There was a problem hiding this comment.
probably there's a Nala token for this - possibly you want --leo-color-divider-subtle or -strong?
You can import
import { color} from '@brave/leo/tokens/css'for intellisense colors
There was a problem hiding this comment.
I will fix it in follow up, as I removed consent dialog from the current PR
| export const CheckBoxIconCompleted = styled(Icon)` | ||
| --leo-icon-color: #079235; | ||
| ` | ||
| export const CheckBoxIconFailed = styled(Icon)` | ||
| --leo-icon-color: #f10a0a; |
There was a problem hiding this comment.
can we not use the checkbox component?
There was a problem hiding this comment.
I will fix it in follow up, as I removed consent dialog from the current PR
| function initialize() { | ||
| render( | ||
| <ThemeProvider theme={Theme}> | ||
| <PsstDlgContainer someProp={'T'} /> |
There was a problem hiding this comment.
someProp='T'? was this for debugging?
There was a problem hiding this comment.
I will fix it in follow up, as I removed consent dialog from the current PR
| <part file="omnibox_strings.grdp" /> | ||
| <part file="playlist_strings.grdp" /> | ||
| <part file="../brave_account/resources/brave_account_strings.grdp" /> | ||
| <if expr="not (is_android or is_ios)"> |
There was a problem hiding this comment.
should this feature have its own buildflag?
There was a problem hiding this comment.
I will fix it in follow up, as I removed consent dialog from the current PR
|
A Storybook has been deployed to preview UI for the latest push |
|
also, in the linked video there's no clear delineation between the infobar and the webcontents - has the @brave/sec-team okay'd that? Additionally, this PR is nearly 3000 lines - is there some way we could split it up to be smaller? At the very least, its probably worth separating out the frontend/backend changes |
@ShivanKaul or @fmarier would be in charge of approving that |
@fallaciousreasoning see https://github.com/brave/reviews/issues/1299#issuecomment-3974240534 |
Signed-off-by: Vadym Struts <vstruts@brave.com> splitting the pr: remove browser test, will be returned back in follow up Signed-off-by: Vadym Struts <vstruts@brave.com>
ac659f7 to
74dd8b1
Compare
| script_result_dict.FindString(kPolicyScriptResultNextUrlPropName); | ||
| if (next_url && !next_url->empty()) { | ||
| // Go to next URL | ||
| web_contents()->GetController().LoadURL( |
There was a problem hiding this comment.
GURL(*next_url) is constructed from data returned by an injected script and passed directly to LoadURL without checking is_valid(). Invalid or malicious URLs from script results could cause unexpected behavior. best practice
I removed consent dialog from the PR |
|
[puLL-Merge] - brave/brave-core@34174 DescriptionThis PR adds UI presentation capabilities to the PSST (Privacy Settings Suggestions Tool) feature in Brave browser. It introduces an infobar-based consent flow that notifies users when PSST can optimize a site's privacy settings, allowing them to review suggestions before changes are applied. The PR also refactors the navigation observation mechanism (switching from Possible Issues
Security Hotspots
ChangesChanges
sequenceDiagram
participant User
participant WebContents
participant Observer as PsstTabWebContentsObserver
participant Registry as PsstRuleRegistry
participant UiDelegate as PsstUiDelegateImpl
participant Presenter as UiDesktopPresenter
participant InfoBar as PsstInfoBarDelegate
WebContents->>Observer: NavigationEntryCommitted()
Observer->>Observer: Set PsstNavigationData on entry
WebContents->>Observer: DocumentOnLoadCompleted()
Observer->>Registry: CheckIfMatch(url)
Registry-->>Observer: MatchedRule (user_script + policy_script)
Observer->>WebContents: Execute user_script
WebContents-->>Observer: OnUserScriptResult(user_id, tasks, site_name)
Observer->>UiDelegate: GetPsstWebsiteSettings(origin, user_id)
UiDelegate-->>Observer: nullopt (first visit)
Observer->>UiDelegate: Show(origin, settings, site_name, tasks, callback)
UiDelegate->>Presenter: ShowIcon()
UiDelegate->>Presenter: ShowInfoBar(callback)
Presenter->>InfoBar: Create(infobar_manager, callback)
InfoBar->>User: Display infobar
User->>InfoBar: Click "Review suggestions"
InfoBar->>Presenter: OnInfobarAccepted(true)
Presenter->>UiDelegate: OnUserAcceptedInfobar(origin, true)
UiDelegate->>Observer: ConsentCallback(disabled_checks=[])
Observer->>Observer: PrepareParametersForPolicyExecution()
Observer->>WebContents: Execute policy_script with params
WebContents-->>Observer: OnPolicyScriptResult(result)
alt Result has retry_counter
Observer->>Observer: MaybeGetPolicyScriptResult (poll localStorage)
else Result has psst data
Observer->>UiDelegate: UpdateTasks(progress, tasks, status)
UiDelegate->>UiDelegate: Notify Observers
Observer->>WebContents: LoadURL(next_url)
end
|
Resolves brave/brave-browser#52731
Added display of the infobar when PSST feature availability is detected for the website.
When the user clicks the Review button on the infobar, we plan to show a WebUI modal dialog, which will be added in follow up.
2026-02-25.14-19-11.mp4