Implement "Desktop Welcome Pages" feature #23289

zenparsing · 2024-04-25T19:38:27Z

Resolves brave/brave-browser#40157

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

components/brave_education/resources/brave_education.ts

brave-builds · 2024-04-25T20:38:31Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-04-30T17:37:11Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-05-07T20:01:20Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-05-07T22:27:57Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-05-09T12:38:41Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-05-09T15:07:32Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-05-20T13:17:01Z

A Storybook has been deployed to preview UI for the latest push

bridiver · 2024-08-05T20:39:19Z

browser/ui/BUILD.gn

@@ -228,6 +228,14 @@ source_set("ui") {
      "toolbar/brave_bookmark_sub_menu_model.cc",
      "toolbar/brave_bookmark_sub_menu_model.h",
      "toolbar/brave_recent_tabs_sub_menu_model.h",
+      "webui/brave_education/brave_browser_command_handler.cc",
+      "webui/brave_education/brave_browser_command_handler.h",
+      "webui/brave_education/brave_education_ui.cc",


these should ideally go in their own BUILD.gn unless there would be a circular dependency issue? I think there probably is one for brave_education_ui so it could go directly in webui/brave_education_ui and the rest of the code could go in webui/brave_education with a separate BUILD.gn

I took a look at the code in browser/ui/webui and none of those folders use their own BUILD.gn. Is this a new pattern that we want to establish?

it's not a new pattern, there is just a lot of incorrect code

browser/ui/webui/brave_education/brave_browser_command_handler.cc

bridiver · 2024-08-06T00:35:11Z

patches/chrome-browser-ui-webui-browser_command-browser_command_handler.cc.patch

+     case Command::kOpenSafetyCheckFromWhatsNew:
+       can_execute = true;
+       break;
+    default: break;


why do we need this? Shouldn't we only be calling the superclass if we don't handle it in our subclass?

That switch statement doesn't have a default clause, so when we extend the enum with our own Command values, it results in a compile-time error here.

oic, because there is an existing mojo enum we are extending

but this patch does not follow standard patching conventions. It should be BRAVE_CAN_EXECUTE_COMMAND and this should also include NOTREACHED_NORETURN

github-actions · 2024-08-07T22:09:51Z

browser/ui/webui/brave_education/getting_started_helper.cc

+  url_loader_->DownloadToString(
+      url_loader_factory.get(),
+      base::BindOnce(&GettingStartedHelper::OnURLResponse,
+                     base::Unretained(this), *content_type),


_{reported by reviewdog 🐶}
[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.
Consider swapping Unretained for a weak reference.
base::Unretained usage may be acceptable when a callback owner is guaranteed
to be destroyed with the object base::Unretained is pointing to, for example:

- PrefChangeRegistrar
- base::*Timer
- mojo::Receiver
- any other class member destroyed when the class is deallocated

Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/chromium-uaf.yaml

Cc @thypon @goodov @iefremov

brave-builds · 2024-08-07T23:10:58Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-08-08T22:51:19Z

A Storybook has been deployed to preview UI for the latest push

github-actions · 2024-08-09T19:02:49Z

[puLL-Merge] - brave/brave-core@23289

Here's my review of the pull request:

Description

This PR adds a new "Getting Started" feature to the Brave browser. It introduces a new WebUI page that displays educational content about Brave's features after the user completes the Welcome UX flow. The content is loaded from Brave's website into an iframe, and the page supports executing browser commands through a mojo interface.

Changes

Changes

New files in browser/ui/webui/brave_education/:
- brave_education_ui.cc/h: Implements the WebUI for the new education page.
- education_page_handler.cc/h: Handles requests from the WebUI page.
- getting_started_helper.cc/h: Helps determine the appropriate "getting started" WebUI URL.
New files in components/brave_education/:
- education_content_urls.cc/h: Defines URLs for education content.
- features.cc/h: Defines feature flags for the new functionality.
- Various new resource files for the WebUI implementation.
Modified browser/ui/webui/welcome_page/welcome_dom_handler.cc/h:
- Added functionality to get the "welcome complete" URL.
New files in browser/ui/webui/browser_command/:
- brave_browser_command_handler.cc/h: Implements Brave-specific browser commands.
Various build file updates to include the new components.
Updates to existing files:
- browser/brave_content_browser_client.cc: Registered new WebUI controller.
- browser/ui/BUILD.gn: Added new source files.
- browser/about_flags.cc: Added new feature entries.
- components/constants/webui_url_constants.h: Added new constants.

Possible Issues

The implementation relies on loading content from an external website (brave.com) into an iframe. This could potentially lead to issues if the website is unavailable or changes unexpectedly.
The new feature is controlled by a feature flag (kShowGettingStartedPage). Ensure proper testing is done with both enabled and disabled states.

Security Hotspots

brave_education_ui.cc: The implementation allows embedding of iframes from brave.com. While this is likely intentional, it's worth ensuring that proper security measures (like Content Security Policy) are in place to prevent potential XSS attacks.
message_data_reader.ts: This file parses messages from the iframe. Ensure that all input is properly validated to prevent potential security issues.
brave_browser_command_handler.cc: This file executes browser commands based on messages from the iframe. Make sure that proper access controls are in place to prevent unauthorized command execution.

Overall, the changes appear well-structured and follow Brave's coding conventions. The new functionality is modular and seems to integrate well with existing systems. However, careful testing should be done to ensure the new feature works as expected and doesn't introduce any security vulnerabilities.

brave-builds · 2024-08-09T20:01:26Z

A Storybook has been deployed to preview UI for the latest push

bridiver · 2024-08-19T21:55:58Z

browser/brave_content_browser_client.cc

@@ -220,6 +220,7 @@ using extensions::ChromeContentBrowserClientExtensionsPart;
 #if !BUILDFLAG(IS_ANDROID)
 #include "brave/browser/new_tab/new_tab_shows_navigation_throttle.h"
 #include "brave/browser/ui/geolocation/brave_geolocation_permission_tab_helper.h"
+#include "brave/browser/ui/webui/brave_education/brave_education_ui.h"


same as above, do not use os buildflags to guard features that are disabled on some platforms.

bridiver · 2024-08-19T21:57:15Z

test/BUILD.gn

@@ -405,7 +405,9 @@ test("brave_unit_tests") {
      "//brave/browser/ui/views/bookmarks/brave_bookmark_context_menu_unittest.cc",
      "//brave/browser/ui/views/frame/brave_contents_layout_manager_unittest.cc",
      "//brave/browser/ui/views/wallet_bubble_focus_observer_unittest.cc",
+      "//brave/browser/ui/webui/brave_education/getting_started_helper_unittest.cc",


do not add individual files here, these should go in their own target

browser/ui/webui/browser_command/brave_browser_command_handler.cc

bridiver · 2024-08-19T22:04:28Z

browser/ui/webui/browser_command/brave_browser_command_handler.cc

+    case Command::kOpenPrivacySettings:
+      return true;
+    case Command::kOpenVPNOnboarding:
+      return CanShowVPNBubble(profile_);


this seems a bit odd, it's just going to do nothing if the user clicks on the link for this?

bridiver · 2024-08-19T22:09:29Z

browser/ui/webui/brave_education/getting_started_helper.cc

+
+GettingStartedHelper::~GettingStartedHelper() = default;
+
+void GettingStartedHelper::GetEducationURL(GetEducationURLCallback callback) {


I don't quite understand what is going on here. Why are we making a remote request to determine what page to show?

At the end of the welcome flow, we determine which education content to show based on feature flag params. However, if the server URL is not available for some reason (e.g. the network is not available, or the website is messed up, etc.) then we don't want to send the user to a dead end. It would be better to bypass the education page and send them to the new tab page.

This class is responsible for returning the appropriate education content WebUI URL and ensuring to a reasonable degree that it will be successfully loaded.

A similar check is used by upstream for the What's New page in whats_new_fetcher.h.

brave-builds · 2024-09-09T22:41:34Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-09-09T23:00:08Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-09-10T00:29:18Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-10-17T16:40:52Z

A Storybook has been deployed to preview UI for the latest push

zenparsing requested a review from a team as a code owner April 25, 2024 19:38

zenparsing marked this pull request as draft April 25, 2024 19:38

github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) labels Apr 25, 2024

github-actions bot assigned zenparsing Apr 25, 2024

github-actions bot added the puLL-Merge label Apr 25, 2024

github-actions bot reviewed Apr 25, 2024

View reviewed changes

components/brave_education/resources/brave_education.ts Show resolved Hide resolved

github-actions bot added the needs-security-review label Apr 25, 2024

github-actions bot assigned bcaller, goodov, iefremov and thypon Apr 25, 2024

zenparsing force-pushed the ksmith-getting-started-pages branch from 102b6ee to 16c2ca6 Compare April 30, 2024 16:06

zenparsing removed the CI/storybook-url Deploy storybook and provide a unique URL for each build label Apr 30, 2024

zenparsing force-pushed the ksmith-getting-started-pages branch from 16c2ca6 to 4bf9fe7 Compare April 30, 2024 16:51

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Apr 30, 2024

zenparsing force-pushed the ksmith-getting-started-pages branch 2 times, most recently from c8e1e52 to 19b72aa Compare May 7, 2024 19:01

zenparsing force-pushed the ksmith-getting-started-pages branch from 19b72aa to 56cb238 Compare May 7, 2024 21:48

zenparsing force-pushed the ksmith-getting-started-pages branch from 56cb238 to b4e59a9 Compare May 9, 2024 11:51

zenparsing force-pushed the ksmith-getting-started-pages branch from b4e59a9 to b3a56cc Compare May 9, 2024 14:19

zenparsing force-pushed the ksmith-getting-started-pages branch from b3a56cc to 635ff1f Compare May 20, 2024 12:17

zenparsing force-pushed the ksmith-getting-started-pages branch from 635ff1f to d916306 Compare May 22, 2024 15:45

bridiver reviewed Aug 5, 2024

View reviewed changes

bridiver reviewed Aug 6, 2024

View reviewed changes

browser/ui/webui/brave_education/brave_browser_command_handler.cc Outdated Show resolved Hide resolved

bridiver reviewed Aug 6, 2024

View reviewed changes

zenparsing force-pushed the ksmith-getting-started-pages branch from 124daaa to 56f5042 Compare August 7, 2024 22:05

github-actions bot reviewed Aug 7, 2024

View reviewed changes

zenparsing force-pushed the ksmith-getting-started-pages branch from 56f5042 to 1ddd8a1 Compare August 8, 2024 21:13

zenparsing requested review from bsclifton and a team as code owners August 8, 2024 21:13

mihaiplesa approved these changes Aug 9, 2024

View reviewed changes

zenparsing force-pushed the ksmith-getting-started-pages branch from 1ddd8a1 to 9a0f8c8 Compare August 9, 2024 19:02

bridiver reviewed Aug 19, 2024

View reviewed changes

browser/ui/webui/browser_command/brave_browser_command_handler.cc Outdated Show resolved Hide resolved

bridiver reviewed Aug 19, 2024

View reviewed changes

zenparsing marked this pull request as draft September 9, 2024 21:35

zenparsing force-pushed the ksmith-getting-started-pages branch from 9a0f8c8 to f8a8568 Compare September 9, 2024 21:47

zenparsing force-pushed the ksmith-getting-started-pages branch 2 times, most recently from 71173f5 to fc2d37d Compare September 11, 2024 14:44

Implement "Desktop Welcome Pages" feature

af92608

zenparsing force-pushed the ksmith-getting-started-pages branch from fc2d37d to af92608 Compare October 17, 2024 15:39

zenparsing marked this pull request as ready for review October 17, 2024 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement "Desktop Welcome Pages" feature #23289

Implement "Desktop Welcome Pages" feature #23289

zenparsing commented Apr 25, 2024 •

edited

Loading

brave-builds commented Apr 25, 2024

brave-builds commented Apr 30, 2024

brave-builds commented May 7, 2024

brave-builds commented May 7, 2024

brave-builds commented May 9, 2024

brave-builds commented May 9, 2024

brave-builds commented May 20, 2024

bridiver Aug 5, 2024

zenparsing Aug 7, 2024

bridiver Aug 19, 2024

bridiver Aug 6, 2024

zenparsing Aug 7, 2024

bridiver Aug 19, 2024

bridiver Aug 19, 2024

github-actions bot Aug 7, 2024

brave-builds commented Aug 7, 2024

brave-builds commented Aug 8, 2024

github-actions bot commented Aug 9, 2024

Changes

brave-builds commented Aug 9, 2024

bridiver Aug 19, 2024

bridiver Aug 19, 2024

bridiver Aug 19, 2024

bridiver Aug 19, 2024

zenparsing Aug 20, 2024

brave-builds commented Sep 9, 2024

brave-builds commented Sep 9, 2024

brave-builds commented Sep 10, 2024

brave-builds commented Oct 17, 2024


		GettingStartedHelper::~GettingStartedHelper() = default;

		void GettingStartedHelper::GetEducationURL(GetEducationURLCallback callback) {

Implement "Desktop Welcome Pages" feature #23289

Are you sure you want to change the base?

Implement "Desktop Welcome Pages" feature #23289

Conversation

zenparsing commented Apr 25, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Apr 25, 2024

brave-builds commented Apr 30, 2024

brave-builds commented May 7, 2024

brave-builds commented May 7, 2024

brave-builds commented May 9, 2024

brave-builds commented May 9, 2024

brave-builds commented May 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Aug 7, 2024

Choose a reason for hiding this comment

brave-builds commented Aug 7, 2024

brave-builds commented Aug 8, 2024

github-actions bot commented Aug 9, 2024

Description

Changes

Possible Issues

Security Hotspots

brave-builds commented Aug 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brave-builds commented Sep 9, 2024

brave-builds commented Sep 9, 2024

brave-builds commented Sep 10, 2024

brave-builds commented Oct 17, 2024

zenparsing commented Apr 25, 2024 •

edited

Loading