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: preliminary support for //extensions #17440

Merged
merged 62 commits into from Jul 24, 2019

Conversation

@samuelmaddock
Copy link
Member

commented Mar 18, 2019

Description of Change

Overview

These are the initial, exploratory changes for adding support for Chrome extensions using the //extensions dependency from Chromium's source code. This dependency is meant to allow embedders of Chromium the ability to add extension support for their project.

Within the Electron community, there are several projects with their own implementation of Chrome extensions. No such implementation is complete and ends up duplicating the work of other implementations.

The goal of these changes are to introduce the first steps towards a more complete, official implementation of Chrome extensions for Electron-based projects.

Summary of changes

What's included

  • Loading of unpackaged Chrome extension
  • Content script injection
  • World isolation (aka context isolation) of content scripts
  • Content script list in DevTools
  • Extension manifest parsing and validation

What's not included (yet)

  • Background scripts
  • DevTools extensions
  • Extension badges
  • Popup windows for extension badges

What won't be included

Usage

Currently an extension can be loaded by providing a --load-extension=/absolute/path/to/extension switch. This is planned to be superseded by JavaScript bindings prior to merging.

Architecture

The file and code additions to this PR are mostly sourced from the //src/extensions/shell project.

app_shell is an experimental project to build a minimal environment like content_shell.
The goal is to be able to run a v2 app and supply most of the chrome.* extension APIs without running the rest of Chrome.

In other words, the changes in this PR are mostly necessary boilerplate copied from app_shell to embed extensions.

app_shell implements support for Chrome Apps while we want support Chrome Extensions (differences). Luckily they both share a majority of the same core APIs. The main addition we required was to create an instance of SharedUserScriptMaster. The history of commits provide a good timeline of APIs added.

Below are some of the notable classes added or changed as part of embedding extensions.

Browser

The entry point of changes for the browser process can be found in atom_browser_main_parts.cc.

Class Description
SharedUserScriptMaster Manages shared memory for communicating content scripts changes between the browser and renderer processes.
AtomExtensionSystem Initializes core extension APIs for the browser process.
Responsible for creating an instance of SharedUserScriptMaster. Globally accessible using extensions::ExtensionSystem::Get(browser_context). Will likely be used by Electron JavaScript bindings for managing extensions.
AtomExtensionLoader Provides functionality for loading and reloading extensions.
AtomExtensionsBrowserClient Interface to allow the extensions module to make browser-process-specific queries of Electron. This is where we can ignore specific features of the extensions module such as platform apps, profiles, and background updates.
AtomBrowserContext Now responsible for managing extension preferences. This required switching from using PrefRegistrySimple and PrefRegistrySyncable (which inherits PrefRegistrySimple).

Common

Class Description
AtomExtensionsClient Electron-variant of ExtensionsClient singleton for providing common global APIs between the browser and renderer process. This is where we can add what chrome.* API features we choose to support for Electron.

There's a bit of code generation that happens for implementing common extension features. This is done by atom/common/extensions/api/BUILD.gn which sources _manifest_features.json. For now, this only generates a manifest feature for content scripts. The config came from Chrome's _manifest_features.json.

Renderer

The entry point of changes for the renderer process can be found in renderer_client_base.cc.

Class Description
AtomExtensionsRendererClient Provides Electron-specific hooks into the extensions system for the renderer. Communicating to its extensions::Dispatcher reference sends events to the rest of the extensions system.

Bindings API design (WIP)

As extensions seem to be tied closely to a BrowserContext, it might be a good idea to provide methods on the Session API. This is the approach that the Muon fork took.

interface Extension {
  id: string,
  path: string,
  manifest: chrome.Manifest
}

class Session {
  /** Emitted when extension is installed and ready. */
  on(event: 'extension-ready', listener: (event: Event, extension: Extension) => void): this;

  loadExtension(path: string): Extension;
  removeExtension(extensionId: string): void;
  enableExtension(extensionId: string): void;
  disableExtension(extensionId: string): void;
}

Roadmap to merging

  • Get initial feedback from maintainers
  • Add #if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS) guards
  • Fix tests
  • Rename Shell* classes to Atom*

Followup roadmap

  • Add support for extensions in multiple BrowserContexts
  • Electron bindings for extension management (see Bindings API design)
  • Feature parity with existing JS extension implementation
    • Background scripts
    • DevTool extensions
  • Deprecate JS extensions implementation
  • Remove JS extensions implementation
  • Documentation for supported chrome.* APIs
  • Allow runtime toggling of extensions support?

Checklist

Release Notes

Notes: no-notes

Initial merge won't allow developers to use these features just yet.

@electron-cation electron-cation bot added new-pr 🌱 and removed new-pr 🌱 labels Mar 18, 2019

@samuelmaddock samuelmaddock changed the title WIP: Add extensions dependency WIP: feat: Add chromium extensions dependency bindings Mar 26, 2019

@samuelmaddock samuelmaddock marked this pull request as ready for review Mar 26, 2019

@ckerr ckerr requested a review from electron/wg-upgrades Mar 27, 2019

@ckerr

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

I know this is still a WIP but @'ing wg-upgrades anyway to get feedback from that team anyway for feedback on the idea of this feature in electron/electron

@samuelmaddock samuelmaddock force-pushed the samuelmaddock:extensions-dep branch 3 times, most recently from c1d32ae to c46b7e0 Mar 29, 2019

@samuelmaddock samuelmaddock changed the title WIP: feat: Add chromium extensions dependency bindings feat: Add chromium extensions dependency bindings [WIP] Mar 30, 2019

@codebytere codebytere changed the title feat: Add chromium extensions dependency bindings [WIP] [wip] feat: Add chromium extensions dependency bindings Apr 1, 2019

@codebytere codebytere added the wip label Apr 1, 2019

@ckerr
Copy link
Member

left a comment

This review was done as a group at the hackweek.

atom/browser/atom_browser_context.cc Outdated Show resolved Hide resolved
base::OnceClosure closure) {
// This method is called for Extension supports, but tests do not need to
// support exceptional CORS handling.
base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, std::move(closure));

This comment has been minimized.

Copy link
@ckerr

ckerr Apr 24, 2019

Member

Could a // TODO be added here -- we probably do need to support these allow/block patterns

atom/browser/atom_browser_main_parts.cc Outdated Show resolved Hide resolved
atom/browser/atom_browser_main_parts.cc Outdated Show resolved Hide resolved
atom/browser/atom_browser_main_parts.cc Outdated Show resolved Hide resolved
atom/common/extensions/api/_manifest_features.json Outdated Show resolved Hide resolved
AtomExtensionsAPIProvider::AtomExtensionsAPIProvider() = default;
AtomExtensionsAPIProvider::~AtomExtensionsAPIProvider() = default;

// TODO(samuelmaddock): generate API features?

This comment has been minimized.

Copy link
@ckerr

ckerr Apr 24, 2019

Member

We don't understand this comment. What needs to be done here?

This comment has been minimized.

Copy link
@samuelmaddock

samuelmaddock Apr 24, 2019

Author Member

AddShellAPIFeatures is a code generated function sourced from _api_features.json. The extensions subsystem and Chrome have their own features. We might need to do the same here.

AddAtomManifestFeatures, on line 32, is generated by the BUILD.gn file in this directory.


const std::string AtomExtensionsClient::GetProductName() {
// TODO(samuelmaddock):
return "app_shell";

This comment has been minimized.

Copy link
@ckerr

ckerr Apr 24, 2019

Member

this should use atom again

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Apr 28, 2019

Member

The issue here is that this in in common and therefore can run in either process. We can't guarantee to have access to atom::Browser::Get()->GetName()

cc @deepak1556 @nornagon suggestions?

This comment has been minimized.

Copy link
@nornagon

nornagon Jul 15, 2019

Contributor

where does this string end up? maybe just Electron is sufficient?

// extensions, so we always return 1. Note that 0 is reserved for the global
// world.
// TODO(samuelmaddock): skip electron worlds
return 10;

This comment has been minimized.

Copy link
@ckerr

ckerr Apr 24, 2019

Member

🤔

This should be calculated to skip the isolated world rather than a hardcoded magic number

This comment has been minimized.

Copy link
@samuelmaddock
atom/renderer/renderer_client_base.cc Outdated Show resolved Hide resolved

@MarshallOfSound MarshallOfSound force-pushed the samuelmaddock:extensions-dep branch 2 times, most recently from 030a062 to 8176fbd Apr 28, 2019

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

To avoid editing the original PR body I'd like to suggest slightly modified user facing API.

interface Extension {
  id: string,
  path: string,
  manifest: chrome.Manifest
}

class Session {
  /** Emitted when extension is installed and ready. */
  on(event: 'chrome-extension-will-activate', listener: (event: Event, extension: Extension) => void): this;
  on(event: 'chrome-extension-activated', listener: (event: Event, extension: Extension) => void): this;
  on(event: 'chrome-extension-deactivated', listener: (event: Event, extension: Extension) => void): this;

  /** This should reject non-absolute paths **/
  loadChromeExtension(path: string): Extension;
  removeChromeExtension(extensionId: string): void;
  enableChromeExtension(extensionId: string): void;
  disableChromeExtension(extensionId: string): void;
}

Summary:

  • All APIs are now chrome scoped to make it clear these are chrome extensions
  • Added two new events (activated and deactivated) to help with and UI around enable and disable
    • Although the internal hooks are activated and deactivated maybe we should expose events called enabled and disabled to make the API nicer 🤔
  • Added a will-activate event that is preventable (calling event.preventDefault() will prevent the installation). Can be powered by AtomExtensionLoader::PreAddExtension

#if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS)
// TODO: Why?
extension_system_ = nullptr;

This comment has been minimized.

Copy link
@samuelmaddock

samuelmaddock Apr 28, 2019

Author Member

This originates from the extension shell code. It doesn't seem useful from a functional benefit, but maybe rather signifies to a reader that it should no longer be used within that context.


extension_system_ = static_cast<extensions::AtomExtensionSystem*>(
extensions::ExtensionSystem::Get(this));
extension_system_->InitForRegularProfile(true /* extensions_enabled */);

This comment has been minimized.

Copy link
@samuelmaddock

samuelmaddock Apr 28, 2019

Author Member

I think we might need to implement AtomExtensionSystem::InitForIncognitoProfile to fix tests failing at

[958:0428/030848.424091:FATAL:storage_frontend.cc(140)] Check failed: !browser_context_->IsOffTheRecord().
@sentialx

This comment has been minimized.

Copy link

commented May 12, 2019

@samuelmaddock Will we be able to handle UI related things like browserAction.onClicked or chrome.tabs.query?

@samuelmaddock

This comment has been minimized.

Copy link
Member Author

commented May 12, 2019

@samuelmaddock Will we be able to handle UI related things like browserAction.onClicked or chrome.tabs.query?

Not with the currently introduced changes. Some chrome.* APIs will be missing until they're implemented specifically for Electron.

I've thought through how chrome.browserAction could work. Providing a custom button element, similar to <webview>, implemented as <browser-action extensionid="name" tabid="1"> would provide a simple API for Electron developers. Bindings similar to Muon's implementation would forward IPC messages to the renderer for connecting the custom element.

I should mention that I've since stopped actively working on this PR as the project I needed it for is no longer using Electron. Without some kind of sponsorship, I don't see this changing. Electron maintainers have shown interest in merging the changes though.

@sentialx

This comment has been minimized.

Copy link

commented May 19, 2019

I've created a library for implementing some of Chrome APIs, maybe this could help: https://github.com/wexond/electron-extensions

@jkleinsc jkleinsc referenced this pull request May 23, 2019
@nornagon nornagon referenced this pull request May 30, 2019
4 of 6 tasks complete

@samuelmaddock samuelmaddock requested a review from electron/wg-releases as a code owner Jun 3, 2019

@nornagon

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

Okay, this is getting close to ready to land I think. There's still plenty of work left to do but we're approaching a good "step 1".

TODO before landing:

  1. Currently the tests are crashing when in-memory sessions are used (e.g. in the netLog tests, which create an in-memory session). We should fix this so that all the tests pass when the enable_electron_extensions build flag is enabled.
  2. We should have at least a couple more tests. Currently we just test that a content script gets injected. We should also test a few chrome.* APIs that we expect to work.
  3. We're not ready for prime-time yet so we should disable the enable_electron_extensions flag before landing.

(3) will mean that it's going to be easy for the extensions code to break unless we have a CI environment building with extensions enabled, so we'll have to stay on top of keeping it working by running tests on our local machines until we can enable the flag by default (and eventually remove the flag altogether).

@nornagon nornagon changed the title [wip] feat: Add chromium extensions dependency bindings feat: Add chromium extensions dependency bindings Jul 18, 2019

@nornagon nornagon removed the wip label Jul 18, 2019

@nornagon nornagon changed the title feat: Add chromium extensions dependency bindings feat: preliminary support for //extensions Jul 18, 2019

@nornagon

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

(1) above is fixed, and there are now a few simple tests. The mac build failure is the autoUpdater spec which is broken on PRs from forks.

BUILD.gn Outdated Show resolved Hide resolved

# Enable Chrome extensions support
# TODO: Disable before initial merge
enable_electron_extensions = true

This comment has been minimized.

Copy link
@nornagon

nornagon Jul 23, 2019

Contributor
Suggested change
enable_electron_extensions = true
enable_electron_extensions = false
shell/browser/atom_browser_main_parts.cc Outdated Show resolved Hide resolved
shell/browser/atom_browser_main_parts.cc Outdated Show resolved Hide resolved

nornagon added some commits Jul 23, 2019

@samuelmaddock
Copy link
Member Author

left a comment

Thanks @nornagon for fixing this up. 🎉

(session.defaultSession as any).loadChromeExtension(path.join(fixtures, 'extensions', 'simple'))
const w = new BrowserWindow({show: false})
const customSession = session.fromPartition(`persist:${require('uuid').v4()}`);
(customSession as any).loadChromeExtension(path.join(fixtures, 'extensions', 'red-bg'))

This comment has been minimized.

Copy link
@samuelmaddock

samuelmaddock Jul 23, 2019

Author Member

Should there be a TODO dropped in here to add type definitions for loadChromeExtension when the extensions build flag is toggled on?

This comment has been minimized.

Copy link
@nornagon

nornagon Jul 23, 2019

Contributor

I expect the API surface will change substantially between now and then so I'm inclined to leave it for now.

@@ -0,0 +1,55 @@
// Copyright 2014 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

This comment has been minimized.

Copy link
@samuelmaddock

samuelmaddock Jul 23, 2019

Author Member

Is it okay to leave these copyright headers introduced from the extensions shell code? Or should they be replaced with the GitHub copyright?

This comment has been minimized.

Copy link
@nornagon

nornagon Jul 23, 2019

Contributor

🤷‍♂

This comment has been minimized.

Copy link
@jkleinsc

jkleinsc Jul 24, 2019

Contributor

We have many examples in our code where we have left in the Chromium copyright and/or added additional copyright depending on the person who created the code (eg there is a Slack copyright in some files). All that to say, its fine to leave as is unless an author wants to add their copyright.

spec-main/fixtures/extensions/chrome-storage/main.js Outdated Show resolved Hide resolved
@jkleinsc
Copy link
Contributor

left a comment

Overall LGTM. The only nit I would have is whether or not we should continue using the "Atom" prefix for new classes (vs "Electron" prefix).

@@ -0,0 +1,55 @@
// Copyright 2014 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

This comment has been minimized.

Copy link
@jkleinsc

jkleinsc Jul 24, 2019

Contributor

We have many examples in our code where we have left in the Chromium copyright and/or added additional copyright depending on the person who created the code (eg there is a Slack copyright in some files). All that to say, its fine to leave as is unless an author wants to add their copyright.


namespace extensions {

AtomRuntimeAPIDelegate::AtomRuntimeAPIDelegate(

This comment has been minimized.

Copy link
@jkleinsc

jkleinsc Jul 24, 2019

Contributor

If we are adding new classes, should we still use the "Atom" prefix or move over to "Electron" prefix?

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Jul 24, 2019

Member

I thought we were going to stop prefixing, that's what namespacing is for 😆

cc @deepak1556 I think we talked about this

This comment has been minimized.

Copy link
@nornagon

nornagon Jul 24, 2019

Contributor

gonna leave this for now and it can get caught when we do the first sweep.

spec-main/api-net-log-spec.js Show resolved Hide resolved

@nornagon nornagon merged commit 9597729 into electron:master Jul 24, 2019

8 of 9 checks passed

build-mac Workflow: build-mac
Details
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Jul 24, 2019

No Release Notes

@nornagon nornagon referenced this pull request Jul 25, 2019
1 of 11 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.