Added ms-edge protocol handler option #10879

simonhong · 2021-11-04T12:29:04Z

Applied below FF's implementation to set default handler w/o using windows system ui.
It calculated hash value by using some properties such as user sid, protocol or timestamp.
If this method is not available, system ui will be launched instead.
https://github.com/mozilla/gecko-dev/blob/master/toolkit/mozapps/defaultagent/SetDefaultBrowser.cpp

Resolves

Option state when brave is not yet set for ms-edge protocol handler

Option state when brave is ms-edge protocol handler

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_unit_tests -- --filter=DefaultProtocolHandlerUtilsWinTest

Test 1

Check this browser is not a default ms-edge protocol handler from control panel
Launch browser and load brave://settings/system/
Check Make default button is visible and click
Check button is disappeared
Search any keyword from windows taskbar and bing url is loaded in this browser
Change default handler to another browser like edge from control panel
Check brave://settings/system/ is updated properly

Test 2

Check this browser is not a default ms-edge protocol handler from control panel
Launch browser with --use-system-ui-for-ms-edge cmd line switch
load brave://settings/system/ and click button
Check system ui is launched for setting default apps for protocol

NOTE: With Older than Windows 10 RS2, system ui will be launched.

simonhong · 2021-11-04T12:31:22Z

@rebron Needs above options's all texts.

goodov · 2021-11-10T12:41:07Z

this is crazy! (I love it) 😀

browser/default_protocol_handler_utils_win.cc

simonhong

@goodov @bsclifton Comments are all addressed. PTAL again!

browser/default_protocol_handler_utils_win.cc

browser/ui/webui/new_tab_page/ms_edge_protocol_message_handler.cc

As ms will not allow setting 3p applications as a default ms-edge protocol handler, this option should not be visible from that version (Win11, 22494 build). See https://www.ctrl.blog/entry/microsoft-edge-protocol-competition.html

bsclifton

Code looks great and works perfectly. After setting to default, it hides the button. When default is changed away from Brave, button re-appears. Thanks for putting in the Windows 11 version check for 22494 😄

rmcfadden3 · 2021-11-16T18:12:56Z

@simonhong & @rebron — text lgtm. No changes.

simonhong · 2021-11-16T22:49:07Z

Merged as only upload step was failed in CI.

Added ms-edge protocol handler option

kjozwiak · 2021-11-23T22:17:45Z

Verification PASSED on Win 11 x64 using the following build:

Brave | 1.34.31 Chromium: 96.0.4664.45 (Official Build) nightly (64-bit)
-- | --
Revision | 76e4c1bb2ab4671b8beba3444e61c0f17584b2fc-refs/branch-heads/4664@{#947}
OS | Windows 11 Version 21H2 (Build 22000.318)

Test Case #1

ensured that Default microsoft-edge protocol handler under brave://settings/system displays a Make default when the MICROSOFT-EDGE protocol is set to another browser
ensured that selecting Make default changes MICROSOFT-EDGE to Brave under Apps -> Default apps -> Brave Nightly
ensured that the Make default isn't visible under brave://settings/system when Brave is set as the default application for the MICROSOFT-EDGE protocol
ensured that opening links within the task manager correctly opens Brave when it's not running
ensured that opening links within the task manager correctly opens a new tab within Brave when it's already running

`Brave not ms-edge default`	`Button visible when not default`	`After clicking button`	`Brave as default`

Test Case #2

launched `Brave using the following terminal switches as specified via Added ms-edge protocol handler option #10879 (comment)

brave.exe --enable-logging=stderr --use-system-ui-for-ms-edge

ensured that clicking on Make Default under brave://settings/system launched the Default app system UI

Verification PASSED on Win 10 x64 using the following build:

Test Case #1

ensured that Default microsoft-edge protocol handler under brave://settings/system displays a Make default when the MICROSOFT-EDGE protocol is set to another browser
ensured that selecting Make default changes MICROSOFT-EDGE to Brave under Apps -> Default apps -> Brave Nightly
ensured that the Make default isn't visible under brave://settings/system when Brave is set as the default application for the MICROSOFT-EDGE protocol
ensured that opening links within the task manager correctly opens Brave when it's not running
ensured that opening links within the task manager correctly opens a new tab within Brave when it's already running

`Brave not ms-edge default`	`Button visible when not default`	`After clicking button`	`Brave as default`

Test Case #2

launched `Brave using the following terminal switches as specified via Added ms-edge protocol handler option #10879 (comment)

brave.exe --enable-logging=stderr --use-system-ui-for-ms-edge

ensured that clicking on Make Default under brave://settings/system launched the Default app system UI

kjozwiak · 2021-11-23T22:19:23Z

While running through the verifications via #10879 (comment), I did run into the following:

However, this is a Windows limitation and much we can do. Once you click on OK, I never received the screen again. Even after switching the ms-edge protocol to Edge <-> Brave a few times.

bridiver · 2021-12-03T19:51:07Z

browser/ui/webui/new_tab_page/ms_edge_protocol_message_handler.cc

+
+void MSEdgeProtocolMessageHandler::OnRegValChanged() {
+  CheckMSEdgeProtocolDefaultHandlerState();
+  StartWatching();


this doesn't seem right, aren't we already watching it? What happens if you call StartWatching more than once on the same key?

I guess RegistryMonitor does this, but why aren't we using that?

Calling StartWatching from callback doesn't make any issue because it's onetime watching.

// To stop watching, delete this RegKey object. To continue watching the // object after the callback is invoked, call StartWatching again.

Well, RegistryMonitor seems installer specific (it inherits InstalledVersionMonitor interface).
And the logic for monitoring is quite simple.

github-actions bot assigned simonhong Nov 4, 2021

simonhong force-pushed the ms_edge_protocol_setting branch 2 times, most recently from 10e64f3 to 3a352f9 Compare November 5, 2021 02:49

simonhong changed the title ~~WIP: Added ms-edge protion setting~~ WIP: Added ms-edge protocol handler option setting Nov 5, 2021

simonhong changed the title ~~WIP: Added ms-edge protocol handler option setting~~ WIP: Added ms-edge protocol handler option Nov 5, 2021

simonhong force-pushed the ms_edge_protocol_setting branch 9 times, most recently from 0988b4d to 261efb9 Compare November 10, 2021 07:18

simonhong marked this pull request as ready for review November 10, 2021 12:18

simonhong requested review from a team as code owners November 10, 2021 12:18

simonhong requested review from bsclifton and petemill November 10, 2021 12:18

simonhong changed the title ~~WIP: Added ms-edge protocol handler option~~ Added ms-edge protocol handler option Nov 10, 2021

simonhong force-pushed the ms_edge_protocol_setting branch from c92206d to 9f379a1 Compare November 10, 2021 12:32

simonhong added the CI/skip Do not run CI builds (except noplatform) label Nov 11, 2021

simonhong force-pushed the ms_edge_protocol_setting branch from 9f379a1 to d45f6ea Compare November 11, 2021 05:53

simonhong removed the CI/skip Do not run CI builds (except noplatform) label Nov 11, 2021

simonhong requested a review from rebron November 11, 2021 06:10

goodov reviewed Nov 11, 2021

View reviewed changes

browser/default_protocol_handler_utils_win.cc Show resolved Hide resolved

goodov reviewed Nov 11, 2021

View reviewed changes

browser/default_protocol_handler_utils_win.cc Outdated Show resolved Hide resolved

Use namespace for default_protocol_handler apis

705530d

simonhong force-pushed the ms_edge_protocol_setting branch 3 times, most recently from 9c5de9f to b6a61ff Compare November 15, 2021 05:53

simonhong commented Nov 15, 2021

View reviewed changes

simonhong requested a review from goodov November 15, 2021 05:57

goodov reviewed Nov 15, 2021

View reviewed changes

browser/ui/webui/new_tab_page/ms_edge_protocol_message_handler.cc Outdated Show resolved Hide resolved

goodov approved these changes Nov 15, 2021

View reviewed changes

simonhong force-pushed the ms_edge_protocol_setting branch from b6a61ff to 30eea76 Compare November 15, 2021 06:27

simonhong requested a review from rmcfadden3 November 15, 2021 06:28

simonhong added 3 commits November 15, 2021 15:46

Used base::StringPrintf instead of c printf apis

0cede88

Used proper task traits

1dd14b3

Hide default ms edge protocol handler option

346a6f6

As ms will not allow setting 3p applications as a default ms-edge protocol handler, this option should not be visible from that version (Win11, 22494 build). See https://www.ctrl.blog/entry/microsoft-edge-protocol-competition.html

simonhong force-pushed the ms_edge_protocol_setting branch from 30eea76 to 346a6f6 Compare November 15, 2021 06:47

bsclifton approved these changes Nov 16, 2021

View reviewed changes

simonhong merged commit a866388 into master Nov 16, 2021

simonhong deleted the ms_edge_protocol_setting branch November 16, 2021 22:49

simonhong added this to the 1.34.x - Nightly milestone Nov 16, 2021

simonhong added a commit that referenced this pull request Nov 23, 2021

Merge pull request #10879 from brave/ms_edge_protocol_setting

a96ed2b

Added ms-edge protocol handler option

simonhong mentioned this pull request Nov 23, 2021

Set Brave as a default browser w/o system UI on Windows (uplift to 1.33.x) #11217

Merged

7 tasks

simonhong added a commit that referenced this pull request Nov 23, 2021

Merge pull request #10879 from brave/ms_edge_protocol_setting

89db54a

Added ms-edge protocol handler option

simonhong mentioned this pull request Nov 23, 2021

Set Brave as a default browser w/o system UI on Windows (uplift to 1.32.x) #11218

Merged

7 tasks

btlechowski mentioned this pull request Nov 24, 2021

Add entry in settings webui for configuring Brave as a default app for microsoft-edge protocol brave/brave-browser#17558

Closed

GeetaSarvadnya mentioned this pull request Nov 25, 2021

Brave isn't set as a default app for microsoft-edge protocol brave/brave-browser#19735

Closed

bridiver reviewed Dec 3, 2021

View reviewed changes

bsclifton mentioned this pull request Mar 9, 2022

Remove microsoft-edge protocol handler brave/brave-browser#21553

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ms-edge protocol handler option #10879

Added ms-edge protocol handler option #10879

simonhong commented Nov 4, 2021 •

edited

Loading

simonhong commented Nov 4, 2021

goodov commented Nov 10, 2021

simonhong left a comment

bsclifton left a comment

rmcfadden3 commented Nov 16, 2021

simonhong commented Nov 16, 2021

kjozwiak commented Nov 23, 2021 •

edited by stephendonner

Loading

kjozwiak commented Nov 23, 2021

bridiver Dec 3, 2021

bridiver Dec 3, 2021

simonhong Dec 4, 2021

simonhong Dec 4, 2021

Added ms-edge protocol handler option #10879

Added ms-edge protocol handler option #10879

Conversation

simonhong commented Nov 4, 2021 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Test 1

Test 2

simonhong commented Nov 4, 2021

goodov commented Nov 10, 2021

simonhong left a comment

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

rmcfadden3 commented Nov 16, 2021

simonhong commented Nov 16, 2021

kjozwiak commented Nov 23, 2021 • edited by stephendonner Loading

kjozwiak commented Nov 23, 2021

bridiver Dec 3, 2021

Choose a reason for hiding this comment

bridiver Dec 3, 2021

Choose a reason for hiding this comment

simonhong Dec 4, 2021

Choose a reason for hiding this comment

simonhong Dec 4, 2021

Choose a reason for hiding this comment

simonhong commented Nov 4, 2021 •

edited

Loading

kjozwiak commented Nov 23, 2021 •

edited by stephendonner

Loading