Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add don't ask again checkbox to default browser prompt #8253

Merged
merged 2 commits into from
Apr 6, 2021

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Mar 15, 2021

Default browser prompt is disabled if it's checked.

fix brave/brave-browser#14469

image

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • 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, npm run lint, 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:

Test Plan:

npm run test brave_browser_tests -- --filter=*BraveLocalStatePref*

  1. Launch with clean profile
  2. Close welcome page and create NTP
  3. Re-launch and check default prompt is visible
  4. Re-launch more than 20 times and check prompt is not visible anymore (Previously, prompt was launched at 20th launch)
  5. Create new profile and close all other profiles
  6. Launch browser and check it uses new profile
  7. Check prompt is not launched also for this new profile

@simonhong simonhong self-assigned this Mar 15, 2021
@simonhong simonhong marked this pull request as draft March 16, 2021 01:44
@simonhong simonhong changed the title Add don't ask again checkbox to default browser prompt WIP: Add don't ask again checkbox to default browser prompt Mar 18, 2021
@simonhong simonhong changed the title WIP: Add don't ask again checkbox to default browser prompt Add don't ask again checkbox to default browser prompt Apr 5, 2021
@simonhong simonhong marked this pull request as ready for review April 5, 2021 01:12
@simonhong
Copy link
Member Author

PTAL :)

Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

#include "brave/grit/brave_generated_resources.h"
#include "chrome/browser/browser_process.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add a dep on //chrome/browser:browser_process?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

@karenkliu karenkliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Is there any way to use our checkbox and button component from the design system?

This dialog should look like this:
image

Default browser prompot is disabled if it's checked and user
will not see this prompt anymore.
This checking is stored in local state so new profile also can't see
this prompt dialog anymore.

fix brave/brave-browser#14469
@simonhong
Copy link
Member Author

simonhong commented Apr 5, 2021

Hi! Is there any way to use our checkbox and button component from the design system?

@karenkliu I'll work on it as a separate task because it's global native ui change.
Filed brave/brave-browser#15133

@karenkliu
Copy link

@simonhong Okay - thanks!

@simonhong simonhong requested a review from mkarolin April 6, 2021 01:53
Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

@simonhong
Copy link
Member Author

Merged because previous CI run was all green and last commit is just grd file description text change.

@simonhong simonhong merged commit de13afc into master Apr 6, 2021
@simonhong simonhong deleted the default_browser_prompt branch April 6, 2021 22:48
@simonhong simonhong added this to the 1.24.x - Nightly milestone Apr 6, 2021
@phineas12
Copy link

Why was this never fixed on Android as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

follow-up to #12203 add No button or Do not ask again option to show default browser dialog
5 participants