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

Handle microsoft-edge: protocol on Windows #9778

Merged
merged 5 commits into from
Aug 24, 2021
Merged

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Aug 18, 2021

When Brave is set as a default app for microsoft-edge protocol,
Windows will pass "microsoft-edge:XXX" as command line arguments.
This edge protocol includes url information.
To make browser load passed url included in edge protocol, browser
should parse ms-edge protocol args fetch url from it.
Then, new browser or existing browser window will load that url.

Resolves brave/brave-browser#13875

Security review: https://github.com/brave/security/issues/552

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=*MSEdgeProtocolTest*


Use the Windows Defaults program to set the microsoft-edge handler manually
image


Test 1 - from chrome uninstall process

  1. Set brave as a default app for microsoft-edge protocol
  2. Install chrome and then uninstall
  3. Check uninstall survey page is loaded in brave

Test 2 - search from windows task bar

  1. Set brave as a default app for microsoft-edge protocol
  2. Search any keyword from windows taskbar.
  3. Check bing search url is loaded in brave
  4. Search another keyword from windows taskbar
  5. Check another tab is created and new tab loads bing searcu url

Test 3 - uninstall

  1. Set brave as a default app for microsoft-edge protocol
  2. Uninstall brave
  3. Search any keyword from windows taskbar
  4. Check Windows asks users to set default app for microsoft-edge protocol again

@simonhong simonhong changed the title Handle microsoft-edge protocol from command line Handle microsoft-edge protocol Aug 18, 2021
@simonhong simonhong changed the title Handle microsoft-edge protocol Handle microsoft-edge: protocol on Windows Aug 18, 2021
@simonhong
Copy link
Member Author

simonhong commented Aug 19, 2021

Found some issue - If there is running browser instance, url in edge protocol args is not loaded. New window is created instead. Fixed.

@simonhong simonhong force-pushed the handle_ms_edge_protocol branch 2 times, most recently from 735cf40 to 1441f35 Compare August 19, 2021 07:50
@simonhong simonhong marked this pull request as ready for review August 19, 2021 08:21
@simonhong simonhong requested review from a team as code owners August 19, 2021 08:21
@simonhong simonhong force-pushed the handle_ms_edge_protocol branch 2 times, most recently from 17d42e5 to 15e9692 Compare August 20, 2021 08:44
browser/microsoft_edge_protocol_util.h Outdated Show resolved Hide resolved
browser/microsoft_edge_protocol_util.cc Outdated Show resolved Hide resolved
browser/microsoft_edge_protocol_util.cc Outdated Show resolved Hide resolved
browser/microsoft_edge_protocol_util.cc Outdated Show resolved Hide resolved
browser/microsoft_edge_protocol_util.cc Outdated Show resolved Hide resolved
browser/microsoft_edge_protocol_util.cc Outdated Show resolved Hide resolved
browser/microsoft_edge_protocol_util.cc Outdated Show resolved Hide resolved
When Brave is set as a default app for microsoft-edge protocol,
Windows will pass "microsoft-edge:XXX" as command line arguments.
This edge protocol includes url information.
To make browser load passed url included in edge protocol, browser
should parse url info from ms-edge protocol args and appends url
alone to command line again. Then, that url will be loaded during
the startup process.
With this, brave will be visible in the microsoft-edge target app list
in control panel.
BraveMainDelegate::BasicStartupComplete() is not called again when
browser is already running. So, it's not good place to handle command
line args. Instead, StartupBrowserCreatorImpl::Launch() is proper
place to get url from command line.
Copy link
Member

@goodov goodov left a comment

Choose a reason for hiding this comment

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

just some nits to fix

browser/microsoft_edge_protocol_util.h Outdated Show resolved Hide resolved
chromium_src/chrome/installer/util/shell_util.cc Outdated Show resolved Hide resolved
browser/microsoft_edge_protocol_util.cc Outdated Show resolved Hide resolved
Copy link
Member Author

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

All done. Thanks @goodov :)

browser/microsoft_edge_protocol_util.h Outdated Show resolved Hide resolved
chromium_src/chrome/installer/util/shell_util.cc Outdated Show resolved Hide resolved
browser/microsoft_edge_protocol_util.cc Outdated Show resolved Hide resolved
@simonhong simonhong added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Aug 24, 2021
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Build installer, installed, and gave it a full run! This works great 🙂 Really nice work here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow Brave to Handle Searches from Windows Shell and Cortana
3 participants