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

create specification for self.browser #508

Merged
merged 14 commits into from
Jan 29, 2024

Conversation

patrickkettner
Copy link
Contributor

@patrickkettner patrickkettner commented Dec 14, 2023

Create specification for window.browser/self.browser.

preview available here

@patrickkettner
Copy link
Contributor Author

@Rob--W @xeenon would yall be able to review when time permits?

Copy link
Collaborator

@xeenon xeenon left a comment

Choose a reason for hiding this comment

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

Looks good! Some references need corrected to point in the right places.

Also, do you have the original bike shed file? We have Bikeshed transformation setup in our repo, so you should be able to just post that file, like index.bs which is more human friendly. That gets published here.

specification/window.browser.html Outdated Show resolved Hide resolved
specification/window.browser.html Outdated Show resolved Hide resolved
specification/window.browser.html Outdated Show resolved Hide resolved
@patrickkettner
Copy link
Contributor Author

thanks @xeenon!
I added the bikeshed file. The current build system is using pr-preview, and configured to build the existing spec. pr-preview has a long running request to support multiple files, but the existing PR has been untouched for years. @dotproto and I discussed replacing the build system with something that supports multiple files built directly on bikeshed, rather than using pr-preview. I plan on digging into it soon. In the meantime, given devlin's desire to give a few months notice to developer before any change landing, I didn't want to block on that.

specification/window.browser.bs Outdated Show resolved Hide resolved
specification/window.browser.bs Outdated Show resolved Hide resolved
specification/window.browser.bs Outdated Show resolved Hide resolved
specification/window.browser.bs Show resolved Hide resolved
Copy link
Member

@oliverdunk oliverdunk left a comment

Choose a reason for hiding this comment

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

Thanks so much for putting this together, I'm excited! Left a couple of thoughts but this generally looks good.

specification/window.browser.bs Outdated Show resolved Hide resolved
specification/window.browser.bs Outdated Show resolved Hide resolved
specification/window.browser.bs Show resolved Hide resolved
specification/window.browser.bs Outdated Show resolved Hide resolved
@Jack-Works
Copy link

are you considering renaming this PR (or part of the non-normative sections) to self.browser? that covers service workers.

Copy link
Member

@dotproto dotproto left a comment

Choose a reason for hiding this comment

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

LGTM

@patrickkettner patrickkettner changed the title create specification for window.browser create specification for self.browser Jan 18, 2024
Copy link
Member

@oliverdunk oliverdunk left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@patrickkettner
Copy link
Contributor Author

@Rob--W any thoughts?

specification/window.browser.bs Outdated Show resolved Hide resolved
specification/window.browser.html Outdated Show resolved Hide resolved
specification/window.browser.bs Outdated Show resolved Hide resolved
specification/window.browser.bs Outdated Show resolved Hide resolved
Comment on lines 68 to 69
When {{browser}} is defined on {{window}}, it SHOULD also be implemented on {{ServiceWorkerGlobalScope}} and {{DedicatedWorkerGlobalScope}}.
It MUST be used exclusively for WebExtension purposes, however the contents of each instance of {{browser}} is UA defined.
Copy link
Member

Choose a reason for hiding this comment

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

Question: The mention of Service Workers here may raise questions, because Service workers are defined for http(s) URLs only: https://w3c.github.io/ServiceWorker/#start-register-algorithm
(in practice mainly https, http is only supported for edge cases such as localhost).

Chrome (and Firefox's experimental implementation) does not permit extension code to register arbitrary service workers. Instead, the extension framework takes care of registering workers despite the http(s) restrictions.

Should we clarify in a note that extensions cannot register Service workers as usual (i.e. the https restriction), but that browsers may spawn a Service worker associated with a WebExtension origin?

Should we drop the mention of workers entirely from this "window.browser" document, and define the worker behavior in an entirely separate spec/PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback @Rob--W!

because Service workers are defined for http(s) URLs only: https://w3c.github.io/ServiceWorker/#start-register-algorithm (in practice mainly https, http is only supported for edge cases such as localhost).

I am not sure I follow. The spec in this PR says that if window.browser is defined, then self.browser should be defined on any ServiceWorkerGlobalScope whose origin is a webextension. While the service worker spec is only http(s), the thing that is registered as a background script in Chrome for manifest v3 is
ServiceWorkerGlobalScope.

Chrome (and Firefox's experimental implementation) does not permit extension code to register arbitrary service workers. Instead, the extension framework takes care of registering workers despite the http(s) restrictions.

If they are not permitted, then I don't see the problem? ServiceWorkerGlobalScope is what is created when you have an extension in Chrome/Firefox's-experimental-implementation. Can you explain what you are wanting changed?

Should we clarify in a note that extensions cannot register Service workers as usual (i.e. the https restriction)

While I see the use for that in a broader web extensions specification, as there is no other portion of this document is about registering a service worker, or when it should be registered on window. That seems outside of this document's scope.

Should we drop the mention of workers entirely from this "window.browser" document, and define the worker behavior in an entirely separate spec/PR?

Given the current level of web extension specification, having two essentially identical documents to accomplish the same thing ("globalThis.browser should only be used for web extensions") feels unnecessary.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the feedback @Rob--W!

because Service workers are defined for http(s) URLs only: https://w3c.github.io/ServiceWorker/#start-register-algorithm (in practice mainly https, http is only supported for edge cases such as localhost).

I am not sure I follow. The spec in this PR says that if window.browser is defined, then self.browser should be defined on any ServiceWorkerGlobalScope whose origin is a webextension. While the service worker spec is only http(s), the thing that is registered as a background script in Chrome for manifest v3 is
ServiceWorkerGlobalScope.

In the realm of specifications, Service Workers (and thus ServiceWorkerGlobalScope instances) only exist on https schemes (and localhost). Here we specify that ServiceWorkerGlobalScope should have a browser object, for WebExtension origins only. I initially found this inconsistent, because the SW spec doesn't leave room for SW on other schemes, while our spec here implies the existence of an extension origin.

I have now reconciled this inconsistency by observing that "extension origin" could theoretically also involve one of the accepted schemes (even though that doesn't happen in practice - as an example from a far past, Chrome had the concept of Hosted apps where regular websites received extra privileges if a crx file defining the "hosted app" was installed).
We (browser vendors) understand the intent and practical meaning here, and I am fine with not mentioning anything more about SW for now.

specification/window.browser.bs Outdated Show resolved Hide resolved
@Rob--W
Copy link
Member

Rob--W commented Jan 26, 2024

I know that we wanted us to spec the existence of browser because Chrome considers a spec as a prerequisite to exposing this. The specific behavior of "browser" is left up to the UA, so my question here is not a blocking part of the review. Still, I'm curious about the ideas behind it.

Chrome (and Safari) both have the concept of "isolated world" for content scripts. An isolated world shares the same view of the DOM (DOM instances from native code are shared), with unique JavaScript wrappers (isolated per world). If window.browser were to be defined in the idl as part of window, what does that say about the value read by the main world vs isolated world? Is it the same (native) object, but with different wrappers that somehow filters the available extension API methods? Or is the browser namespace a different object (and therefore implicitly also a separate wrapper if any)?

Here is some background on isolated world and a description of bindings: https://chromium.googlesource.com/chromium/src.git/+/123.0.6266.0/third_party/blink/renderer/bindings/core/v8/V8BindingDesign.md

In Firefox, externally_connectable has not been implemented yet (bug 1319168), which means that there is no browser namespace in the main world, only in that of content scripts. The current implementation of the sandbox in Firefox is that globalThis is a sandbox inheriting from window. globalThis.browser is the extension API namespace, window.browser is currently undefined. I imagine that if we were to implement window.browser, that the main world version would be generated through WebIDL, and that the content script continues to have its own implementation-defined extension API namespace separate from window.browser.

@xeenon
Copy link
Collaborator

xeenon commented Jan 26, 2024

The Safari and WebKit implementations of web extensions currently dynamically add the browser object as needed to the DOM global object, per world. So it is always a different wrapper and a different internal object.

It needed to be different since the things contained in the browser namespace for the main world in web pages is drastically reduced and different from the extension content script world or main world in extensions pages.

We will likely continue to dynamically add this property to the global scope, instead of adding it in our DOM window IDL.

I don't think that fundamentally blocks anything here. Browser implementations can expose the APIs however their implementation requires, as long as it matches how the IDL in the spec defines it.

Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

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

LGTM! I can't easily verify the preview, but the source looks good!

@oliverdunk oliverdunk merged commit 4c0ba61 into w3c:gh-pages Jan 29, 2024
1 check passed
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.

6 participants