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.

Sign up for GitHub

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

Jump to bottom

feat: add API for receiving logs from service workers #20624

Merged
merged 10 commits into from
Feb 20, 2020
Merged

feat: add API for receiving logs from service workers #20624

merged 10 commits into from
Feb 20, 2020

Conversation

MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented Oct 17, 2019 •
edited
Loading

Pretty simple API here, plans are for the following

  • console-message event. Vaguely equivalent to the console-message event on the webContents API so that apps can capture logs from their service workers.
  • getWorkerInfoFromID() Get service worker scope and script url for a given ID
  • getRunningWorkerInfos() Get service worker scope, script url and id for all active workers

The second two APIs are blocked on a Chromium roll hence WIP, the event is already implemented.

TODO:

  • console-message
  • getAll()
  • getFromVersionId()
  • Docs
  • Tests

Notes: Added session.serviceWorkerContext API to access basic service worker info and receive console logs from service workers.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 17, 2019
@miniak
Copy link
Contributor

miniak commented Oct 18, 2019 •
edited
Loading

@MarshallOfSound Is session.getRunningWorkerInfos() a replacement for webContents.getAllSharedWorkers() added in #20389?

@miniak
Copy link
Contributor

miniak commented Oct 18, 2019

@MarshallOfSound should we also deprecate webContents.inspectServiceWorker() and webContents.inspectSharedWorker() and replace it with a new API that supports any type of worker?

@MarshallOfSound
Copy link
Member Author

@miniak SharedWorker !== ServiceWorker afaik. This APIs primary purpose is getting logs from service workers. If we feel it's appropriate for this to work for all workers that is substantially more complicated as service workers, shared workers and web workers are each very different things in Chrome

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 18, 2019
@MarshallOfSound MarshallOfSound force-pushed the service-worker-logging-api branch from e5d35cc to 46bc01f Compare November 19, 2019 23:56
@MarshallOfSound MarshallOfSound changed the title [WIP] feat: add API for receiving logs from service workers feat: add API for receiving logs from service workers Nov 20, 2019
@MarshallOfSound
Copy link
Member Author

Removing WIP as this is ready for code review. I'll work on tests shortly 👍

@MarshallOfSound MarshallOfSound force-pushed the service-worker-logging-api branch from 46bc01f to ebd981c Compare November 26, 2019 21:28
nornagon
nornagon suggested changes Nov 26, 2019
View reviewed changes
shell/browser/api/atom_api_service_worker_context.cc Outdated

namespace {

std::string MessageSourceToString(
Copy link
Member

Choose a reason for hiding this comment

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

gin converter?

shell/browser/api/atom_api_service_worker_context.cc Outdated Show resolved Hide resolved
shell/browser/api/atom_api_service_worker_context.cc Outdated Show resolved Hide resolved
shell/browser/api/atom_api_service_worker_context.cc Outdated Show resolved Hide resolved
shell/browser/api/atom_api_service_worker_context.cc Outdated Show resolved Hide resolved
shell/browser/api/atom_api_service_worker_context.cc Outdated Show resolved Hide resolved
nornagon
nornagon reviewed Nov 26, 2019
View reviewed changes
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

also, please add tests.

@MarshallOfSound MarshallOfSound force-pushed the service-worker-logging-api branch 2 times, most recently from fc0a70d to 4a0ec9c Compare January 9, 2020 23:07
nornagon
nornagon approved these changes Jan 13, 2020
View reviewed changes
docs/api/service-workers.md Show resolved Hide resolved
docs/api/service-workers.md Show resolved Hide resolved
docs/api/structures/service-worker-info.md

* `scriptUrl` String - The full URL to the script that this service worker runs
* `scope` String - The base URL that this service worker is active for.
* `renderProcessId` Number - The virtual ID of the process that this service worker is running in. This is not an OS level PID. This aligns with the ID set used for `webContents.getProcessId()`.
Copy link
Member

Choose a reason for hiding this comment

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

If we wanted to add e.g. methods for killing / restarting service workers, would they go on this object? e.g.

session.defaultSession.serviceWorkers.getAllRunning().forEach(sw => sw.shutdown())

or something else?

Copy link
Member Author

Choose a reason for hiding this comment

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

Either that, or have serviceWorkers.stopByVersionId(versionid)

spec-main/api-service-workers-spec.ts Show resolved Hide resolved
spec-main/api-service-workers-spec.ts Outdated Show resolved Hide resolved
@MarshallOfSound MarshallOfSound force-pushed the service-worker-logging-api branch from 9599b43 to 770c164 Compare February 20, 2020 22:17
@MarshallOfSound MarshallOfSound merged commit e7b0a9c into master Feb 20, 2020
@release-clerk
Copy link

release-clerk bot commented Feb 20, 2020

Release Notes Persisted

Added session.serviceWorkerContext API to access basic service worker info and receive console logs from service workers.

@MarshallOfSound MarshallOfSound deleted the service-worker-logging-api branch February 20, 2020 23:19
@trop trop bot mentioned this pull request Feb 20, 2020
Merged
@trop
Copy link
Contributor

trop bot commented Feb 20, 2020

I have automatically backported this PR to "9-x-y", please check out #22313

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

@nornagon nornagon nornagon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MarshallOfSound @miniak @nornagon