Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 #358Add
runtime.getContexts()
proposal #358Changes from 3 commits
d2110d2
d0043ef
d7a6655
a1b960b
7de6f80
402a463
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When used as a filter parameter, is it possible to use a match pattern for the value of
ContextFilter.documentUrl
?There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do
documentUrl
anddocumentOrigin
refer to the document that the extension context is inside (e.g. top level main frame of a tab), the extension context itself, or something else? Depending on the answer, I may have other questions.The "Sandboxed Pages" section notes that this method will not return sandboxed pages as they have "a separate origin (
"null"
) and do not have access to extension APIs." I find this somewhat surprising as I had assumed that thedocumentOrigin
property to the origin of the extension context and therefore this property would either match the extension's origin (i.e. the value returned by evaluatingnew URL(browser.runtime.getURL("")).origin
) for normal extension pages ornull
for sandboxed pages.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They refer to the document the extension context is executing in, so it depends on the context:
I've added a new section on this in "Additional Considerations"
This was inline with Rob's suggestion on the old PR to not include sandboxed pages as contexts here, which I agree with (they are reasonably separate from the rest of the extension). We could always change that in the future by adding a new context type.
There was a problem hiding this comment.
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
andframeId
for messaging. Developers can create persistent ports, but it is ugly.There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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."