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

Introduce Chrome extensions management APIs independent of Dev Tools Extensions #9918

Merged
merged 6 commits into from Aug 8, 2017

Conversation

Projects
None yet
6 participants
@alexstrat
Contributor

alexstrat commented Jul 3, 2017

With the objective to load some Chrome extensions in an Electron app, I introduce here an API to add/remove/get extensions. It's the same mechanism than the one used in addDevToolsExtension except the loaded extensions are not persisted.

  • documentation
  • tests

Related: #1498

Introduce extensions management APIs indépendant of Dev Tools Extensions
- introduce API BrowserWindow#[add,remove,get]Extension
- make [add,remove, get]DevToolsExtension use newly introduced API
- make the app persist only the extensions added via
#addDevToolsExtension

@alexstrat alexstrat changed the title from Introduce Chrome extensions management APIs independent of Dev Tools Extensions to [WIP] Introduce Chrome extensions management APIs independent of Dev Tools Extensions Jul 3, 2017

@alexstrat alexstrat changed the title from [WIP] Introduce Chrome extensions management APIs independent of Dev Tools Extensions to Introduce Chrome extensions management APIs independent of Dev Tools Extensions Jul 5, 2017

alexstrat added some commits Jul 5, 2017

@alexstrat

This comment has been minimized.

Show comment
Hide comment
@alexstrat

alexstrat Jul 5, 2017

Contributor

Not sure the build on Travis is failing for legit reasons 🤔

Contributor

alexstrat commented Jul 5, 2017

Not sure the build on Travis is failing for legit reasons 🤔

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Jul 14, 2017

Contributor

The implementation looks good to me but I'm wondering if it would be better to just augment the
existing addDevToolsExtension and removeDevToolsExtension API with a second options parameter where you could set something like {persist: false} to not persist the change.

And we could then add a persisted property to the objects returned from getDevToolsExtensions to be able to differentiate the two.

What do you think about that @alexstrat?

Contributor

kevinsawicki commented Jul 14, 2017

The implementation looks good to me but I'm wondering if it would be better to just augment the
existing addDevToolsExtension and removeDevToolsExtension API with a second options parameter where you could set something like {persist: false} to not persist the change.

And we could then add a persisted property to the objects returned from getDevToolsExtensions to be able to differentiate the two.

What do you think about that @alexstrat?

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Jul 14, 2017

Contributor

Not sure the build on Travis is failing for legit reasons 🤔

Yeah, I'd say don't worry about any flaky travis specs that are non-devtools related for your changes.

Contributor

kevinsawicki commented Jul 14, 2017

Not sure the build on Travis is failing for legit reasons 🤔

Yeah, I'd say don't worry about any flaky travis specs that are non-devtools related for your changes.

@alexstrat

This comment has been minimized.

Show comment
Hide comment
@alexstrat

alexstrat Jul 16, 2017

Contributor

@kevinsawicki Further than making optional the persistence behind addDevToolsExtension, a motivation behind the proposal is to have an API that officializes the support for Chrome extensions already living behind the implementation of addDevToolsExtension.

I know electron maintainers are apparently not willing to support Chrome extensions, so it'd make sense to chose a path that gets around officializing the support for Chrome extensions.
On one hand, I'd find it too bad because the already-existing implementation is a correct support of parts of Chrome extensions, but on the other hand, an official support is a commitment.

Happy to contribute towards a path to an official support of Chrome extensions — or towards simply tweaking the addDevToolsExtension with an option like you suggested. Your call! :)

Contributor

alexstrat commented Jul 16, 2017

@kevinsawicki Further than making optional the persistence behind addDevToolsExtension, a motivation behind the proposal is to have an API that officializes the support for Chrome extensions already living behind the implementation of addDevToolsExtension.

I know electron maintainers are apparently not willing to support Chrome extensions, so it'd make sense to chose a path that gets around officializing the support for Chrome extensions.
On one hand, I'd find it too bad because the already-existing implementation is a correct support of parts of Chrome extensions, but on the other hand, an official support is a commitment.

Happy to contribute towards a path to an official support of Chrome extensions — or towards simply tweaking the addDevToolsExtension with an option like you suggested. Your call! :)

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Jul 17, 2017

Contributor

Further than making optional the persistence behind addDevToolsExtension, a motivation behind the proposal is to have an API that officializes the support for Chrome extensions already living behind the implementation of addDevToolsExtension.

Thanks for clarifying this, please ignore my previous suggestion then 👍

Contributor

kevinsawicki commented Jul 17, 2017

Further than making optional the persistence behind addDevToolsExtension, a motivation behind the proposal is to have an API that officializes the support for Chrome extensions already living behind the implementation of addDevToolsExtension.

Thanks for clarifying this, please ignore my previous suggestion then 👍

@zcbenz

zcbenz approved these changes Jul 24, 2017

Looks good to me.

@jkleinsc jkleinsc merged commit ccdff72 into electron:master Aug 8, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@TimNZ

This comment has been minimized.

Show comment
Hide comment
@TimNZ

TimNZ Aug 8, 2017

@alexstrat Awesome. Please tell me you are going to implement the full chrome extension api! :)

TimNZ commented Aug 8, 2017

@alexstrat Awesome. Please tell me you are going to implement the full chrome extension api! :)

@bil88

This comment has been minimized.

Show comment
Hide comment
@bil88

bil88 Aug 18, 2017

@alexstrat One question, is it valid for any extension?
I am trying but after BrowserWindow.addExtension is already installed or must be installed?
On the other hand you have to pack as such the extension in the app or you can do a get from the store?

bil88 commented Aug 18, 2017

@alexstrat One question, is it valid for any extension?
I am trying but after BrowserWindow.addExtension is already installed or must be installed?
On the other hand you have to pack as such the extension in the app or you can do a get from the store?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment