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

Fix a potential bug while setting brave as default #22766

Closed
Pavneet-Sing opened this issue May 10, 2022 · 2 comments · Fixed by brave/brave-core#13282
Closed

Fix a potential bug while setting brave as default #22766

Pavneet-Sing opened this issue May 10, 2022 · 2 comments · Fixed by brave/brave-core#13282
Assignees
Labels
bug OS/Android Fixes related to Android browser functionality priority/P3 The next thing for us to work on. It'll ride the trains. QA Pass - Android ARM QA Pass - Android Tab QA/Yes release-notes/include

Comments

@Pavneet-Sing
Copy link

Description

The BraveSetDefaultBrowserUtils.setBraveDefaultSuccess(); will never be executed as MONTHLY_CONTRIBUTION_REQUEST_CODE and DEFAULT_BROWSER_ROLE_REQUEST_CODE Both have same values. MONTHLY_CONTRIBUTION_REQUEST_CODE == DEFAULT_BROWSER_ROLE_REQUEST_CODE

        } else if (resultCode == RESULT_OK && requestCode == 36) {
            dismissRewardsPanel();

        } else if (resultCode == RESULT_OK
                && requestCode == 36) {
            BraveSetDefaultBrowserUtils.setBraveDefaultSuccess();
        }

so the setBraveDefaultSuccess will never be executed.

Actual result

Haven't tested this use case but looks like it can affect set default browser flow, where it will always trigger dismissRewardsPanel instead of setBraveDefaultSuccess.

Expected result

  • DEFAULT_BROWSER_ROLE_REQUEST_CODE should have a different value than MONTHLY_CONTRIBUTION_REQUEST_CODE constant
  • Alternately, Prefer to create a separate constant class for public constants (instead of in Utils file) and move BraveActivity's public constant to the constant file as well if suitable.

Issue reproduces how often

easy

Additional information

Not sure which constant value should be changed, though usually should be latter one.
cc: @tapanmodh, Tagging @srirambv to verify the flow, if required.

@Pavneet-Sing Pavneet-Sing added bug priority/P3 The next thing for us to work on. It'll ride the trains. QA/Yes release-notes/include OS/Android Fixes related to Android browser functionality labels May 10, 2022
@tapanmodh
Copy link

@Pavneet-Sing Nice catch! I'll fix it

@Uni-verse
Copy link
Contributor

Uni-verse commented Jun 10, 2022

Verification PASSED on Samsung Galaxy S21 & Tab S7 using

Brave	1.40.91 Chromium: 102.0.5005.78 (Official Build) beta (64-bit) 
Revision	df6dbb5a9fd82af3f567198af2eb5fb4876ef99c-refs/branch-heads/5005_59@{#3}
OS	Android 12; Build/SP1A.210812.016
  • Confirmed user gets prompted to select default browser after using the set default browser in 3-dot menu
  • Confirmed selecting brave from the default browser prompt will remove setting from menu.
  • Confirmed opening link in browser option will launch url in brave browser.
  • Confirmed user is able to make monthly contribution to publisher after setting brave as default.
1 2 3 4
screenshot-1654887435014 screenshot-1654887452734 screenshot-1654887603996 screenshot-1654888228919

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug OS/Android Fixes related to Android browser functionality priority/P3 The next thing for us to work on. It'll ride the trains. QA Pass - Android ARM QA Pass - Android Tab QA/Yes release-notes/include
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants