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

chore(extensions): remove unused features json #25660

Merged
merged 2 commits into from Sep 30, 2020

Conversation

samuelmaddock
Copy link
Member

@samuelmaddock samuelmaddock commented Sep 27, 2020

Description of Change

A few of the extension features we include were copied from Chrome's feature json files. As such, they included support for contexts unsupported by Electron such as platform apps, hosted apps, and login screens (ChromeOS).

ref #19447

Checklist

Release Notes

Notes: none

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Sep 27, 2020
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Sep 28, 2020
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

Nice catch.

Was this a random find, or did you do something that led you to this? I'm wondering now if we include other unused pieces...

@samuelmaddock
Copy link
Member Author

Was this a random find, or did you do something that led you to this? I'm wondering now if we include other unused pieces...

I had been looking into how script contexts are classified by the extensions system. Different classifications cause different APIs to be injected/supported.

In this case, I was comparing which extension permissions are supported in Electron vs Chrome. Fwiw, I checked the other shell/common/extensions/api/_*_features.json files and this was the only one with unused features.

@codebytere
Copy link
Member

codebytere commented Sep 29, 2020

CI failure looks like a legit crash caused by this PR:

[529:0929/154713.804183:FATAL:complex_feature.cc(10)] Check failed: features->size() > 1UL (1 vs. 1)
#0 0x00005c6e93df base::debug::CollectStackTrace()
#1 0x00005c62af82 base::debug::StackTrace::StackTrace()
#2 0x00005c641fa7 logging::LogMessage::~LogMessage()
#3 0x00005c6429df logging::LogMessage::~LogMessage()
#4 0x00005c626f32 logging::CheckError::~CheckError()
#5 0x000060e094de extensions::ComplexFeature::ComplexFeature()
#6 0x00005c538517 extensions::AddElectronPermissionFeatures()
#7 0x000058cf1e8c electron::ElectronExtensionsAPIProvider::AddPermissionFeatures()
#8 0x000060e09189 extensions::ExtensionsClient::CreateFeatureProvider()
#9 0x000060e0a48c extensions::FeatureProvider::GetByName()

A few of the features we include were copied from Chrome's feature json files.
As such, they included a support for contexts unsupported by Electron such as
platform apps, hosted apps, and login screens (ChromeOS).

Simply put, this will generate extra code not relevant to Electron.
@samuelmaddock
Copy link
Member Author

@ckerr to elaborate further on unused APIs, the extensions system includes its own core collection of them which we use via

AddAPIProvider(std::make_unique<extensions::CoreExtensionsAPIProvider>());

This comes with a lot of API features via _api_features.json which we're not making full use of yet. My hope is to one day change that by possibly allowing a script context type to be configurable in Electron.

@samuelmaddock
Copy link
Member Author

@codebytere thanks, it should be good now. the mac build may need a restart

@codebytere codebytere merged commit 462de5f into electron:master Sep 30, 2020
@release-clerk
Copy link

release-clerk bot commented Sep 30, 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