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

Clarify `script.addPreloadScript` with duplicate `context` #620

Merged

Lightning00Blade merged 5 commits into main from enforce-set-in-add-preload-script

Dec 19, 2023

Contributor

Lightning00Blade commented Dec 14, 2023 •

edited by pr-preview bot

Loading

Closes #617.

Lightning00Blade requested a review from sadym-chromium

December 14, 2023 11:54


          Clarify script.addPreloadScript with duplicate context

baf1d88

Lightning00Blade force-pushed the enforce-set-in-add-preload-script branch from 9f2bfc5 to baf1d88 Compare

December 14, 2023 12:05

Contributor

whimboo commented Dec 14, 2023

As I've mentioned on the issue I wonder if we should do the same for session.subscribe and session.unsubscribe as well given that those two methods suffer from the same issue.

sadym-chromium reviewed

View reviewed changes

index.bs Outdated Show resolved Hide resolved

Contributor

sadym-chromium commented Dec 14, 2023

As I've mentioned on the issue I wonder if we should do the same for session.subscribe and session.unsubscribe as well given that those two methods suffer from the same issue.

In I'm general agree with your proposal to fix the subscribe / unsubscribe as well, but let's split the changes.

Contributor Author

Lightning00Blade commented Dec 14, 2023

Agree that we should change it there as well so I would keep the issue open till then, but let's do it in separate PRs.

sadym-chromium reviewed

View reviewed changes

index.bs Outdated Show resolved Hide resolved

sadym-chromium reviewed

View reviewed changes

index.bs Outdated Show resolved Hide resolved

sadym-chromium approved these changes

View reviewed changes

Lightning00Blade requested a review from whimboo

December 14, 2023 13:21

jgraham approved these changes

View reviewed changes

index.bs Outdated Show resolved Hide resolved

Lightning00Blade added 2 commits

December 14, 2023 15:53


          address comments

7f5a54b


          update

2a46e7f

Lightning00Blade force-pushed the enforce-set-in-add-preload-script branch from 56123b4 to 2a46e7f Compare

December 14, 2023 14:55

Contributor

whimboo commented Dec 14, 2023

@Lightning00Blade would you be so kind and add a wpt test for the multiple usage of context?

whimboo approved these changes

View reviewed changes

index.bs Outdated Show resolved Hide resolved

Contributor

whimboo commented Dec 14, 2023

@Lightning00Blade would you be so kind and add a wpt test for the multiple usage of context?

Err, this will actually be part of web-platform-tests/wpt#41932.

Lightning00Blade added 2 commits

December 18, 2023 16:50


          Change order of commands

0f08aef


          fix typo

cfbea2d

Lightning00Blade requested a review from whimboo

December 18, 2023 17:06

jrandolf self-requested a review

December 18, 2023 17:08

jrandolf approved these changes

View reviewed changes

whimboo approved these changes

View reviewed changes

Contributor

whimboo left a comment

Looks good. Thanks!

Lightning00Blade merged commit b6adb45 into main

5 checks passed

Lightning00Blade deleted the enforce-set-in-add-preload-script branch

December 19, 2023 10:46

github-actions bot added a commit that referenced this pull request


          Clarify script.addPreloadScript with duplicate context (#620)

e6a6dd1

SHA: b6adb45
Reason: push, by Lightning00Blade

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