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 proposal for runtime.onEnabled and runtime.onExtensionLoaded events #729

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

sohailrajdev97
Copy link

@hanguokai
Copy link
Member

Thanks for writing the formal proposal and sponsoring the implementation.

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 picking this up! I left one comment, but otherwise LGTM.

proposals/runtime_on_load_on_enabled_events.md Outdated Show resolved Hide resolved
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.

See above comments.

@sohailrajdev97
Copy link
Author

@Rob--W - Can you also take a look at this one?

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 - thanks! Seems like the discussion on enum values is resolved.

@Rob--W
Copy link
Member

Rob--W commented Jan 16, 2025

I reviewed the proposal, and it generally looks good. There is one unspecified aspect... What are the expected semantics when incognito: "split" is used? This is currently only implemented by Chromium browsers (e.g. Firefox does not support it).

When "incognito": "split" is specified in manifest.json, a new extension instance (background script) starts when the first Incognito window opens. When the last incognito window closes, the incognito session including extensions are torn down. In this model, my expectation is that Chrome may consider firing onExtensionLoaded whenever a new incognito window is opened, even if a non-incognito window has already been initialized.

The proposal currently suggests to use the onExtensionLoaded event to decide on whether to populate storage.session. That use case does not require a new event: an extension could unconditionally call storage.session.get at startup and check whether the value was already populated, and if not, start the initialization. Note that in Chrome, storage.session in incognito and non-incognito modes are separate.

@oliverdunk
Copy link
Member

I reviewed the proposal, and it generally looks good. There is one unspecified aspect... What are the expected semantics when incognito: "split" is used?

This is a great callout. I think https://issues.chromium.org/41029550 which you opened is good context as well. This is an existing problem with the onInstalled event and it would be nice to agree on something as part of this work. My first thought is that firing the event probably makes sense but I'd be interested to hear any other arguments.

The proposal currently suggests to use the onExtensionLoaded event to decide on whether to populate storage.session. That use case does not require a new event: an extension could unconditionally call storage.session.get at startup and check whether the value was already populated

I think we can probably just remove that example from the proposal. The other use cases are compelling and harder to do with existing workarounds.

@sohailrajdev97
Copy link
Author

This is an existing problem with the onInstalled event and it would be nice to agree on something as part of this work. My first thought is that firing the event probably makes sense but I'd be interested to hear any other arguments.

I too agree that firing the event for split mode incognito workers makes sense. I have updated the proposal accordingly. Please take a look 😄.

FYI @Rob--W, @oliverdunk, @rdcronin, @xeenon

@oliverdunk
Copy link
Member

Looks good to me. I confirmed that Devlin was happy with the split mode behavior from a Chrome perspective.

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