Skip to content

/ servo

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

Fix defineProperty codegen #5845

Merged
merged 1 commit into from Apr 28, 2015
Merged

Fix defineProperty codegen #5845

merged 1 commit into from Apr 28, 2015

Conversation

@snf
Copy link
Contributor

snf commented Apr 26, 2015

This patch should get rid of #5223.

Review on Reviewable
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Apr 26, 2015

Critic review: https://critic.hoppipolla.co.uk/r/4799

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.
@snf
Copy link
Contributor Author

snf commented Apr 26, 2015

Don't review it yet. I found a bug when the id is a number. We should convert any id object to a string (but few exceptions) inside defineProperty.
@snf
Copy link
Contributor Author

snf commented Apr 27, 2015

Ok, I will fix that in another PR.
@jdm
Copy link
Member

jdm commented Apr 27, 2015

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

Reviewed files:

  • components/script/dom/bindings/codegen/CodegenRust.py @ r1
  • tests/wpt/metadata/webstorage/builtins.html.ini @ r2
  • tests/wpt/metadata/webstorage/in.html.ini @ r2
  • tests/wpt/metadata/webstorage/setitem.html.ini @ r2

components/script/dom/bindings/codegen/CodegenRust.py, line 3935 [r2] (raw file):
Let's file an issue to fix this after the SpiderMonkey upgrade, and leave a TODO here pointing to it.

Comments from the review on Reviewable.io
@snf
Copy link
Contributor Author

snf commented Apr 27, 2015

squash?
@jdm
Copy link
Member

jdm commented Apr 27, 2015

Yep!
@snf
fix defineproperty for cases with namedsetter
Loading status checks…
cdcd667
@snf snf force-pushed the snf:defineProperty_fix branch from 34fa7a7 to cdcd667 Apr 27, 2015
@snf
Copy link
Contributor Author

snf commented Apr 27, 2015

Done!
@jdm
Copy link
Member

jdm commented Apr 27, 2015

@bors-servo: r+
@bors-servo
Copy link
Contributor

bors-servo commented Apr 27, 2015

📌 Commit cdcd667 has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2015

⌛ Testing commit cdcd667 with merge f5cb757...
@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2015

💔 Test failed - gonk
@mbrubeck
Copy link
Contributor

mbrubeck commented Apr 28, 2015

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2015

⌛ Testing commit cdcd667 with merge db97cc6...
bors-servo pushed a commit that referenced this pull request Apr 28, 2015
bors-servo
Auto merge of #5845 - snf:defineProperty_fix, r=jdm
db97cc6 
This patch should get rid of #5223.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5845)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2015

💔 Test failed - linux1
@jdm
Copy link
Member

jdm commented Apr 28, 2015

@bors-servo: retry
bors-servo pushed a commit that referenced this pull request Apr 28, 2015
bors-servo
Auto merge of #5845 - snf:defineProperty_fix, r=jdm
Loading status checks…
01925f0 
This patch should get rid of #5223.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5845)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2015

⌛ Testing commit cdcd667 with merge 01925f0...
@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2015

💔 Test failed - linux2
@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 28, 2015

@bors-servo: retry
@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2015

⚡ Previous build results for android, gonk, linux1, mac1, mac2 are reusable. Rebuilding only linux2...
@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2015

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2
@bors-servo bors-servo merged commit cdcd667 into servo:master Apr 28, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@snf snf deleted the snf:defineProperty_fix branch Apr 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@snf @hoppipolla-critic-bot @jdm @bors-servo @mbrubeck @Ms2ger @highfive
You can’t perform that action at this time.