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.

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

HTML: document.open() and global variables #10815

Merged
merged 6 commits into from
Aug 17, 2018
Merged

HTML: document.open() and global variables #10815

merged 6 commits into from
Aug 17, 2018

Conversation

annevk
Copy link
Member

@annevk annevk commented May 3, 2018 •
edited by wpt-pr-bot
Loading

@wpt-pr-bot wpt-pr-bot added the html label May 3, 2018
@wpt-pr-bot wpt-pr-bot requested review from ayg, jdm, jgraham and zqzhang May 3, 2018 08:13
@annevk
Copy link
Member Author

annevk commented May 3, 2018

(Marked "do not merge yet" as this requires a change in the HTML Standard to not allocate a new global. Appreciate the review though!)

@TimothyGu TimothyGu mentioned this pull request Jul 12, 2018
Closed
19 tasks
@TimothyGu TimothyGu self-assigned this Jul 31, 2018
@TimothyGu TimothyGu force-pushed the annevk/document-open-steps-global-replacement branch from 3b06a0c to f3ed415 Compare August 2, 2018 21:54
@wpt-pr-bot wpt-pr-bot requested review from domenic, foolip, sideshowbarker and zcorpan and removed request for ayg August 2, 2018 21:54
foolip
foolip reviewed Aug 3, 2018
View reviewed changes
html/webappapis/dynamic-markup-insertion/opening-the-input-stream/no-new-global.window.js Outdated
assert_equals(frame.contentWindow.hey, "You", "precondition");
frame.contentDocument.open();
assert_equals(frame.contentWindow.hey, "You", "actual check");
// Ensure a load event gets dispatched to unblock testharness
Copy link
Member

Choose a reason for hiding this comment

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

This test fails in Firefox, so this line wasn't reached and yet testharness wasn't blocked waiting for the load event. If this is just defensive coding, maybe put it in a cleanup step before the assert still?

Copy link
Member

Choose a reason for hiding this comment

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

That's because of the cleanup step I added earlier in the test with frame.remove(). I could remove this.

html/webappapis/dynamic-markup-insertion/opening-the-input-stream/no-new-global.window.js Outdated

testIdentity("Document", frame => frame.contentDocument, frame => frame.contentWindow.Document);
testIdentity("WindowProxy", frame => frame.contentWindow, frame => frame.contentWindow.Window);
testIdentity("Navigator", frame => frame.contentWindow.navigator, frame => frame.contentWindow.Navigator);
Copy link
Member

Choose a reason for hiding this comment

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

There's something a bit special about Location as well, see whatwg/html#2545. Are there any tests for Location that would make sense here?

Copy link
Member

Choose a reason for hiding this comment

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

Sure.

TimothyGu added a commit to TimothyGu/html that referenced this pull request Aug 14, 2018
@TimothyGu @annevk
Remove document.open() realm creation
7234c89 
Based on the work by Anne van Kesteren in whatwg#3651, but without the parts
concerning the session history. Changes to that area will come later
(see whatwg#3818).

Tests: web-platform-tests/wpt#10773
Tests: web-platform-tests/wpt#10778
Tests: web-platform-tests/wpt#10789
Tests: web-platform-tests/wpt#10815
Tests: web-platform-tests/wpt#10818

Fixes whatwg#1698.
Fixes whatwg#3286.
Fixes whatwg#3306.
Closes whatwg#3665.

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
TimothyGu added a commit to TimothyGu/html that referenced this pull request Aug 14, 2018
@TimothyGu @annevk
Remove document.open() realm creation
091ac08 
Based on the work by Anne van Kesteren in whatwg#3651, but without the parts
concerning the session history. Changes to that area will come later
(see whatwg#3818).

Tests: web-platform-tests/wpt#10773
Tests: web-platform-tests/wpt#10778
Tests: web-platform-tests/wpt#10815
Tests: web-platform-tests/wpt#10818

Fixes whatwg#1698.
Fixes whatwg#3286.
Fixes whatwg#3306.
Closes whatwg#3665.

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
@TimothyGu TimothyGu mentioned this pull request Aug 14, 2018
Merged
@TimothyGu TimothyGu force-pushed the annevk/document-open-steps-global-replacement branch from f3ed415 to 2e483da Compare August 15, 2018 15:56
@foolip
Copy link
Member

foolip commented Aug 15, 2018

Thanks @TimothyGu !

domenic pushed a commit to whatwg/html that referenced this pull request Aug 16, 2018
@TimothyGu @domenic
document.open() simplifications, part 1
6f769b8 
In particular, removes the realm creation, document unloading, and tasks
removal steps.

Based on the work by Anne van Kesteren in #3651, but without the parts
concerning the session history. Changes to that area will come later
(see #3818).

These changes allow us to remove several auxiliary concepts that only
existed to support document.open():

- The recycle parameter to the "unload a Document" algorithm
- The window parameter to the "set the active document" algorithm

Tests: web-platform-tests/wpt#10773
Tests: web-platform-tests/wpt#10778
Tests: web-platform-tests/wpt#10815
Tests: web-platform-tests/wpt#10818

Fixes #1698.
Fixes #3286.
Fixes #3306.
Closes #3665.
@TimothyGu
restore 005.html
0fc6858 
Its removal should be in tasks
@TimothyGu TimothyGu merged commit 70c15a6 into master Aug 17, 2018
@TimothyGu TimothyGu deleted the annevk/document-open-steps-global-replacement branch August 17, 2018 15:16
zcorpan pushed a commit that referenced this pull request Aug 27, 2018
@annevk @TimothyGu @zcorpan
HTML: document.open() and global variables (#10815)
81fcc63 
For whatwg/html#3918.

Several tests were removed as they no longer apply after the change in HTML. The expectations were reversed and moved to the new no-new-global.window.js file.

Co-authored-by: Timothy Gu <timothygu99@gmail.com>
mustaqahmed pushed a commit to mustaqahmed/html that referenced this pull request Feb 15, 2019
@TimothyGu @mustaqahmed
document.open() simplifications, part 1
d13d93e 
In particular, removes the realm creation, document unloading, and tasks
removal steps.

Based on the work by Anne van Kesteren in whatwg#3651, but without the parts
concerning the session history. Changes to that area will come later
(see whatwg#3818).

These changes allow us to remove several auxiliary concepts that only
existed to support document.open():

- The recycle parameter to the "unload a Document" algorithm
- The window parameter to the "set the active document" algorithm

Tests: web-platform-tests/wpt#10773
Tests: web-platform-tests/wpt#10778
Tests: web-platform-tests/wpt#10815
Tests: web-platform-tests/wpt#10818

Fixes whatwg#1698.
Fixes whatwg#3286.
Fixes whatwg#3306.
Closes whatwg#3665.
mustaqahmed pushed a commit to mustaqahmed/html that referenced this pull request Feb 15, 2019
@TimothyGu @mustaqahmed
document.open() simplifications, part 1
29c6fee 
In particular, removes the realm creation, document unloading, and tasks
removal steps.

Based on the work by Anne van Kesteren in whatwg#3651, but without the parts
concerning the session history. Changes to that area will come later
(see whatwg#3818).

These changes allow us to remove several auxiliary concepts that only
existed to support document.open():

- The recycle parameter to the "unload a Document" algorithm
- The window parameter to the "set the active document" algorithm

Tests: web-platform-tests/wpt#10773
Tests: web-platform-tests/wpt#10778
Tests: web-platform-tests/wpt#10815
Tests: web-platform-tests/wpt#10818

Fixes whatwg#1698.
Fixes whatwg#3286.
Fixes whatwg#3306.
Closes whatwg#3665.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyGu TimothyGu TimothyGu left review comments

@jdm jdm jdm approved these changes

@foolip foolip foolip approved these changes

@jgraham jgraham Awaiting requested review from jgraham

@zqzhang zqzhang Awaiting requested review from zqzhang

@domenic domenic Awaiting requested review from domenic

@sideshowbarker sideshowbarker Awaiting requested review from sideshowbarker

@zcorpan zcorpan Awaiting requested review from zcorpan

Assignees

@TimothyGu TimothyGu

Labels
html
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@annevk @foolip @jdm @TimothyGu @wpt-pr-bot