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

Add runtime.getContexts() proposal #358

Merged
merged 6 commits into from
Mar 29, 2023

Conversation

rdcronin
Copy link
Contributor

@rdcronin rdcronin commented Mar 3, 2023

This adds a proposal for a new runtime.getContexts() API.

This work is based off the original pull request from justinlulejian here.

@dotproto
Copy link
Member

dotproto commented Mar 3, 2023

FYI for reviewers: you can view a rendered version of the proposal Markdown by navigating to the Files Changed tab, opening the options menu for the file, and selecting "View file".

Screen Shot 2023-03-03 at 11 31 00 AM

@dotproto
Copy link
Member

dotproto commented Mar 3, 2023

@rdcronin, the W3C's IPR (Intellectual Property Rights) bot was not able to verify your membership in the group. To fix this, you'll need to create a W3C account (if you don't already have one), make sure your GitHub account is linked to your W3C account, and join the WebExtensions Community Group.

@rdcronin
Copy link
Contributor Author

rdcronin commented Mar 3, 2023

Request to join sent. (I had linked Github + W3C, but hadn't yet joined)

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.

Overall this proposal looks good to me and I'm very supportive of the direction. I have a several questions about the current draft, but I don't think any of them are blocking for experimental implementation purposes.

proposals/runtime_get_contexts.md Outdated Show resolved Hide resolved
proposals/runtime_get_contexts.md Outdated Show resolved Hide resolved
proposals/runtime_get_contexts.md Outdated Show resolved Hide resolved
proposals/runtime_get_contexts.md Outdated Show resolved Hide resolved
proposals/runtime_get_contexts.md Outdated Show resolved Hide resolved
Like `documentId`s, the extension `contextId` will update on (non-same-page)
navigation.

#### Incognito mode
Copy link
Member

Choose a reason for hiding this comment

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

This section currently describes split mode but does not directly address spanning mode. It seems reasonable to assume that in spanning mode getContexts() will return both private and non-private contexts, but I'd prefer to be explicit about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

spanning mode is... weird.

Try embedding an iframe in an incognito window in a spanning mode extension: you get a pseudo-split-mode-but-not-really extension.

I didn't really want to spec out what happens here. I've taken a page from the fetch API and said "it's left as an exercise to the implementor."

proposals/runtime_get_contexts.md Outdated Show resolved Hide resolved
proposals/runtime_get_contexts.md Outdated Show resolved Hide resolved
proposals/runtime_get_contexts.md Show resolved Hide resolved
proposals/runtime_get_contexts.md Show resolved Hide resolved
Copy link
Contributor Author

@rdcronin rdcronin left a comment

Choose a reason for hiding this comment

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

Thanks, Simeon! Comments addressed.

proposals/runtime_get_contexts.md Outdated Show resolved Hide resolved
proposals/runtime_get_contexts.md Outdated Show resolved Hide resolved
proposals/runtime_get_contexts.md Outdated Show resolved Hide resolved
proposals/runtime_get_contexts.md Outdated Show resolved Hide resolved
frameId: int,
// The current URL of the document, or undefined if this context is not
// hosted in a document.
documentUrl?: string,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, no. I've thought about if we'd want to support a different filter object (so you could specify e.g. array of types, match patterns, etc), but I wasn't sure if it was worth it. It seems like you'd normally either a) be able to specify an explicit URL or b) identify the contexts in different ways (say, by context type).

If folks feel strongly that we should support a richer filter from the beginning, I'm amenable to introducing a new ContextFilter type. Otherwise, we could potentially do this in the future (by supporting choices for the appropriate fields). Lemme know what y'all think.

proposals/runtime_get_contexts.md Outdated Show resolved Hide resolved
Like `documentId`s, the extension `contextId` will update on (non-same-page)
navigation.

#### Incognito mode
Copy link
Contributor Author

Choose a reason for hiding this comment

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

spanning mode is... weird.

Try embedding an iframe in an incognito window in a spanning mode extension: you get a pseudo-split-mode-but-not-really extension.

I didn't really want to spec out what happens here. I've taken a page from the fetch API and said "it's left as an exercise to the implementor."

proposals/runtime_get_contexts.md Outdated Show resolved Hide resolved
proposals/runtime_get_contexts.md Show resolved Hide resolved
proposals/runtime_get_contexts.md Show resolved Hide resolved
defined as:

```js
extension.ContextType = {
Copy link
Member

Choose a reason for hiding this comment

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

Is this enum meant to be exhaustive? Can other browsers extend it? For example, a number of browsers support sidebars. Some extensions for Firefox and Vivaldi currently struggle with messaging with these contexts (since sidebars do not have a meaningful tabId and frameId for messaging. Developers can create persistent ports, but it is ugly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As with all APIs discussed in this community group, it's non-binding, and browser vendors can deviate when it makes sense. If a browser supports a different type of context, they can add it in here, and if they don't support a given context, they can remove the enum entry.

FWIW, Chrome is also going to support side panels, so we will add a SIDE_PANEL context type when that happens. (I was tempted to include it here, but didn't want to confuse the proposal by having unshipped concepts).

Copy link
Member

Choose a reason for hiding this comment

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

@rdcronin Could you list all types that you're thinking of for Chrome? If you'd like, prefaced by a comment // (Chrome-specific), so that we can see where we could converge towards a common type if desired.

E.g. I would expect at least devtools and devtools panel to have their distinct type here (as these do not fit in any of the other existing categories).

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 call-out! Devtools are interesting -- in Chromium, they commit to a different origin (even though from the extension's POV, they are just the extension page). They are also not currently included at all in extension.getViews(). I'd like to tackle these separately, but I've added an explicit section for them in future work.

I also added in a type for SIDE_PANEL (though it's still under development in Chrome)

proposals/runtime_get_contexts.md Outdated Show resolved Hide resolved
* Change links to avoid http://go/.
* Add more details on the `ContextFilter` behavior.
@rdcronin
Copy link
Contributor Author

Thanks for the feedback, all! I think all comments have been addressed. @Rob--W , if this looks good, please feel free to merge it in.

@Rob--W Rob--W merged commit 6183ce7 into w3c:main Mar 29, 2023
github-actions bot added a commit that referenced this pull request Mar 29, 2023
SHA: 6183ce7
Reason: push, by Rob--W

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

4 participants