All interface objects now share the same hasInstance #9633

GuillaumeGomez · 2016-02-14T14:56:47Z

highfive · 2016-02-14T14:56:50Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

GuillaumeGomez · 2016-02-14T14:57:29Z

components/script/dom/bindings/interface.rs

@@ -273,22 +278,25 @@ pub unsafe fn has_instance(
    }
    let mut value = RootedObject::new(cx, value.to_object());

-    // Steps 2-3 only concern callback interface objects.
+    let class = JS_GetClass(*prototype.ptr);
+    if let Ok(object) = get_dom_class(UncheckedUnwrapObject(class, 0)) {


How do you get NonCallbackInterfaceObjectClass instance here?

&*(&class as *const _ as *const NonCallbackInterfaceObjectClass)

nox · 2016-02-14T20:59:35Z

Great work, just a few corrections needed.

-S-awaiting-review +S-needs-code-changes

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 15 unresolved discussions.

components/script/dom/bindings/codegen/CodegenRust.py, line 1898 [r2] (raw file):
depth

components/script/dom/bindings/codegen/CodegenRust.py, line 1902 [r2] (raw file):
Everything related to hasInstance should go away from here.

components/script/dom/bindings/codegen/CodegenRust.py, line 4740 [r2] (raw file):
With this patch, this whole class should go away.

components/script/dom/bindings/codegen/CodegenRust.py, line 5363 [r2] (raw file):
No need to import this here, nor has_instance.

components/script/dom/bindings/interface.rs, line 282 [r1] (raw file):
Call these two variables js_class and object_class.

components/script/dom/bindings/interface.rs, line 88 [r2] (raw file):
This type isn't useful anymore and can go away.

components/script/dom/bindings/interface.rs, line 107 [r2] (raw file):
"The prototype ID of that interface, used in the hasInstance hook."

components/script/dom/bindings/interface.rs, line 109 [r2] (raw file):
"The prototype depth of that interface, used in the hasInstance hook."

components/script/dom/bindings/interface.rs, line 110 [r2] (raw file):
proto_depth: u16

components/script/dom/bindings/interface.rs, line 119 [r2] (raw file):
There shouldn't be a variable has_instance parameter anymore, instead class.hasInstance should be set to has_instance_hook.

components/script/dom/bindings/interface.rs, line 122 [r2] (raw file):
proto_depth

components/script/dom/bindings/interface.rs, line 126 [r2] (raw file):
Nit: could you put these two fields after the class one? It weirds me out.

components/script/dom/bindings/interface.rs, line 271 [r2] (raw file):
has_instance_hook, and it doesn't need to be public anymore.
Also nit: please indent the arguments like I did for the other functions in that module.

components/script/dom/bindings/interface.rs, line 286 [r2] (raw file):
No need to be public anymore.

components/script/dom/bindings/interface.rs, line 302 [r2] (raw file):
[object_class.proto_depth as usize]

Comments from the review on Reviewable.io

nox · 2016-02-14T21:07:33Z

-S-awaiting-review +S-needs-code-changes

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 13 unresolved discussions.

Comments from the review on Reviewable.io

nox · 2016-02-14T22:30:07Z

@bors-servo try

bors-servo · 2016-02-14T22:30:09Z

⌛ Trying commit 67b5221 with merge 2afd08e...


        Auto merge of #9633 - GuillaumeGomez:has_instance, r=<try>

@nox

All interface objects now share the same hasInstance r? @nox  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9633)

bors-servo · 2016-02-14T23:17:36Z

💔 Test failed - linux-rel

bors-servo · 2016-02-15T16:24:14Z

☔ The latest upstream changes (presumably #9645) made this pull request unmergeable. Please resolve the merge conflicts.

GuillaumeGomez · 2016-02-15T16:51:32Z

Re-updated.

KiChjang · 2016-02-16T11:02:15Z

@bors-servo try

bors-servo · 2016-02-16T11:02:18Z

⌛ Trying commit 857cf87 with merge fd8f228...


        Auto merge of #9633 - GuillaumeGomez:has_instance, r=<try>

@nox

All interface objects now share the same hasInstance r? @nox  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9633)

bors-servo · 2016-02-16T11:50:10Z

💔 Test failed - linux-rel


        All interface objects now share the same hasInstance

KiChjang · 2016-02-19T08:29:35Z

@bors-servo retry

#9148, #9699

bors-servo · 2016-02-19T08:32:58Z

⌛ Testing commit bb9d369 with merge 778230d...


        Auto merge of #9633 - GuillaumeGomez:has_instance, r=nox

@nox

All interface objects now share the same hasInstance r? @nox  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9633)

bors-servo · 2016-02-19T09:21:16Z

💔 Test failed - linux-rel

KiChjang · 2016-02-19T09:24:42Z

@bors-servo retry

#8191

bors-servo · 2016-02-19T10:59:24Z

⌛ Testing commit bb9d369 with merge 691eeb3...


        Auto merge of #9633 - GuillaumeGomez:has_instance, r=nox

@nox

All interface objects now share the same hasInstance r? @nox  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9633)

bors-servo · 2016-02-19T11:51:13Z

💔 Test failed - mac-rel-wpt

KiChjang · 2016-02-19T12:01:47Z

@bors-servo retry

#9063

bors-servo · 2016-02-19T12:42:45Z

⚡ Previous build results for android, gonk, linux-dev, mac-dev-unit, mac-rel-css are reusable. Rebuilding only linux-rel, mac-rel-wpt...

bors-servo · 2016-02-19T13:30:58Z

💔 Test failed - linux-rel

KiChjang · 2016-02-19T13:33:06Z

@bors-servo retry

#8191, #8127

bors-servo · 2016-02-19T14:19:34Z

⌛ Testing commit bb9d369 with merge 2676307...


        Auto merge of #9633 - GuillaumeGomez:has_instance, r=nox

@nox

All interface objects now share the same hasInstance r? @nox  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9633)

bors-servo · 2016-02-19T15:15:45Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt

highfive added the S-awaiting-review label Feb 14, 2016

GuillaumeGomez reviewed Feb 14, 2016
View changes

nox added S-needs-code-changes and removed S-awaiting-review labels Feb 14, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Feb 14, 2016

nox added S-needs-code-changes and removed S-awaiting-review labels Feb 14, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Feb 14, 2016

highfive added the S-tests-failed label Feb 14, 2016

highfive added S-needs-rebase and removed S-tests-failed labels Feb 15, 2016

jdm removed the S-needs-rebase label Feb 15, 2016

highfive added the S-tests-failed label Feb 16, 2016

All interface objects now share the same hasInstance

Loading status checks…

bb9d369

KiChjang assigned nox Feb 17, 2016

highfive added the S-tests-failed label Feb 19, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Feb 19, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 19, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Feb 19, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 19, 2016

highfive added the S-tests-failed label Feb 19, 2016

nox added S-awaiting-merge and removed S-tests-failed labels Feb 19, 2016

bors-servo merged commit bb9d369 into servo:master Feb 19, 2016
2 checks passed

2 checks passed

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

homu Test successful
Details

nox mentioned this pull request Feb 19, 2016

Make all interface objects share the same hasInstance hook in codegen #9281

Closed

GuillaumeGomez deleted the GuillaumeGomez:has_instance branch Feb 23, 2016

servo / servo

All interface objects now share the same hasInstance #9633

All interface objects now share the same hasInstance #9633

GuillaumeGomez commented Feb 14, 2016

highfive commented Feb 14, 2016

This comment has been minimized.

This comment has been minimized.

nox commented Feb 14, 2016

nox commented Feb 14, 2016

nox commented Feb 14, 2016

bors-servo commented Feb 14, 2016

bors-servo commented Feb 14, 2016

bors-servo commented Feb 15, 2016

GuillaumeGomez commented Feb 15, 2016

KiChjang commented Feb 16, 2016

bors-servo commented Feb 16, 2016

bors-servo commented Feb 16, 2016

KiChjang commented Feb 19, 2016

bors-servo commented Feb 19, 2016

bors-servo commented Feb 19, 2016

KiChjang commented Feb 19, 2016

bors-servo commented Feb 19, 2016

bors-servo commented Feb 19, 2016

KiChjang commented Feb 19, 2016

bors-servo commented Feb 19, 2016

bors-servo commented Feb 19, 2016

KiChjang commented Feb 19, 2016

bors-servo commented Feb 19, 2016

bors-servo commented Feb 19, 2016

servo / servo

Join GitHub today

All interface objects now share the same hasInstance #9633

All interface objects now share the same hasInstance #9633

Conversation

GuillaumeGomez commented Feb 14, 2016

highfive commented Feb 14, 2016

This comment has been minimized.

GuillaumeGomez Feb 14, 2016

This comment has been minimized.

nox Feb 14, 2016

nox commented Feb 14, 2016

nox commented Feb 14, 2016

nox commented Feb 14, 2016

bors-servo commented Feb 14, 2016

bors-servo commented Feb 14, 2016

bors-servo commented Feb 15, 2016

GuillaumeGomez commented Feb 15, 2016

KiChjang commented Feb 16, 2016

bors-servo commented Feb 16, 2016

bors-servo commented Feb 16, 2016

KiChjang commented Feb 19, 2016

bors-servo commented Feb 19, 2016

bors-servo commented Feb 19, 2016

KiChjang commented Feb 19, 2016

bors-servo commented Feb 19, 2016

bors-servo commented Feb 19, 2016

KiChjang commented Feb 19, 2016

bors-servo commented Feb 19, 2016

bors-servo commented Feb 19, 2016

KiChjang commented Feb 19, 2016

bors-servo commented Feb 19, 2016

bors-servo commented Feb 19, 2016