Skip to content

Simplify Navigation::scriptExecutionContext()#26054

Merged
webkit-commit-queue merged 1 commit into
WebKit:mainfrom
TingPing:pgriffis/navigation/navigation-refactor
Mar 21, 2024
Merged

Simplify Navigation::scriptExecutionContext()#26054
webkit-commit-queue merged 1 commit into
WebKit:mainfrom
TingPing:pgriffis/navigation/navigation-refactor

Conversation

@TingPing
Copy link
Copy Markdown
Contributor

@TingPing TingPing commented Mar 18, 2024

30c07bf

Simplify Navigation::scriptExecutionContext()
https://bugs.webkit.org/show_bug.cgi?id=271161

Reviewed by Chris Dumez.

As Navigation is a LocalDOMWindowProperty it always has
access to its own ScriptExecutionContext.

* Source/WebCore/page/LocalDOMWindow.cpp:
(WebCore::LocalDOMWindow::navigation):
* Source/WebCore/page/Navigation.cpp:
(WebCore::Navigation::Navigation):
(WebCore::Navigation::initializeEntries):
(WebCore::Navigation::scriptExecutionContext const):
(WebCore::Navigation::serializeState):
(WebCore::Navigation::navigate):
(WebCore::Navigation::updateCurrentEntry):
(WebCore::Navigation::updateForNavigation):
* Source/WebCore/page/Navigation.h:
* Source/WebCore/page/Navigation.idl:

Canonical link: https://commits.webkit.org/276478@main

6b35672

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug 🧪 wpe-wk2
✅ 🧪 webkitperl 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-skia
🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv-sim ✅ 🧪 api-gtk
✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

@TingPing TingPing requested a review from cdumez as a code owner March 18, 2024 17:15
@TingPing TingPing self-assigned this Mar 18, 2024
@TingPing TingPing added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label Mar 18, 2024
@TingPing TingPing requested review from darinadler and rwlbuis March 18, 2024 17:15
Copy link
Copy Markdown
Contributor

@rwlbuis rwlbuis left a comment

Choose a reason for hiding this comment

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

LGTM.

@darinadler darinadler requested review from cdumez and removed request for cdumez March 19, 2024 21:51
@darinadler
Copy link
Copy Markdown
Member

@cdumez, I would like to understand if the script execution context can ever be a different document from the same website.

@cdumez
Copy link
Copy Markdown
Contributor

cdumez commented Mar 19, 2024

@cdumez, I would like to understand if the script execution context can ever be a different document from the same website.

When dealing with a Page (not workers), a ScriptExecutionContext is a Document. There is one per frame on the page. ScriptExecutionContexts are generally not aware of each other unless they have the same security origin (otherwise the frames wouldn't be able to script each other).

The patch here does have some behavior change I believe because of the changes to the IDL. The IDL used to do CallWith=CurrentScriptExecutionContext for navigate(). IIRC, this means we were passing the caller's scriptExecutionContext to navigate(), which may be distinct from the Navigation object's ScriptExecutionContext.

e.g. otherWindow.navigate() where Frame A is calling navigate on another same origin iframe's Window. I didn't look into details to see which ScriptExecutionContext navigate() is supposed to use per the specification. However, we need to be mindful that there is a behavior change here and that it may or may not be in the right direction.

@TingPing
Copy link
Copy Markdown
Contributor Author

TingPing commented Mar 20, 2024

The patch here does have some behavior change I believe because of the changes to the IDL.

FWIW the IDL attribute there before was just a webkit change.

There are some cross-document parts of the spec but I’m not sure we need their context. It’s something I’m aware of going forward as I understand it more now.

Comment thread Source/WebCore/page/Navigation.cpp Outdated
Comment thread Source/WebCore/page/Navigation.cpp Outdated
Comment thread Source/WebCore/page/Navigation.cpp Outdated
Comment thread Source/WebCore/page/Navigation.cpp Outdated
Comment thread Source/WebCore/page/Navigation.h Outdated
@TingPing TingPing force-pushed the pgriffis/navigation/navigation-refactor branch from e5b88b3 to 1f39161 Compare March 20, 2024 15:15
@TingPing TingPing requested a review from cdumez March 20, 2024 15:16
@TingPing TingPing added the safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks label Mar 20, 2024
@TingPing TingPing force-pushed the pgriffis/navigation/navigation-refactor branch from 1f39161 to 6b35672 Compare March 21, 2024 15:21
@TingPing TingPing added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Mar 21, 2024
https://bugs.webkit.org/show_bug.cgi?id=271161

Reviewed by Chris Dumez.

As Navigation is a LocalDOMWindowProperty it always has
access to its own ScriptExecutionContext.

* Source/WebCore/page/LocalDOMWindow.cpp:
(WebCore::LocalDOMWindow::navigation):
* Source/WebCore/page/Navigation.cpp:
(WebCore::Navigation::Navigation):
(WebCore::Navigation::initializeEntries):
(WebCore::Navigation::scriptExecutionContext const):
(WebCore::Navigation::serializeState):
(WebCore::Navigation::navigate):
(WebCore::Navigation::updateCurrentEntry):
(WebCore::Navigation::updateForNavigation):
* Source/WebCore/page/Navigation.h:
* Source/WebCore/page/Navigation.idl:

Canonical link: https://commits.webkit.org/276478@main
@webkit-commit-queue webkit-commit-queue force-pushed the pgriffis/navigation/navigation-refactor branch from 6b35672 to 30c07bf Compare March 21, 2024 18:20
@webkit-commit-queue
Copy link
Copy Markdown
Collaborator

Committed 276478@main (30c07bf): https://commits.webkit.org/276478@main

Reviewed commits have been landed. Closing PR #26054 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Mar 21, 2024
@webkit-commit-queue webkit-commit-queue merged commit 30c07bf into WebKit:main Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants