Enter dom compartment wrapper #23344

KaczuH · 2019-05-08T07:00:13Z

I'm still not sure if the changes are entirely correct.
Replaced occurrences:
JSAutoCompartment::new(global.get_cx(), global.reflector().get_jsobject().get()); with fn enter_compartment(object: &DomObject) -> JSAutoCompartment

There are still occurrences of JSAutoCompartment that i was unable to replace. Could anyone give me a hint if it is possible?

→ rg -Fi --type rust "JSAutoCompartment::"
components/script/compartments.rs
38:    JSAutoCompartment::new(

components/script/dom/create.rs
159:                                let _ac = JSAutoCompartment::new(

components/script/dom/eventtarget.rs
527:                let _ac = JSAutoCompartment::new(cx, self.reflector().get_jsobject().get());

components/script/dom/windowproxy.rs
223:            let _ac = JSAutoCompartment::new(cx, window_jsobject.get());

components/script/dom/customelementregistry.rs
337:            let _ac = JSAutoCompartment::new(cx, proto_object.get());
349:            let _ac = JSAutoCompartment::new(cx, constructor.get());
538:            let _ac = JSAutoCompartment::new(cx, self.constructor.callback());
668:        let _ac = JSAutoCompartment::new(cx, constructor.callback());

components/script/dom/window.rs
2216:            let _ac = JSAutoCompartment::new(cx, obj.get());

components/script/dom/paintworkletglobalscope.rs
254:        let _ac = JSAutoCompartment::new(cx, self.worklet_global.reflector().get_jsobject().get());

components/script/dom/globalscope.rs
544:                let _ac = JSAutoCompartment::new(cx, globalhandle.get());

components/script/dom/websocket.rs
573:            let _ac = JSAutoCompartment::new(cx, ws.reflector().get_jsobject().get());

components/script/dom/workerglobalscope.rs
397:                        let _ac = JSAutoCompartment::new(

components/script/dom/promise.rs
144:        let _ac = JSAutoCompartment::new(cx, global.reflector().get_jsobject().get());
156:        let _ac = JSAutoCompartment::new(cx, global.reflector().get_jsobject().get());

components/script/dom/bindings/htmlconstructor.rs
118:        let _ac = JSAutoCompartment::new(window.get_cx(), callee.get());

components/script/dom/bindings/utils.rs
411:    let _ac = JSAutoCompartment::new(cx, obj.get());

components/script/dom/bindings/callback.rs
276:                let _ac = JSAutoCompartment::new(

components/script/dom/bindings/interface.rs
163:    let _ac = JSAutoCompartment::new(cx, rval.get());

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #23266

These changes do not require tests because no logic was changed only some code extracted to wrapper function

This change is

jdm · 2019-05-08T13:50:06Z

This is what I expected. Any call site that already has access to a *mut JSContext to pass doesn't make sense to replace with this helper.

bors-servo · 2019-05-11T05:51:08Z

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

KaczuH · 2019-05-18T10:13:07Z

Rebase done.

jdm · 2019-05-19T14:32:17Z

I will review this on Tuesday. Sorry for the delay!

jdm · 2019-05-23T14:04:53Z

components/script/dom/customelementregistry.rs

@@ -324,7 +324,7 @@ impl CustomElementRegistryMethods for CustomElementRegistry {
        // Steps 10.1 - 10.2
        rooted!(in(cx) let mut prototype = UndefinedValue());
        {
-            let _ac = JSAutoRealm::new(cx, constructor.get());
+            let _ac = enter_realm(&*self);


This is not an equivalent change, so we should revert it.

jdm · 2019-05-23T14:04:53Z

components/script/dom/eventtarget.rs

@@ -506,7 +507,7 @@ impl EventTarget {

        let scopechain = AutoObjectVectorWrapper::new(cx);

-        let _ac = JSAutoRealm::new(cx, window.reflector().get_jsobject().get());
+        let _ac = enter_realm(&*document);


Let's keep using window here instead of document.

jdm · 2019-05-23T14:04:53Z

components/script/dom/windowproxy.rs

@@ -148,7 +149,7 @@ impl WindowProxy {
                ((*get_object_class(window_jsobject.get())).flags & JSCLASS_IS_GLOBAL),
                0
            );
-            let _ac = JSAutoRealm::new(cx, window_jsobject.get());
+            let _ac = enter_realm(window);


Let's revert the changes in this file. We already have the cx and JSObject values, so calling this method to reacquire them is redundant.

bors-servo · 2019-06-02T21:36:57Z

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

jdm · 2019-06-24T14:54:54Z

@KaczuH Are you planning to finish this work?

KaczuH · 2019-06-29T16:43:38Z

All changes are now applied, sorry for long wait

jdm · 2019-06-29T17:03:51Z

Looks good! Could you squash the commits together into one?


        Create a helper API for entering a DOM object's compartment

Revert some unnecessary changes Fix fmt errors

KaczuH · 2019-06-29T17:24:06Z

@jdm squash done :)

jdm · 2019-06-29T17:55:41Z

@bors-servo r+
Thanks!

bors-servo · 2019-06-29T17:55:42Z

📌 Commit adb4024 has been approved by jdm

bors-servo · 2019-06-29T17:55:46Z

⌛ Testing commit adb4024 with merge 23c49bb...


        Auto merge of #23344 - KaczuH:enter_dom_compartment_wrapper, r=jdm

Enter dom compartment wrapper I'm still not sure if the changes are entirely correct. Replaced occurrences: `JSAutoCompartment::new(global.get_cx(), global.reflector().get_jsobject().get());` with `fn enter_compartment(object: &DomObject) -> JSAutoCompartment` There are still occurrences of `JSAutoCompartment` that i was unable to replace. Could anyone give me a hint if it is possible? ``` → rg -Fi --type rust "JSAutoCompartment::" components/script/compartments.rs 38: JSAutoCompartment::new( components/script/dom/create.rs 159: let _ac = JSAutoCompartment::new( components/script/dom/eventtarget.rs 527: let _ac = JSAutoCompartment::new(cx, self.reflector().get_jsobject().get()); components/script/dom/windowproxy.rs 223: let _ac = JSAutoCompartment::new(cx, window_jsobject.get()); components/script/dom/customelementregistry.rs 337: let _ac = JSAutoCompartment::new(cx, proto_object.get()); 349: let _ac = JSAutoCompartment::new(cx, constructor.get()); 538: let _ac = JSAutoCompartment::new(cx, self.constructor.callback()); 668: let _ac = JSAutoCompartment::new(cx, constructor.callback()); components/script/dom/window.rs 2216: let _ac = JSAutoCompartment::new(cx, obj.get()); components/script/dom/paintworkletglobalscope.rs 254: let _ac = JSAutoCompartment::new(cx, self.worklet_global.reflector().get_jsobject().get()); components/script/dom/globalscope.rs 544: let _ac = JSAutoCompartment::new(cx, globalhandle.get()); components/script/dom/websocket.rs 573: let _ac = JSAutoCompartment::new(cx, ws.reflector().get_jsobject().get()); components/script/dom/workerglobalscope.rs 397: let _ac = JSAutoCompartment::new( components/script/dom/promise.rs 144: let _ac = JSAutoCompartment::new(cx, global.reflector().get_jsobject().get()); 156: let _ac = JSAutoCompartment::new(cx, global.reflector().get_jsobject().get()); components/script/dom/bindings/htmlconstructor.rs 118: let _ac = JSAutoCompartment::new(window.get_cx(), callee.get()); components/script/dom/bindings/utils.rs 411: let _ac = JSAutoCompartment::new(cx, obj.get()); components/script/dom/bindings/callback.rs 276: let _ac = JSAutoCompartment::new( components/script/dom/bindings/interface.rs 163: let _ac = JSAutoCompartment::new(cx, rval.get()); ``` ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #23266  - [X] These changes do not require tests because no logic was changed only some code extracted to wrapper function    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23344)

bors-servo · 2019-06-29T20:29:38Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing 23c49bb to master...

jdm added the S-awaiting-review label May 8, 2019

jdm self-assigned this May 8, 2019

highfive added the S-needs-rebase label May 11, 2019

KaczuH force-pushed the KaczuH:enter_dom_compartment_wrapper branch from fde71b3 to 9fcb884 May 18, 2019

jdm requested changes May 23, 2019

View changes

jdm added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels May 23, 2019

highfive added the S-needs-rebase label Jun 2, 2019

KaczuH force-pushed the KaczuH:enter_dom_compartment_wrapper branch from 9fcb884 to 097a3a2 Jun 29, 2019

highfive added S-awaiting-review and removed S-needs-code-changes labels Jun 29, 2019

jdm removed the S-needs-rebase label Jun 29, 2019

jdm added S-needs-squash and removed S-awaiting-review labels Jun 29, 2019

Create a helper API for entering a DOM object's compartment

Loading status checks…

adb4024

Revert some unnecessary changes Fix fmt errors

KaczuH force-pushed the KaczuH:enter_dom_compartment_wrapper branch from c202455 to adb4024 Jun 29, 2019

highfive added the S-awaiting-review label Jun 29, 2019

highfive removed the S-awaiting-review label Jun 29, 2019

highfive added S-awaiting-merge and removed S-needs-squash labels Jun 29, 2019

bors-servo merged commit adb4024 into servo:master Jun 29, 2019
3 checks passed

3 checks passed

Travis CI - Pull Request Build Passed
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jun 29, 2019

bors-servo mentioned this pull request Jun 29, 2019

Continue message port #23637

Merged

0 of 5 tasks complete

servo / servo

Enter dom compartment wrapper #23344

Enter dom compartment wrapper #23344

KaczuH commented May 8, 2019 •

edited by SimonSapin

jdm commented May 8, 2019

bors-servo commented May 11, 2019

KaczuH commented May 18, 2019

jdm commented May 19, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Jun 2, 2019

jdm commented Jun 24, 2019

KaczuH commented Jun 29, 2019

jdm commented Jun 29, 2019

KaczuH commented Jun 29, 2019

jdm commented Jun 29, 2019

bors-servo commented Jun 29, 2019

bors-servo commented Jun 29, 2019

bors-servo commented Jun 29, 2019

servo / servo

Join GitHub today

Enter dom compartment wrapper #23344

Enter dom compartment wrapper #23344

Conversation

KaczuH commented May 8, 2019 • edited by SimonSapin

jdm commented May 8, 2019

bors-servo commented May 11, 2019

KaczuH commented May 18, 2019

jdm commented May 19, 2019

This comment has been minimized.

jdm May 23, 2019

This comment has been minimized.

jdm May 23, 2019

This comment has been minimized.

jdm May 23, 2019

bors-servo commented Jun 2, 2019

jdm commented Jun 24, 2019

KaczuH commented Jun 29, 2019

jdm commented Jun 29, 2019

KaczuH commented Jun 29, 2019

jdm commented Jun 29, 2019

bors-servo commented Jun 29, 2019

bors-servo commented Jun 29, 2019

bors-servo commented Jun 29, 2019

KaczuH commented May 8, 2019 •

edited by SimonSapin