feat: add app.getApplicationNameForProtocol API #20399

ajmacd · 2019-10-01T20:56:03Z

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

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR title follows semantic commit guidelines
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

notes: Added app.getApplicationNameForProtocol() API

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

docs/api/app.md

spec-main/api-app-spec.ts

shell/browser/browser_linux.cc

codebytere · 2019-10-01T21:30:37Z

shell/browser/browser_mac.mm

+  base::ScopedCFTypeRef<CFURLRef> openingApp(LSCopyDefaultApplicationURLForURL(
+      (CFURLRef)ns_url, kLSRolesAll, out_err.InitializeInto()));
+  if (out_err) {
+    // likely kLSApplicationNotFoundErr


let's LOG(ERROR) this error here?

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.

miniak · 2019-10-02T18:35:31Z

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?

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

…ctron into ajm-get-app-name-for-protocol

ajmacd · 2019-10-03T05:18:56Z

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 · 2019-10-08T16:51:58Z

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

ajmacd · 2019-10-08T16:52:35Z

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

ajmacd · 2019-10-17T18:30:37Z

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

zcbenz

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.

ajmacd · 2019-10-24T23:48:25Z

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

zcbenz · 2019-10-29T02:35:36Z

If no opposition I would like to land this.

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

ajmacd · 2019-10-29T07:03:34Z

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

codebytere

Approved on behalf of the API WG!

codebytere · 2019-11-05T18:24:28Z

@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'.

…for-protocol

ajmacd · 2019-11-06T19:47:45Z

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

release-clerk · 2019-11-07T01:50:36Z

Release Notes Persisted

Added app.getApplicationNameForProtocol() API

jkleinsc · 2019-11-13T21:35:52Z

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

jkleinsc · 2019-11-13T21:39:00Z

/trop run backport-to 8-x-y

trop · 2019-11-13T21:39:02Z

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

trop · 2019-11-13T21:39:14Z

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

* 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 · 2019-11-13T22:05:37Z

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

* 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.

ajmacd added 7 commits September 26, 2019 16:14

Add GetApplicationNameForProtocol.

27ad0e4

Fix Windows implementation.

6053e86

Fix up test.

e5dec8c

Add documentation.

22c7616

Implement for real on Linux using xdg-mime.

c286e13

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

Improve docs for Linux.

ec1a9ae

Clean up tests.

2a66cc8

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 1, 2019

Add a note about not relying on the precise format.

cd932ad

codebytere reviewed Oct 1, 2019

View reviewed changes

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

codebytere reviewed Oct 1, 2019

View reviewed changes

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

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 2, 2019

ajmacd and others added 5 commits October 2, 2019 21:40

Update docs/api/app.md

dc3f085

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

Remove needless done()s from tests.

b42b23c

Merge branch 'ajm-get-app-name-for-protocol' of github.com:ajmacd/ele…

3eb8113

…ctron into ajm-get-app-name-for-protocol

Use vector list initialization.

6d1ca11

Add a simple test for isDefaultProtocolClient.

5d2c6aa

jkleinsc added the api-review/requested 🗳 label Oct 8, 2019

Remove unneeded include and skip a test on Linux CI.

db467a4

zcbenz approved these changes Oct 18, 2019

View reviewed changes

zcbenz requested review from codebytere and miniak October 18, 2019 00:53

Merge remote-tracking branch 'upstream/master' into ajm-get-app-name-…

ec41020

…for-protocol Conflicts: shell/browser/browser_linux.cc

codebytere approved these changes Nov 5, 2019

View reviewed changes

miniak approved these changes Nov 5, 2019

View reviewed changes

ajmacd added 2 commits November 6, 2019 10:12

Merge remote-tracking branch 'upstream/master' into ajm-get-app-name-…

1cf63c1

…for-protocol

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

9525a01

codebytere merged commit 9b01bb0 into electron:master Nov 7, 2019

ajmacd deleted the ajm-get-app-name-for-protocol branch November 11, 2019 22:02

trop bot added the needs-manual-bp/8-x-y label Nov 13, 2019

ajmacd mentioned this pull request Nov 13, 2019

feat: add app.getApplicationNameForProtocol API (8-x-y) #21117

Merged

trop bot added in-flight/8-x-y and removed needs-manual-bp/8-x-y labels Nov 13, 2019

sofianguy added this to Fixed in 8.0.0-beta.3 in 8.2.x Nov 21, 2019

codebytere added merged/8-x-y and removed in-flight/8-x-y labels Jan 16, 2020

georgexu99 mentioned this pull request Jun 12, 2020

feat: add app.getApplicationInfoForProtocol API #24112

Merged

7 tasks

MarshallOfSound removed the api-review/requested 🗳 label Sep 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add app.getApplicationNameForProtocol API #20399

feat: add app.getApplicationNameForProtocol API #20399

ajmacd commented Oct 1, 2019 •

edited by miniak

codebytere Oct 1, 2019

ajmacd Oct 3, 2019

miniak commented Oct 2, 2019 •

edited

ajmacd commented Oct 3, 2019

ajmacd commented Oct 8, 2019

ajmacd commented Oct 8, 2019

ajmacd commented Oct 17, 2019

zcbenz left a comment

ajmacd commented Oct 24, 2019

zcbenz commented Oct 29, 2019

ajmacd commented Oct 29, 2019

codebytere left a comment

codebytere commented Nov 5, 2019

ajmacd commented Nov 6, 2019

release-clerk bot commented Nov 7, 2019

jkleinsc commented Nov 13, 2019

jkleinsc commented Nov 13, 2019

trop bot commented Nov 13, 2019

trop bot commented Nov 13, 2019

trop bot commented Nov 13, 2019

feat: add app.getApplicationNameForProtocol API #20399

feat: add app.getApplicationNameForProtocol API #20399

Conversation

ajmacd commented Oct 1, 2019 • edited by miniak

Description of Change

Checklist

Release Notes

codebytere Oct 1, 2019

Choose a reason for hiding this comment

ajmacd Oct 3, 2019

Choose a reason for hiding this comment

miniak commented Oct 2, 2019 • edited

ajmacd commented Oct 3, 2019

ajmacd commented Oct 8, 2019

ajmacd commented Oct 8, 2019

ajmacd commented Oct 17, 2019

zcbenz left a comment

Choose a reason for hiding this comment

ajmacd commented Oct 24, 2019

zcbenz commented Oct 29, 2019

ajmacd commented Oct 29, 2019

codebytere left a comment

Choose a reason for hiding this comment

codebytere commented Nov 5, 2019

ajmacd commented Nov 6, 2019

release-clerk bot commented Nov 7, 2019

jkleinsc commented Nov 13, 2019

jkleinsc commented Nov 13, 2019

trop bot commented Nov 13, 2019

trop bot commented Nov 13, 2019

trop bot commented Nov 13, 2019

ajmacd commented Oct 1, 2019 •

edited by miniak

miniak commented Oct 2, 2019 •

edited