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

Enumerate less cross-origin properties #3186

Merged
merged 2 commits into from Nov 6, 2017

Conversation

@annevk
Copy link
Member

commented Nov 2, 2017

In 205659f we made all properties on cross-origin objects enumerable, equivalent to their same-origin object counterparts.

However, this turned out not be web-compatible. This makes them unenumerable again with the exception of array index property names, which need to be enumerable.

Tests: ...

Fixes #3183.

In 205659f we made all properties on cross-origin objects enumerable, equivalent to their same-origin object counterparts.

However, this turned out not be web-compatible. This makes them unenumerable again with the exception of array index property names, which need to be enumerable.

Tests: ...

Fixes #3183.
@annevk

This comment has been minimized.

Copy link
Member Author

commented Nov 2, 2017

(That should probably read "Enumerate fewer ..." looking at it again.)

annevk added a commit to web-platform-tests/wpt that referenced this pull request Nov 2, 2017
See whatwg/html#3186 for context.
@annevk

This comment has been minimized.

Copy link
Member Author

commented Nov 2, 2017

Tests are at web-platform-tests/wpt#8045.

Review on those and this PR appreciated @bzbarsky and @cdumez.

Given that Firefox and Safari are actively involved, we'll only need to update these bugs once this is merged:

(Brief testing shows at least Chrome still needing changes as array index property names are not enumerable currently.)

@bzbarsky

This comment has been minimized.

Copy link
Collaborator

commented Nov 2, 2017

@annevk Thank you for fixing this. Is it at all possible to see what the rendered version looks like? Reviewing the diff is a bit of a pain, especially with github refusing to expand the context...

@annevk

This comment has been minimized.

Copy link
Member Author

commented Nov 3, 2017

I flipped the third and fourth instance of [[Enumerable]] on https://html.spec.whatwg.org/multipage/browsers.html and the second instance on https://html.spec.whatwg.org/multipage/window-object.html. Let me know if that helps. If it doesn't I can see about generating a copy (I'm still transitioning setups so I don't have everything up and running locally).

Copy link
Collaborator

left a comment

Looks good assuming I got the right steps in #3186 (comment)

@domenic

This comment has been minimized.

Copy link
Member

commented Nov 3, 2017

Should we add a note or example about how this inconsistency is required for web compat? Or at least a HTML comment so we don't forget and try to "fix" things again in 5 years?

Happy to do that as a follow-up.

@annevk

This comment has been minimized.

Copy link
Member Author

commented Nov 4, 2017

If you have a suggested place I'm happy to add something. Or you can push a fixup. I don't think we need to land this now, but somewhere within the next week would be good.

cdumez added a commit to web-platform-tests/wpt that referenced this pull request Nov 4, 2017
* Enumerate fewer cross-origin properties

See whatwg/html#3186 for context.

* adjust Object.keys() subtests as well
hubot pushed a commit to WebKit/webkit that referenced this pull request Nov 5, 2017
https://bugs.webkit.org/show_bug.cgi?id=179289

Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Re-sync WPT test after:
- web-platform-tests/wpt#8045

Rebaseline a couple of WPT tests now that more checks are passing.

* web-platform-tests/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt:
* web-platform-tests/html/browsers/origin/cross-origin-objects/cross-origin-objects.html:
* web-platform-tests/html/browsers/the-window-object/window-indexed-properties-expected.txt:

Source/WebCore:

Index properties on cross origin Window objects should be enumerable:
- whatwg/html#3186
- web-platform-tests/wpt#8045

All exposed properties used to be enumerable but we had to revert this in
r224287 because it was not Web-compatible. The HTML specification has now
been updated so that only index properties are enumerable cross origin.

No new tests, rebaselined existing tests.

* bindings/js/JSDOMWindowCustom.cpp:
(WebCore::JSDOMWindow::getOwnPropertySlotByIndex):
(WebCore::JSDOMWindow::getOwnPropertyNames):

LayoutTests:

Update / rebaseline existing test to match new expected behavior.

* js/dom/getOwnPropertyDescriptor-expected.txt:
* js/resources/getOwnPropertyDescriptor.js:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@224464 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@domenic

This comment has been minimized.

Copy link
Member

commented Nov 6, 2017

Added a note; let me know what you think.

Copy link
Member Author

left a comment

LGTM. I cannot approve since I submitted the original PR. The only wonder I have is whether we should combine the two notes at the end of CrossOriginGetOwnPropertyHelper into a single <div class=note>.

@domenic

This comment has been minimized.

Copy link
Member

commented Nov 6, 2017

I think separate notes for separate topics is slightly nicer. So, I'll go ahead and merge this.

@domenic domenic merged commit ad88237 into master Nov 6, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@domenic domenic deleted the annevk/cross-origin-properties branch Nov 6, 2017
@annevk

This comment has been minimized.

Copy link
Member Author

commented Nov 6, 2017

Updated the Chrome and Edge bugs.

@bzbarsky

This comment has been minimized.

Copy link
Collaborator

commented Nov 6, 2017

Thank you very much for the quick response on this!

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 7, 2017
…erty enumerability. r=peterv

Updates to whatwg/html#3186

Includes the changes from web-platform-tests/wpt#8045

MozReview-Commit-ID: 5vEo1QGPufE
xeonchen pushed a commit to xeonchen/gecko-cinnabar that referenced this pull request Nov 7, 2017
…erty enumerability. r=peterv

Updates to whatwg/html#3186

Includes the changes from web-platform-tests/wpt#8045

MozReview-Commit-ID: 5vEo1QGPufE
JerryShih pushed a commit to JerryShih/gecko-dev that referenced this pull request Nov 13, 2017
…erty enumerability. r=peterv

Updates to whatwg/html#3186

Includes the changes from web-platform-tests/wpt#8045

MozReview-Commit-ID: 5vEo1QGPufE
aethanyc pushed a commit to aethanyc/gecko-dev that referenced this pull request Nov 16, 2017
…erty enumerability. r=peterv

Updates to whatwg/html#3186

Includes the changes from web-platform-tests/wpt#8045

MozReview-Commit-ID: 5vEo1QGPufE
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 2, 2019
…erty enumerability. r=peterv

Updates to whatwg/html#3186

Includes the changes from web-platform-tests/wpt#8045

MozReview-Commit-ID: 5vEo1QGPufE

UltraBlame original commit: 2a5a80d284b5ef958298cf515867ffbe420478c0
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 2, 2019
…erty enumerability. r=peterv

Updates to whatwg/html#3186

Includes the changes from web-platform-tests/wpt#8045

MozReview-Commit-ID: 5vEo1QGPufE

UltraBlame original commit: 2a5a80d284b5ef958298cf515867ffbe420478c0
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 2, 2019
…erty enumerability. r=peterv

Updates to whatwg/html#3186

Includes the changes from web-platform-tests/wpt#8045

MozReview-Commit-ID: 5vEo1QGPufE

UltraBlame original commit: 2a5a80d284b5ef958298cf515867ffbe420478c0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.