Initial implementation of ownPropertyKeys proxy handler #7254

frewsxcv · 2015-08-17T17:35:31Z

Generates SupportedPropertyNames on DOM structs that should implement
it. Most of them are unimplemented now (which can be implemented in
later PRs), with the exception of HTMLCollection. Also added a couple
relevant WPT tests.

Closes #6390

Closes #2215

frewsxcv · 2015-08-17T17:36:12Z

components/script/dom/bindings/codegen/CodegenRust.py

+                    let mut jsid = js::jsapi::RootedId::new(cx, jsid {asBits: i});
+                    js::glue::AppendToAutoIdVector(props, jsid.handle().get());
+                }
+                """)


Either something in the block above or below is incorrect

frewsxcv · 2015-08-17T17:37:02Z

Note that this currently compiles, but when running certain tests (like ./mach test-wpt tests/wpt/web-platform-tests/html/dom/documents/dom-tree-accessors/document.forms.html), it will segfault

Ms2ger · 2015-08-18T11:36:18Z

@frewsxcv rebase on master and use int_to_jsid(i as i32) (and use use)

frewsxcv · 2015-08-18T13:42:59Z

rebase on master and use int_to_jsid(i as i32) (and use use)

Awesome, that seems to have fixed the issues as I was having. Thanks for adding that in to rust-mozjs :)

This should be ready for a review now

frewsxcv · 2015-08-18T14:02:16Z

Oh, actually, let me do another force push to utilize imports instead of going through namespace hell

frewsxcv · 2015-08-18T14:14:38Z

let me do another force push

Done

Ms2ger · 2015-08-18T15:54:50Z

Reviewed 7 of 13 files at r1, 2 of 3 files at r2.
Review status: 9 of 13 files reviewed at latest revision, 11 unresolved discussions, all commit checks successful.

^{components/script/dom/bindings/codegen/CodegenRust.py, line 4185 [r2] (raw file):}
Please call it own_property_keys per Rust conventions.

^{components/script/dom/bindings/codegen/CodegenRust.py, line 4196 [r2] (raw file):}
Why the r""?

^{components/script/dom/bindings/codegen/CodegenRust.py, line 4199 [r2] (raw file):}
let rooted_jsid = RootedId::new(cx, int_to_jsid(i as i32));

^{components/script/dom/bindings/codegen/CodegenRust.py, line 4209 [r2] (raw file):}
You're leaking the string here. Also, use JS_InternStringN.

^{components/script/dom/bindings/codegen/CodegenRust.py, line 4219 [r2] (raw file):}
Looks like we're missing expandos here. Add a test.

^{components/script/dom/document.rs, line 1918 [r2] (raw file):}
File an issue

^{components/script/dom/domstringmap.rs, line 74 [r2] (raw file):}
Ditto

^{components/script/dom/htmlcollection.rs, line 232 [r2] (raw file):}
Sounds bad.

^{components/script/dom/htmlcollection.rs, line 238 [r2] (raw file):}
Any reason to use fold() over a loop?

^{components/script/dom/htmlcollection.rs, line 240 [r2] (raw file):}
Only for HTML elements. Add a test.

^{components/script/dom/htmlcollection.rs, line 243 [r2] (raw file):}
Order is detectable, so swap those.

Comments from the review on Reviewable.io

Ms2ger · 2015-08-18T16:45:49Z

Reviewed 6 of 13 files at r1.
Review status: 12 of 13 files reviewed at latest revision, 11 unresolved discussions, all commit checks successful.

^{tests/wpt/web-platform-tests/dom/collections/HTMLCollection-supported-property-names.html, line 31 [r1] (raw file):}
I don't think there's a need to sort.

Comments from the review on Reviewable.io

frewsxcv · 2015-08-18T16:54:26Z

^{tests/wpt/web-platform-tests/dom/collections/HTMLCollection-supported-property-names.html, line 31 [r1] (raw file):}
[https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/getOwnPropertyNames#Description](This paragraph) suggests that the order of the non-enumerable elements is not defined, and since non-enumerable elements, are part of the same array as the enumerable elements, the array as a whole has undefined ordering

Comments from the review on Reviewable.io

frewsxcv · 2015-08-18T16:55:23Z

^{tests/wpt/web-platform-tests/dom/collections/HTMLCollection-supported-property-names.html, line 31 [r1] (raw file):}
I messed up the link in that last comment. Click here for the actual location

Comments from the review on Reviewable.io

frewsxcv · 2015-08-18T16:58:44Z

^{components/script/dom/htmlcollection.rs, line 232 [r2] (raw file):}
I agree.

I used it because we use this pattern:

let Collection(ref root, ref filter) = self.collection;

and I'm not sure how to please the unrooted_must_root lint when doing this.

It is also used above a few times for the same reason.

Comments from the review on Reviewable.io

Ms2ger · 2015-08-18T17:16:01Z

Review status: 12 of 13 files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

^{components/script/dom/htmlcollection.rs, line 232 [r2] (raw file):}
Ugh. I'm not sure how I let that slip by. You can use let root = self.collection.0.root();, I think.

^{tests/wpt/web-platform-tests/dom/collections/HTMLCollection-supported-property-names.html, line 31 [r1] (raw file):}
Don't implement things based on docs, that's what the spec is for: https://heycam.github.io/webidl/#property-enumeration

Comments from the review on Reviewable.io

frewsxcv · 2015-08-18T17:19:27Z

^{components/script/dom/htmlcollection.rs, line 243 [r2] (raw file):}
Sorry, what do you mean by this? What is order referring to?

Comments from the review on Reviewable.io

frewsxcv · 2015-08-18T17:23:18Z

^{tests/wpt/web-platform-tests/dom/collections/HTMLCollection-supported-property-names.html, line 0 [r1] (raw file):}

Finally, any enumerable own properties or properties from the object’s prototype chain are then enumerated, in no defined order.

Considering this, doesn't my previous comment still apply?

Comments from the review on Reviewable.io

Ms2ger · 2015-08-18T17:24:18Z

Review status: 12 of 13 files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

^{components/script/dom/htmlcollection.rs, line 243 [r2] (raw file):}
Consider <span id=a name=b>. The spec says we should return [a, b], not [b, a].

Comments from the review on Reviewable.io

frewsxcv · 2015-08-18T17:26:08Z

^{components/script/dom/htmlcollection.rs, line 0 [r2] (raw file):}
Ah, gotcha. Good catch. Thanks!

Comments from the review on Reviewable.io

Ms2ger · 2015-08-20T14:55:03Z

@bors-servo r+

bors-servo · 2015-08-20T14:55:04Z

📌 Commit aca8e1a has been approved by Ms2ger

bors-servo · 2015-08-20T15:19:07Z

⌛ Testing commit aca8e1a with merge 8b7eb77...


        Auto merge of #7254 - frewsxcv:own-property-keys, r=Ms2ger

Initial implementation of ownPropertyKeys proxy handler Generates `SupportedPropertyNames` on DOM structs that should implement it. Most of them are unimplemented now (which can be implemented in later PRs), with the exception of `HTMLCollection`. Also added a couple relevant WPT tests. Closes #6390 Closes #2215  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7254)

bors-servo · 2015-08-20T15:38:00Z

💔 Test failed - linux2

frewsxcv · 2015-08-20T15:47:25Z

PASS expected FAIL document.forms getOwnPropertyNames

The best kind of test fails 👍


        Initial implementation of ownPropertyKeys proxy handler

Generates `SupportedPropertyNames` on DOM structs that should implement it. Most of them are unimplemented now (which can be implemented in later PRs), with the exception of `HTMLCollection`. Also added a couple relevant WPT tests. Closes #6390 Closes #2215

frewsxcv · 2015-08-20T15:59:02Z

In the latest force push: Marked the test as passing and got rid of an unused import

Ms2ger · 2015-08-20T16:00:41Z

@bors-servo r+

bors-servo · 2015-08-20T16:00:43Z

📌 Commit b11be4d has been approved by Ms2ger

bors-servo · 2015-08-20T16:00:45Z

⌛ Testing commit b11be4d with merge d2a8c27...


        Auto merge of #7254 - frewsxcv:own-property-keys, r=Ms2ger

Initial implementation of ownPropertyKeys proxy handler Generates `SupportedPropertyNames` on DOM structs that should implement it. Most of them are unimplemented now (which can be implemented in later PRs), with the exception of `HTMLCollection`. Also added a couple relevant WPT tests. Closes #6390 Closes #2215  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7254)

bors-servo · 2015-08-20T16:34:13Z

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2, mac3


        Bump js (rust-mozjs) to the latest commit

I need the constants added in servo/rust-mozjs#191 for servo#7254


        servo: Merge #7285 - Bump js (rust-mozjs) to the latest commit (from …

…frewsxcv:bump-mozjs); r=Ms2ger I need the constants added in servo/rust-mozjs#191 for servo/servo#7254 Source-Repo: https://github.com/servo/servo Source-Revision: 14b921ee29b1b5e46e8773836b5a31e85faabfd5


        servo: Merge #7285 - Bump js (rust-mozjs) to the latest commit (from …

…frewsxcv:bump-mozjs); r=Ms2ger I need the constants added in servo/rust-mozjs#191 for servo/servo#7254 Source-Repo: https://github.com/servo/servo Source-Revision: 14b921ee29b1b5e46e8773836b5a31e85faabfd5 UltraBlame original commit: 45150e0a57bd5d0cbe5f05c570e4632325406e3d


        servo: Merge #7285 - Bump js (rust-mozjs) to the latest commit (from …

…frewsxcv:bump-mozjs); r=Ms2ger I need the constants added in servo/rust-mozjs#191 for servo/servo#7254 Source-Repo: https://github.com/servo/servo Source-Revision: 14b921ee29b1b5e46e8773836b5a31e85faabfd5 UltraBlame original commit: 45150e0a57bd5d0cbe5f05c570e4632325406e3d


        servo: Merge #7285 - Bump js (rust-mozjs) to the latest commit (from …

…frewsxcv:bump-mozjs); r=Ms2ger I need the constants added in servo/rust-mozjs#191 for servo/servo#7254 Source-Repo: https://github.com/servo/servo Source-Revision: 14b921ee29b1b5e46e8773836b5a31e85faabfd5 UltraBlame original commit: 45150e0a57bd5d0cbe5f05c570e4632325406e3d

highfive added the S-awaiting-review label Aug 17, 2015

frewsxcv reviewed Aug 17, 2015
View changes

frewsxcv force-pushed the frewsxcv:own-property-keys branch 2 times, most recently from c969506 to f5dc1e7 Aug 17, 2015

Ms2ger self-assigned this Aug 18, 2015

Ms2ger added S-needs-code-changes S-needs-rebase and removed S-awaiting-review labels Aug 18, 2015

frewsxcv force-pushed the frewsxcv:own-property-keys branch from f5dc1e7 to 14e7692 Aug 18, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Aug 18, 2015

frewsxcv removed the S-needs-rebase label Aug 18, 2015

frewsxcv force-pushed the frewsxcv:own-property-keys branch from 14e7692 to 54da950 Aug 18, 2015

Ms2ger added S-needs-code-changes and removed S-awaiting-review labels Aug 18, 2015

frewsxcv removed the S-needs-rebase label Aug 20, 2015

highfive added S-awaiting-merge and removed S-awaiting-review labels Aug 20, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 20, 2015

frewsxcv force-pushed the frewsxcv:own-property-keys branch from aca8e1a to b11be4d Aug 20, 2015

highfive added S-awaiting-review and removed S-tests-failed labels Aug 20, 2015

highfive removed the S-awaiting-review label Aug 20, 2015

highfive added the S-awaiting-merge label Aug 20, 2015

bors-servo merged commit b11be4d into servo:master Aug 20, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

frewsxcv deleted the frewsxcv:own-property-keys branch Aug 20, 2015

josiahdaniels added a commit to josiahdaniels/servo that referenced this pull request Sep 28, 2015

Bump js (rust-mozjs) to the latest commit

6787989

I need the constants added in servo/rust-mozjs#191 for servo#7254

servo / servo

Initial implementation of ownPropertyKeys proxy handler #7254

Initial implementation of ownPropertyKeys proxy handler #7254

frewsxcv commented Aug 17, 2015

This comment has been minimized.

frewsxcv commented Aug 17, 2015

Ms2ger commented Aug 18, 2015

frewsxcv commented Aug 18, 2015

frewsxcv commented Aug 18, 2015

frewsxcv commented Aug 18, 2015

Ms2ger commented Aug 18, 2015

Ms2ger commented Aug 18, 2015

frewsxcv commented Aug 18, 2015

frewsxcv commented Aug 18, 2015

frewsxcv commented Aug 18, 2015

Ms2ger commented Aug 18, 2015

frewsxcv commented Aug 18, 2015

frewsxcv commented Aug 18, 2015

Ms2ger commented Aug 18, 2015

frewsxcv commented Aug 18, 2015

Ms2ger commented Aug 20, 2015

bors-servo commented Aug 20, 2015

bors-servo commented Aug 20, 2015

bors-servo commented Aug 20, 2015

frewsxcv commented Aug 20, 2015

frewsxcv commented Aug 20, 2015

Ms2ger commented Aug 20, 2015

bors-servo commented Aug 20, 2015

bors-servo commented Aug 20, 2015

bors-servo commented Aug 20, 2015

servo / servo

Join GitHub today

Initial implementation of ownPropertyKeys proxy handler #7254

Initial implementation of ownPropertyKeys proxy handler #7254

Conversation

frewsxcv commented Aug 17, 2015

This comment has been minimized.

frewsxcv Aug 17, 2015

frewsxcv commented Aug 17, 2015

Ms2ger commented Aug 18, 2015

frewsxcv commented Aug 18, 2015

frewsxcv commented Aug 18, 2015

frewsxcv commented Aug 18, 2015

Ms2ger commented Aug 18, 2015

Ms2ger commented Aug 18, 2015

frewsxcv commented Aug 18, 2015

frewsxcv commented Aug 18, 2015

frewsxcv commented Aug 18, 2015

Ms2ger commented Aug 18, 2015

frewsxcv commented Aug 18, 2015

frewsxcv commented Aug 18, 2015

Ms2ger commented Aug 18, 2015

frewsxcv commented Aug 18, 2015

Ms2ger commented Aug 20, 2015

bors-servo commented Aug 20, 2015

bors-servo commented Aug 20, 2015

bors-servo commented Aug 20, 2015

frewsxcv commented Aug 20, 2015

frewsxcv commented Aug 20, 2015

Ms2ger commented Aug 20, 2015

bors-servo commented Aug 20, 2015

bors-servo commented Aug 20, 2015

bors-servo commented Aug 20, 2015