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

webidl: Implement sequences in unions #9304

Merged
merged 5 commits into from Jan 17, 2016
Merged

webidl: Implement sequences in unions #9304

merged 5 commits into from Jan 17, 2016
+66 −22

Conversation

@emilio
Copy link
Member

emilio commented Jan 14, 2016

Unblocks #9053

Review on Reviewable
@highfive
Copy link

highfive commented Jan 14, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@emilio emilio mentioned this pull request Jan 14, 2016
Closed
@emilio
Copy link
Member Author

emilio commented Jan 14, 2016

r? @jdm or @nox
@emilio emilio force-pushed the emilio:sequence-in-unions branch from 7c6497e to 16a23bc Jan 14, 2016
components/script/dom/bindings/codegen/CodegenRust.py Outdated
conversions.append(CGIfWrapper("value.get().is_object()", templateBody))
elif arrayObject:
templateBody = CGList([arrayObject], "\n")
conversions.append(CGIfWrapper("value.get().is_object()", templateBody))

This comment has been minimized.

Sign in to view
@frewsxcv

frewsxcv Jan 14, 2016

Member

This line can be removed from both branches of the conditional since it's the same

This comment has been minimized.

Sign in to view
@frewsxcv

frewsxcv Jan 14, 2016

Member

Unless you did that thinking other object types might get added to the conditional

This comment has been minimized.

Sign in to view
@emilio

emilio Jan 14, 2016

Author Member

Yeah eventually other types might be added.

It seems to me that that line would be necessary in all of them though (we're inside of hasObjectTypes), so I'll take it out of the conditional.
@emilio emilio mentioned this pull request Jan 14, 2016
Closed
@emilio emilio force-pushed the emilio:sequence-in-unions branch from 16a23bc to 5d3b228 Jan 14, 2016
@jdm
Copy link
Member

jdm commented Jan 14, 2016

r? @nox is probably a better bet at this point
@highfive highfive assigned nox and unassigned jdm Jan 14, 2016
@emilio emilio force-pushed the emilio:sequence-in-unions branch from 5d3b228 to cf997b2 Jan 14, 2016
@nox
Copy link
Member

nox commented Jan 15, 2016

What happens if the type is DOMString or sequence<Blob>?

-S-awaiting-review +S-awaiting-answer

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io
@emilio
webidl: Implement sequences in unions
4ad1a8d 
Unblocks #9053
@emilio emilio force-pushed the emilio:sequence-in-unions branch from cf997b2 to 87f1e2b Jan 15, 2016
@emilio
script: Implement FromJSValConvertible for Root<T>
3327f00
@emilio emilio force-pushed the emilio:sequence-in-unions branch from 87f1e2b to e2e8313 Jan 15, 2016
@emilio
Copy link
Member Author

emilio commented Jan 15, 2016

Good catch! (it only generated code for the former).

I fixed it, and also implemented FromJSValConvertible for Root. It's needed to allow conversion from a HandleValue to a Vec<Root<T>>.

I think the implementation is correct, but since you probably know better than I if root_from_handlevalue suits correctly there.
emilio added 2 commits Jan 15, 2016
@emilio
webidl: Add test for interface sequences
a76efa5 
Sequence interfaces return values worked before, but had no test.

Sequence interface arguments didn't work until the previous commit.
@emilio
webidl: Add test for interface and sequence in the same union
8859c61
@emilio emilio force-pushed the emilio:sequence-in-unions branch from e2e8313 to 601ff74 Jan 15, 2016
@emilio
webidl: Refactor isSequence in getJSToNativeConversionInfo
Loading status checks…
9508219 
That way it does not depend on the return value for the same type.

This hopefully makes the code more clear, and avoids errors if something
changes in the future (for example, we could want to pass slices as
sequence arguments).
@emilio emilio force-pushed the emilio:sequence-in-unions branch from 601ff74 to 9508219 Jan 15, 2016
@nox
Copy link
Member

nox commented Jan 15, 2016

@Ms2ger Do we care about the Vec<Root<T>> here? Union types are weird anyway so I'm tempted to say no.
@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 15, 2016

Not sure.
@jdm
Copy link
Member

jdm commented Jan 15, 2016

@nox Care about in what sense? We established that Vec<Root<T>> isn't a memory-safety issue, at least.
@nox
Copy link
Member

nox commented Jan 16, 2016

@bors-servo r+

@jdm Sure, but that is at odds with sequence<Blob> which just ends up as &[&Blob] in the Rust code.
@bors-servo
Copy link
Contributor

bors-servo commented Jan 16, 2016

📌 Commit 9508219 has been approved by nox
@frewsxcv
Copy link
Member

frewsxcv commented Jan 16, 2016

Related to Vec<Root<T>>: #8737
@bors-servo
Copy link
Contributor

bors-servo commented Jan 17, 2016

⌛ Testing commit 9508219 with merge d059cf9...
bors-servo added a commit that referenced this pull request Jan 17, 2016
@bors-servo
Auto merge of #9304 - ecoal95:sequence-in-unions, r=nox
d059cf9 
webidl: Implement sequences in unions

Unblocks #9053

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

bors-servo commented Jan 17, 2016

⛄ The build was interrupted to prioritize another pull request.
@Manishearth
Copy link
Member

Manishearth commented Jan 17, 2016

@bors-servo p=2 force
@bors-servo
Copy link
Contributor

bors-servo commented Jan 17, 2016

⌛ Testing commit 9508219 with merge 2f0daa1...
bors-servo added a commit that referenced this pull request Jan 17, 2016
@bors-servo
Auto merge of #9304 - ecoal95:sequence-in-unions, r=nox
Loading status checks…
2f0daa1 
webidl: Implement sequences in unions

Unblocks #9053

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

bors-servo commented Jan 17, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 9508219 into servo:master Jan 17, 2016
2 checks passed
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
Reviewers
No reviews
Assignees

@nox nox

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@emilio @highfive @jdm @nox @Ms2ger @bors-servo @frewsxcv @Manishearth
You can’t perform that action at this time.