Extract some of CGClassConstructHook to utils.rs #25432

warren-fisher · 2020-01-05T02:43:33Z

Moving some of the functionality from the massive tripled quoted string in CGClassConstructHook in components/script/dom/bindings/codegen/CodegenRust.py to components/script/dom/bindings/utils.rs. Must be made unsafe because of UnwrapObjectDynamic and other functions. Added imports as necessary as well, as well as cleaning up using test-tidy.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #25395 (GitHub issue number if applicable)

There are tests for these changes OR
These changes do not require tests because the issue says so

highfive · 2020-01-05T02:43:42Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @paulrouget (or someone else) soon.

highfive · 2020-01-05T02:43:44Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/utils.rs
@KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/utils.rs

highfive · 2020-01-05T02:43:46Z

Warning

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

jdm

This is definitely an improvement!

components/script/dom/bindings/utils.rs

bors-servo · 2020-01-12T07:38:41Z

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

warren-fisher · 2020-01-14T21:20:06Z

components/script/dom/bindings/htmlconstructor.rs

+    }
+    // Wrap prototype in this context since it is from the newTarget compartment
+    if !JS_WrapObject(*cx, RawMutableHandle::from(prototype.handle_mut())) {
+        return Err(Error::Type("false".to_owned()));


Error::JSFailed.

components/script/dom/bindings/htmlconstructor.rs

warren-fisher · 2020-01-14T21:27:38Z

components/script/dom/bindings/htmlconstructor.rs

+            b"prototype\\0".as_ptr() as *const _,
+            proto_val.handle_mut(),
+        ) {
+            return Err(Error::Type("false".to_owned()));


I am not sure what type of error is appropriate to return here. Previously it was a return false when it was within the monolithic function. For the sake of getting something "working" I made this placeholder error. Perhaps it is not suitable to throw an error here, and in that case I am not sure the best way of handling this.

This should return Error::JSFailed, instead.

jdm

Almost there!

jdm · 2020-01-14T21:39:15Z

components/script/dom/bindings/htmlconstructor.rs

+            b"prototype\\0".as_ptr() as *const _,
+            proto_val.handle_mut(),
+        ) {
+            return Err(Error::Type("false".to_owned()));


This should return Error::JSFailed, instead.

jdm · 2020-01-14T21:39:15Z

components/script/dom/bindings/htmlconstructor.rs

+    }
+    // Wrap prototype in this context since it is from the newTarget compartment
+    if !JS_WrapObject(*cx, RawMutableHandle::from(prototype.handle_mut())) {
+        return Err(Error::Type("false".to_owned()));


Error::JSFailed.

components/script/dom/bindings/htmlconstructor.rs

jdm · 2020-01-14T21:42:29Z

In principle I understand that monomorphizing anything that is not needed adds complexity and slows down compile times. Would you say these are the main reasons for avoiding monomorphizing code unnecessarily?

It's both compile time and the size of the generated code - monomorphization can easily end up duplicating large blocks of code that do not depend on any generic types, and relying on the compiler to notice this and de-duplicate them doesn't always work.

jdm · 2020-01-14T23:19:51Z

@bors-servo try=wpt

bors-servo · 2020-01-14T23:19:57Z

⌛ Trying commit 02dbb50 with merge ee2a80a...


        Auto merge of #25432 - warren-fisher:HTMLConstructor, r=<try>

Extract some of CGClassConstructHook to utils.rs  Moving some of the functionality from the massive tripled quoted string in CGClassConstructHook in `components/script/dom/bindings/codegen/CodegenRust.py` to `components/script/dom/bindings/utils.rs`. Must be made unsafe because of UnwrapObjectDynamic and other functions. Added imports as necessary as well, as well as cleaning up using test-tidy. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #25395 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [x] These changes do not require tests because the issue says so

warren-fisher · 2020-01-14T23:23:55Z

I fixed up the commit history a bit. Not sure if that will cause bors-servo to mess up at all.

bors-servo · 2020-06-03T17:48:29Z

📌 Commit edfc47e has been approved by jdm

bors-servo · 2020-06-03T18:44:02Z

⌛ Testing commit edfc47e with merge 2b62650...


        Auto merge of #25432 - warren-fisher:HTMLConstructor, r=jdm

Extract some of CGClassConstructHook to utils.rs  Moving some of the functionality from the massive tripled quoted string in CGClassConstructHook in `components/script/dom/bindings/codegen/CodegenRust.py` to `components/script/dom/bindings/utils.rs`. Must be made unsafe because of UnwrapObjectDynamic and other functions. Added imports as necessary as well, as well as cleaning up using test-tidy. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #25395 (GitHub issue number if applicable)  - [x] There are tests for these changes OR - [ ] These changes do not require tests because the issue says so

bors-servo · 2020-06-03T19:02:49Z

💔 Test failed - status-taskcluster


        Reduce code duplication. Move some of CodegenRust.py to htmlconstruct…

…or.rs

jdm · 2020-06-03T19:13:51Z

@bors-servo r+

bors-servo · 2020-06-03T19:13:52Z

📌 Commit 2da07ed has been approved by jdm

bors-servo · 2020-06-03T19:59:44Z

⌛ Testing commit 2da07ed with merge eb21264...


        Auto merge of #25432 - warren-fisher:HTMLConstructor, r=jdm

Extract some of CGClassConstructHook to utils.rs  Moving some of the functionality from the massive tripled quoted string in CGClassConstructHook in `components/script/dom/bindings/codegen/CodegenRust.py` to `components/script/dom/bindings/utils.rs`. Must be made unsafe because of UnwrapObjectDynamic and other functions. Added imports as necessary as well, as well as cleaning up using test-tidy. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #25395 (GitHub issue number if applicable)  - [x] There are tests for these changes OR - [ ] These changes do not require tests because the issue says so

bors-servo · 2020-06-03T20:37:52Z

💔 Test failed - status-taskcluster

jdm · 2020-06-03T20:40:34Z

@bors-servo retry

bors-servo · 2020-06-03T20:59:36Z

⌛ Testing commit 2da07ed with merge 7758d4f...

bors-servo · 2020-06-03T22:05:36Z

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 7758d4f to master...

highfive assigned paulrouget Jan 5, 2020

highfive added the S-awaiting-review label Jan 5, 2020

jdm assigned jdm and unassigned paulrouget Jan 5, 2020

jdm requested changes Jan 9, 2020

View changes

components/script/dom/bindings/utils.rs Outdated Show resolved Hide resolved

components/script/dom/bindings/utils.rs Outdated Show resolved Hide resolved

components/script/dom/bindings/utils.rs Outdated Show resolved Hide resolved

jdm added S-needs-code-changes and removed S-awaiting-review labels Jan 9, 2020

highfive added the S-needs-rebase label Jan 12, 2020

warren-fisher force-pushed the warren-fisher:HTMLConstructor branch from 515c950 to 68c476a Jan 14, 2020

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 14, 2020

warren-fisher reviewed Jan 14, 2020

View changes

jdm requested changes Jan 14, 2020

View changes

jdm added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Jan 14, 2020

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 14, 2020

warren-fisher force-pushed the warren-fisher:HTMLConstructor branch from 02dbb50 to f2f2738 Jan 14, 2020

warren-fisher closed this Jan 14, 2020

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Jun 3, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Jun 3, 2020

Reduce code duplication. Move some of CodegenRust.py to htmlconstruct…

Loading status checks…

2da07ed

…or.rs

jdm force-pushed the warren-fisher:HTMLConstructor branch from edfc47e to 2da07ed Jun 3, 2020

highfive added S-awaiting-review S-awaiting-merge and removed S-tests-failed S-awaiting-review labels Jun 3, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Jun 3, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Jun 3, 2020

bors-servo merged commit 7758d4f into servo:master Jun 3, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jun 3, 2020

servo / servo

Extract some of CGClassConstructHook to utils.rs #25432

Extract some of CGClassConstructHook to utils.rs #25432

warren-fisher commented Jan 5, 2020 •

edited by jdm

highfive commented Jan 5, 2020

highfive commented Jan 5, 2020

highfive commented Jan 5, 2020

jdm left a comment

bors-servo commented Jan 12, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm left a comment

This comment has been minimized.

This comment has been minimized.

jdm commented Jan 14, 2020

jdm commented Jan 14, 2020

bors-servo commented Jan 14, 2020

warren-fisher commented Jan 14, 2020

bors-servo commented Jun 3, 2020

bors-servo commented Jun 3, 2020

bors-servo commented Jun 3, 2020

jdm commented Jun 3, 2020

bors-servo commented Jun 3, 2020

bors-servo commented Jun 3, 2020

bors-servo commented Jun 3, 2020

jdm commented Jun 3, 2020

bors-servo commented Jun 3, 2020

bors-servo commented Jun 3, 2020

servo / servo

Join GitHub today

Extract some of CGClassConstructHook to utils.rs #25432

Extract some of CGClassConstructHook to utils.rs #25432

Conversation

warren-fisher commented Jan 5, 2020 • edited by jdm

highfive commented Jan 5, 2020

highfive commented Jan 5, 2020

highfive commented Jan 5, 2020

jdm left a comment

bors-servo commented Jan 12, 2020

This comment has been minimized.

warren-fisher Jan 14, 2020

This comment has been minimized.

jdm Jan 14, 2020

This comment has been minimized.

warren-fisher Jan 14, 2020

This comment has been minimized.

jdm Jan 14, 2020

jdm left a comment

This comment has been minimized.

jdm Jan 14, 2020

This comment has been minimized.

jdm Jan 14, 2020

jdm commented Jan 14, 2020

jdm commented Jan 14, 2020

bors-servo commented Jan 14, 2020

warren-fisher commented Jan 14, 2020

bors-servo commented Jun 3, 2020

bors-servo commented Jun 3, 2020

bors-servo commented Jun 3, 2020

jdm commented Jun 3, 2020

bors-servo commented Jun 3, 2020

bors-servo commented Jun 3, 2020

bors-servo commented Jun 3, 2020

jdm commented Jun 3, 2020

bors-servo commented Jun 3, 2020

bors-servo commented Jun 3, 2020

warren-fisher commented Jan 5, 2020 •

edited by jdm