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: flesh out the api for //extensions #21587

Merged
merged 5 commits into from Jan 13, 2020
Merged

feat: flesh out the api for //extensions #21587

merged 5 commits into from Jan 13, 2020

Conversation

nornagon
Copy link
Member

Description of Change

This adds a few more APIs to the //extensions system, which is still behind the enable_electron_extensions build flag.

No release notes because none of these features are available in released versions of Electron yet.

Ref #19447

Checklist

Release Notes

Notes: none

@nornagon
Copy link
Member Author

(cc @samuelmaddock fyi!)

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Dec 19, 2019
@samuelmaddock
Copy link
Member

This looks to be going in a good direction to me! 👍

The only opinion I have is about the getLoadedExtensions method name. I could see there being some confusion around whether loaded extensions includes disabled extensions. getAllExtensions might be more clear, but I'm not strongly opinionated about this.

@nornagon
Copy link
Member Author

Makes sense to me! Also updated the sketch over in #19447.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Dec 20, 2019
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

LGTM, but I think the API WG should weigh in on this. Also, would it make more sense to wait to add the unimplemented functions when they are actually implemented?

extension_system->RemoveExtension(extension_id);
}

void Session::EnableExtension(const std::string& extension_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we wait until these are implemented to add them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, seems reasonable. I added them here because it was easy to do them all together but no particular need to keep them in this PR.

@nornagon
Copy link
Member Author

nornagon commented Jan 9, 2020

LGTM, but I think the API WG should weigh in on this.

FWIW, none of these functions are available in any released version of Electron, because these are behind a build flag that is off by default. I agree that we should do API review of the proposed APIs for extensions, over in #19447. I don't think API review should block this PR though as it does not change any exposed APIs of Electron.

Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

I agree this PR does not need API-wg's approval since there is no change to public APIs.

@@ -96,7 +96,10 @@ class Session : public gin_helper::TrackableObject<Session>,
#endif

#if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS)
void LoadChromeExtension(const base::FilePath extension_path);
v8::Local<v8::Promise> LoadExtension(const base::FilePath extension_path);
Copy link
Member

Choose a reason for hiding this comment

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

Should be const base::FilePath&.

@zcbenz zcbenz requested a review from jkleinsc January 13, 2020 06:09
@release-clerk
Copy link

release-clerk bot commented Jan 13, 2020

No Release Notes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants