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

add setAsDefaultProtocolClient for Linux #10670

Merged
merged 8 commits into from Oct 7, 2017

Conversation

Projects
None yet
3 participants
@codebytere
Member

codebytere commented Oct 2, 2017

Tracking progress for implementation of #6440

Todo:

  • Replace GetChromeVersionOfScript in SetAsDefaultProtocolClient and IsDefaultProtocolClient appropriately

@codebytere codebytere requested a review from electron/reviewers as a code owner Oct 2, 2017

@codebytere codebytere changed the title from [WIP} add setAsDefaultProtocolClient for Linux to [WIP] add setAsDefaultProtocolClient for Linux Oct 2, 2017

codebytere added some commits Oct 3, 2017

@codebytere codebytere changed the title from [WIP] add setAsDefaultProtocolClient for Linux to add setAsDefaultProtocolClient for Linux Oct 7, 2017

}
bool SetDefaultWebClient(const std::string& protocol) {
#if defined(OS_CHROMEOS)

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Oct 7, 2017

Member

We can probably not care about the CHROMEOS check

}
bool Browser::IsDefaultProtocolClient(const std::string& protocol,
mate::Arguments* args) {
mate::Arguments* args) {
#if defined(OS_CHROMEOS)

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Oct 7, 2017

Member

Same

base::Process process = base::LaunchProcess(argv, options);
close(devnull);
if (!process.IsValid())return false;

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Oct 7, 2017

Member

This spacing stood out for some reason, and now I can't unsee it 😆

std::vector<std::string> argv;
argv.push_back(kXdgSettings);
argv.push_back("set");
if (!protocol.empty()) {

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Oct 7, 2017

Member

If the protocol is empty do we still want to continue? If they didn't provide a protocol should we just stop trying?

This comment has been minimized.

@codebytere

codebytere Oct 7, 2017

Member

hrm yeah i suppose we can immediately return false if there's no protocol, otherwise continue

#endif
}
// Todo implement

This comment has been minimized.

@MarshallOfSound

This comment has been minimized.

@codebytere

codebytere Oct 7, 2017

Member

i'm not currently sure if it's possible to remove using xdg-settings sadly...docs don't reference anything about it and chromium doesn't implement any form of protocol removal

namespace atom {
const char kXdgSettings[] = "xdg-settings";
const char kXdgSettingsDefaultBrowser[] = "default-web-browser";

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Oct 7, 2017

Member

This doesn't appear to be used

codebytere added some commits Oct 7, 2017

}
bool Browser::IsDefaultProtocolClient(const std::string& protocol,
mate::Arguments* args) {
base::ThreadRestrictions::AssertIOAllowed();

This comment has been minimized.

@deepak1556

deepak1556 Oct 7, 2017

Member

This check is dummy, since DCHECKS are disabled currently. But it is not good to do IO operations on the UI thread. I suggest moving these to the FILE thread instead. Currently we have not started using the new task scheduler api of chromium, which would make scenarios like this easier to deal with. You can look at how the previous version of {set/check}DefaultProtocolClient was implemented in chromium using FILE thread https://codereview.chromium.org/2888693003 .

@codebytere

This comment has been minimized.

Member

codebytere commented Oct 7, 2017

Per @deepak1556's comment above, i'm planning to migrate implementations of SetDefaultProtocolClient and IsDefaultProtocolClient to the FILE thread, but i'll open a separate PR and migrate implementations at the same time on all three platforms

@codebytere codebytere merged commit 242e097 into master Oct 7, 2017

8 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-mas-x64 Build #5350 succeeded in 16 min
Details
electron-osx-x64 Build #5337 succeeded in 13 min
Details

@codebytere codebytere deleted the add-linuxdefaultprotocol branch Oct 7, 2017

@luciorubeens luciorubeens referenced this pull request Jan 23, 2018

Merged

Add support to Ark URI scheme [AIP-13] #536

4 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment