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

Unwrap function before calling IsConstructor #17250

Merged
merged 1 commit into from Aug 16, 2017

Conversation

@cbrewster
Copy link
Member

commented Jun 9, 2017

IsConstructor returns true for all wrappers that are callable. Wrapped
arrow functions are callable and thus IsConstructor will return true
if given one; however, if we unwrap the arrow function, IsConstructor
will return false. The latter is the correct behavior here.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Jun 9, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/customelementregistry.rs
  • @KiChjang: components/script/dom/customelementregistry.rs
@highfive

This comment has been minimized.

Copy link

commented Jun 9, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@cbrewster

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2017

r? @jdm

@highfive highfive assigned jdm and unassigned glennw Jun 9, 2017

@@ -95,7 +96,15 @@ impl CustomElementRegistryMethods for CustomElementRegistry {
rooted!(in(global_scope.get_cx()) let constructor = constructor_.callback());

// Step 1
if unsafe { !IsConstructor(constructor.get()) } {
// We must unwrap the constructor as all wrappers are constructable if they are callable.
rooted!(in(global_scope.get_cx()) let unwrapped_constructor = unsafe { UnwrapObject(constructor.get(), 1) });

This comment has been minimized.

Copy link
@jdm

jdm Jun 12, 2017

Member

We should be storing this unwrapped constructor later, too.

This comment has been minimized.

Copy link
@cbrewster

cbrewster Jun 12, 2017

Author Member

Should I be storing the unwrapped constructor in CustomElementDefinition instead of Rc<Function>? Or do we need to store both?

This comment has been minimized.

Copy link
@jdm

jdm Jun 12, 2017

Member

That's an interesting question. I looked more carefully at Gecko, and they have two different hashmaps. One is keyed on the unwrapped constructor and stores local name values. The other table is keyed on local name values and stores the CustomElementDefinition, which contains the original constructor value. This means that all lookups by the constructor pointer are performed on unwrapped constructors, but the actual value that originated from script is used.

This comment has been minimized.

Copy link
@cbrewster

cbrewster Jun 13, 2017

Author Member

Ok, in that case I think this should be ok without storing the unwrapped constructor. If we did go the Gecko route of using two different HashMaps with different keys, that locks us into using Rc. If we do go that route, that could be saved for a followup.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2017

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

Unwrap function before calling IsConstructor
IsConstructor returns true for all wrappers that are callable. Wrapped
arrow functions are callable and thus IsConstructor will return true
if given one; however, if we unwrap the arrow function, IsConstructor
will return false. The latter is the correct behavior here.

@cbrewster cbrewster force-pushed the cbrewster:unwrap_constructor branch from f112b64 to d8510ba Aug 8, 2017

@jdm

This comment has been minimized.

Copy link
Member

commented Aug 16, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2017

📌 Commit d8510ba has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2017

⌛️ Testing commit d8510ba with merge cdd4de9...

bors-servo added a commit that referenced this pull request Aug 16, 2017
Auto merge of #17250 - cbrewster:unwrap_constructor, r=jdm
Unwrap function before calling IsConstructor

`IsConstructor` returns true for all wrappers that are callable. Wrapped
arrow functions are callable and thus `IsConstructor` will return true
if given one; however, if we unwrap the arrow function, `IsConstructor`
will return false. The latter is the correct behavior here.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17250)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2017

@bors-servo bors-servo merged commit d8510ba into servo:master Aug 16, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.