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

Make window.window/frames/self deal with no browsing context #4410

Merged
merged 1 commit into from
Oct 16, 2019
Merged

Make window.window/frames/self deal with no browsing context #4410

merged 1 commit into from
Oct 16, 2019

Conversation

annevk
Copy link
Member

@annevk annevk commented Mar 4, 2019 •
edited by pr-preview bot
Loading

Tests: ...

Helps with #4363.

/infrastructure.html ( diff )
/window-object.html ( diff )

@annevk
Copy link
Member Author

annevk commented Mar 4, 2019

@bzbarsky Firefox is the only browser to not do this. It's a little bit pointless as @domenic pointed out (just keep the WindowProxy you got), but on the other hand it's nice not to have to store an explicit pointer from Window to WindowProxy.

@domenic domenic added topic: browsing context needs tests Moving the issue forward requires someone to write tests labels Mar 6, 2019
@annevk
Copy link
Member Author

annevk commented Mar 7, 2019 •
edited
Loading

I've reversed my opinion given tc39/ecma262#702 (globalThis is never null). I also found that in Chrome self does not become null. So Chrome and Safari are rather inconsistent here. [I'm not sure what's going on, but Chrome now agrees with Firefox. Perhaps I should not test in Live DOM Viewer... Only Safari has these as null, but globalThis non-null.]

@annevk annevk added the do not merge yet Pull request must not be merged per rationale in comment label Mar 7, 2019
@annevk annevk force-pushed the annevk/window.self-frames-window branch from 0c15092 to 2938b37 Compare March 7, 2019 12:34
@annevk annevk mentioned this pull request Mar 7, 2019
Merged
annevk added a commit to web-platform-tests/wpt that referenced this pull request Mar 7, 2019
@annevk
HTML: test WindowProxy self references
b0806cd 
For whatwg/html#4410.
@annevk annevk mentioned this pull request Mar 7, 2019
Merged
@annevk
Copy link
Member Author

annevk commented Mar 7, 2019

Tests: web-platform-tests/wpt#15720.

Safari bug: https://bugs.webkit.org/show_bug.cgi?id=195406.

@annevk annevk removed the needs tests Moving the issue forward requires someone to write tests label Mar 7, 2019
@annevk annevk mentioned this pull request Mar 7, 2019
Merged
@annevk annevk requested a review from domenic March 7, 2019 13:58
@annevk annevk force-pushed the annevk/window.self-frames-window branch from 2938b37 to 5e12e95 Compare March 7, 2019 16:30
domenic
domenic approved these changes Mar 7, 2019
View reviewed changes
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM but I guess blocked on TC39 and I agree it would be nice if they put the global this value somewhere besides the intrinsics table.

annevk added a commit to web-platform-tests/wpt that referenced this pull request Mar 13, 2019
@annevk
HTML: test WindowProxy self references
1a896b5 
For whatwg/html#4410.
@cdumez
Copy link

cdumez commented Mar 29, 2019 •
edited
Loading

So all browsers seem to fail the new checks with "after discarded", is this intentional?

They are passing in WebKit with my local patch to fix this though.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 1, 2019
@annevk @jgraham
Bug 1534200 [wpt PR 15720] - HTML: test WindowProxy self references, …
33f79fb 
…a=testonly

Automatic update from web-platform-tests
HTML: test WindowProxy self references

For whatwg/html#4410.
--

wpt-commits: 1a896b5997ef877e8f6e3005176857f615d58cd3
wpt-pr: 15720
@annevk
Copy link
Member Author

annevk commented Apr 1, 2019

@cdumez could you be more specific? When I run /html/browsers/the-window-object/self-et-al.window.html locally, latest Chrome and Firefox pass all tests.

@cdumez
Copy link

cdumez commented Apr 1, 2019

@cdumez could you be more specific? When I run /html/browsers/the-window-object/self-et-al.window.html locally, latest Chrome and Firefox pass all tests.

Oh indeed, I am unable to reproduce today. All tests indeed pass in Firefox and Chrome. Not sure why I was seeing different last week. Sorry about the noise.

mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Apr 2, 2019
@annevk @jgraham
Bug 1534200 [wpt PR 15720] - HTML: test WindowProxy self references, …
c58d72f 
…a=testonly

Automatic update from web-platform-tests
HTML: test WindowProxy self references

For whatwg/html#4410.
--

wpt-commits: 1a896b5997ef877e8f6e3005176857f615d58cd3
wpt-pr: 15720
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 23, 2019
@annevk
HTML: test WindowProxy self references
03f64e1 
For whatwg/html#4410.
@annevk annevk force-pushed the annevk/window.self-frames-window branch from 5e12e95 to e9f6d61 Compare October 2, 2019 16:17
@annevk annevk requested a review from domenic October 2, 2019 16:17
@annevk
Copy link
Member Author

annevk commented Oct 2, 2019

I adjusted the text and commit message to align with the recent addition of globalThis. Should be all good now, I hope.

(The amount of bookkeeping objects in the ECMAScript standard is too high.)

gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 4, 2019
@marco-c
Bug 1534200 [wpt PR 15720] - HTML: test WindowProxy self references, …
8abba64 
…a=testonly

Automatic update from web-platform-tests
HTML: test WindowProxy self references

For whatwg/html#4410.
--

wpt-commits: 1a896b5997ef877e8f6e3005176857f615d58cd3
wpt-pr: 15720

UltraBlame original commit: 94c16b51d515e3fb32be07bf02d8832d97161b26
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 4, 2019
@marco-c
Bug 1534200 [wpt PR 15720] - HTML: test WindowProxy self references, …
29628df 
…a=testonly

Automatic update from web-platform-tests
HTML: test WindowProxy self references

For whatwg/html#4410.
--

wpt-commits: 1a896b5997ef877e8f6e3005176857f615d58cd3
wpt-pr: 15720

UltraBlame original commit: 94c16b51d515e3fb32be07bf02d8832d97161b26
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 4, 2019
@marco-c
Bug 1534200 [wpt PR 15720] - HTML: test WindowProxy self references, …
aed973c 
…a=testonly

Automatic update from web-platform-tests
HTML: test WindowProxy self references

For whatwg/html#4410.
--

wpt-commits: 1a896b5997ef877e8f6e3005176857f615d58cd3
wpt-pr: 15720

UltraBlame original commit: 94c16b51d515e3fb32be07bf02d8832d97161b26
@annevk annevk removed the do not merge yet Pull request must not be merged per rationale in comment label Oct 6, 2019
domenic
domenic approved these changes Oct 16, 2019
View reviewed changes
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

So the difference here is that before "this Window object's browsing context" would sometimes be null, causing a spec crash, right? Please include that in the commit message since, coming back to this with fresh eyes, it's not obvious.

But yeah, spec text looks great! If a bit hilariously long.

@annevk annevk merged commit 89b7e23 into master Oct 16, 2019
@annevk annevk deleted the annevk/window.self-frames-window branch October 16, 2019 07:36
@jdm jdm mentioned this pull request Oct 16, 2019
Open
This was referenced Apr 13, 2020
Open
Closed
@hober hober mentioned this pull request Apr 13, 2020
Closed
3 tasks
@nt1m nt1m mentioned this pull request Sep 21, 2023
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domenic domenic domenic approved these changes

Assignees
No one assigned
Labels
topic: browsing context
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@annevk @cdumez @domenic