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

feat: add app.getApplicationNameForProtocol API #20399

Merged
merged 17 commits into from Nov 7, 2019

Conversation

@ajmacd
Copy link
Contributor

ajmacd commented Oct 1, 2019

Description of Change

This API returns the name of the application registered as the default handler for a protocol (e.g. irc://). We're going to use this functionality in Slack to enable a "native app linking" feature, whereby Slack apps will optionally provide URLs to native applications alongside http URLs, which we'll attempt to launch if the correct native application is installed on the user's machine.

It fits well with the other protocol functionality provided by the Browser class. GetApplicationNameForUrl is an existing method in Chromium:
https://chromium.googlesource.com/chromium/src/+/ed716f08f330dda84f38b7f40f3e381cd6af571a/chrome/browser/shell_integration.h#65

and my initial attempt was simply to call down to it. Sadly, this would appear to require depending on the entire //chrome/browser module, which brings in something like a highly undesirable 3k files. Instead I copied the implementations out to Electron, which again is consistent with some of the other methods having similar or identical implementations in Chromium:

  • SetAsDefaultProtocolClient
  • IsDefaultProtocolClient

The Chromium Linux implementation is fairly useless, simply returning "xdg-open":
https://chromium.googlesource.com/chromium/src/+/eaa08411bd1f60a4e9d4b3253ba4710ae05bff3c/chrome/browser/shell_integration_linux.cc#720

but I discovered a useful implementation relying on xdg-mime wasn't all that hard. I'd like to upstream this to Chromium later. (Again, we won't be relying on that implementation, but just to share the spoils as it were.) This also avoids some DCHECKs I hit around disallowed blocking calls on Linux, in both the new and existing functionality.

Checklist

Release Notes

notes: Added app.getApplicationNameForProtocol() API

docs/api/app.md Outdated Show resolved Hide resolved
spec-main/api-app-spec.ts Outdated Show resolved Hide resolved
spec-main/api-app-spec.ts Outdated Show resolved Hide resolved
shell/browser/browser_linux.cc Outdated Show resolved Hide resolved
base::ScopedCFTypeRef<CFURLRef> openingApp(LSCopyDefaultApplicationURLForURL(
(CFURLRef)ns_url, kLSRolesAll, out_err.InitializeInto()));
if (out_err) {
// likely kLSApplicationNotFoundErr

This comment has been minimized.

Copy link
@codebytere

codebytere Oct 1, 2019

Member

let's LOG(ERROR) this error here?

This comment has been minimized.

Copy link
@ajmacd

ajmacd Oct 3, 2019

Author Contributor

This is copied verbatim from Chromium, and I think there's some value in maintaining that. If you think the log here is worth that loss, I'm happy to add it. Let me know.

@nornagon nornagon changed the title feat: add app.GetApplicationNameForProtocol API feat: add app.getApplicationNameForProtocol API Oct 1, 2019
@miniak

This comment has been minimized.

Copy link
Contributor

miniak commented Oct 2, 2019

I am not sure if this API belongs to the app module, it does not operate on the current application instance. What about moving it to systemPreferences instead?

@electron-cation electron-cation bot removed the new-pr 🌱 label Oct 2, 2019
ajmacd and others added 5 commits Oct 3, 2019
Co-Authored-By: Shelley Vohr <codebytere@github.com>
…ctron into ajm-get-app-name-for-protocol
@ajmacd

This comment has been minimized.

Copy link
Contributor Author

ajmacd commented Oct 3, 2019

I am not sure if this API belongs to the app module, it does not operate on the current application instance. What about moving it to systemPreferences instead?

I can see your point, and in a vacuum, I agree, it seems like this new API would be a better fit for systemPreferences. However, it's closely related to the existing protocol APIs and I think they should be grouped together. Thoughts?

@ajmacd

This comment has been minimized.

Copy link
Contributor Author

ajmacd commented Oct 8, 2019

Hello fine reviewers: any more thoughts on my comments? Happy to discuss further if you disagree!

@ajmacd

This comment has been minimized.

Copy link
Contributor Author

ajmacd commented Oct 8, 2019

Sorry should have @codebytere and @miniak in the last comment ^ 🙇

@ajmacd

This comment has been minimized.

Copy link
Contributor Author

ajmacd commented Oct 17, 2019

@codebytere, @miniak any comments on this? I'd like to land it, but am happy to discuss further changes.

@zcbenz
zcbenz approved these changes Oct 18, 2019
Copy link
Member

zcbenz left a comment

The implementation looks solid to me.

I agree this API should be grouped together with other protocol APIs. We should probably move protocol APIs from app to systemPreferences as these APIs are more about changing system settings instead of changing things about the app itself. But for now I'm good keeping this API in app.

@zcbenz zcbenz requested review from codebytere and miniak Oct 18, 2019
@ajmacd

This comment has been minimized.

Copy link
Contributor Author

ajmacd commented Oct 24, 2019

@zcbenz would you be OK with landing this, or would you prefer to wait for @codebytere and @miniak's approval?

@zcbenz

This comment has been minimized.

Copy link
Member

zcbenz commented Oct 29, 2019

If no opposition I would like to land this.

@ajmacd Do you mind rebasing this branch on master as there is conflict?

@ajmacd

This comment has been minimized.

Copy link
Contributor Author

ajmacd commented Oct 29, 2019

I actually chatted quickly with @codebytere about this today. She mentioned wanting to bring it up at the API working group meeting.

…for-protocol

Conflicts:
	shell/browser/browser_linux.cc
Copy link
Member

codebytere left a comment

Approved on behalf of the API WG!

@miniak
miniak approved these changes Nov 5, 2019
@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented Nov 5, 2019

@ajmacd this needs a final conflict fix:

Unhandled exception in main spec runner: electron/spec-main/api-app-spec.ts(866,43): error TS2304: Cannot find name 'isCI'.
@ajmacd

This comment has been minimized.

Copy link
Contributor Author

ajmacd commented Nov 6, 2019

@codebytere I think we're good to go here. The build-linux failure is unrelated to this change.

@codebytere codebytere merged commit 9b01bb0 into electron:master Nov 7, 2019
7 of 9 checks passed
7 of 9 checks passed
build-linux Workflow: build-linux
Details
Artifact Comparison Changes Detected
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-mac Workflow: build-mac
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Nov 7, 2019

Release Notes Persisted

Added app.getApplicationNameForProtocol() API

@ajmacd ajmacd deleted the ajmacd:ajm-get-app-name-for-protocol branch Nov 11, 2019
@jkleinsc

This comment has been minimized.

Copy link
Contributor

jkleinsc commented Nov 13, 2019

Approved by the Releases WG for backport to 8-x-y

@jkleinsc

This comment has been minimized.

Copy link
Contributor

jkleinsc commented Nov 13, 2019

/trop run backport-to 8-x-y

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 13, 2019

The backport process for this PR has been manually initiated -
sending your commits to "8-x-y"!

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 13, 2019

I was unable to backport this PR to "8-x-y" cleanly;
you will need to perform this backport manually.

ajmacd added a commit to ajmacd/electron that referenced this pull request Nov 13, 2019
* Add GetApplicationNameForProtocol.

* Fix Windows implementation.

* Fix up test.

* Add documentation.

* Implement for real on Linux using xdg-mime.

Also ensure we allow blocking calls here to avoid errant DCHECKing.

* Improve docs for Linux.

* Clean up tests.

* Add a note about not relying on the precise format.

* Update docs/api/app.md

Co-Authored-By: Shelley Vohr <codebytere@github.com>

* Remove needless `done()`s from tests.

* Use vector list initialization.

* Add a simple test for isDefaultProtocolClient.

* Remove unneeded include and skip a test on Linux CI.

* We no longer differentiate between CI and non-CI test runs.
@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Nov 13, 2019

@ajmacd has manually backported this PR to "8-x-y", please check out #21117

jkleinsc added a commit that referenced this pull request Nov 14, 2019
* Add GetApplicationNameForProtocol.

* Fix Windows implementation.

* Fix up test.

* Add documentation.

* Implement for real on Linux using xdg-mime.

Also ensure we allow blocking calls here to avoid errant DCHECKing.

* Improve docs for Linux.

* Clean up tests.

* Add a note about not relying on the precise format.

* Update docs/api/app.md

Co-Authored-By: Shelley Vohr <codebytere@github.com>

* Remove needless `done()`s from tests.

* Use vector list initialization.

* Add a simple test for isDefaultProtocolClient.

* Remove unneeded include and skip a test on Linux CI.

* We no longer differentiate between CI and non-CI test runs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.