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

Allow passing arguments to setTimeout/setInterval callbacks #3941

Merged
merged 2 commits into from Nov 15, 2014

Conversation

@mukilan
Copy link
Contributor

mukilan commented Nov 8, 2014

No description provided.

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Nov 8, 2014

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

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.

@jdm
Copy link
Member

jdm commented Nov 8, 2014

Woah, I didn't expect to see this. Exciting!

@mukilan mukilan force-pushed the mukilan:timeout-arguments branch from 67bd526 to 76e9c0d Nov 9, 2014
@guillaumebort
Copy link
Contributor

guillaumebort commented Nov 14, 2014

Hey,

How does it relate to #3936? I was looking at this issue but I'm not sure if the codegen changes of this pull request magically fix it already? Otherwise how could the argument of SetTimeout be either a DOMString of a Function?

@mukilan
Copy link
Contributor Author

mukilan commented Nov 14, 2014

@guillaumebort : This PR does not implement the SetTimeout(DOMString,...) overload. If you look at the changes in Window.webidl file, you'll see that setTimeout(DOMString, long, any) is commented out. To fix #3936 , we need to uncomment this line. The binding generator will notice that setTimeout has 2 overloads and generate the JS -> Rust binding function (extern fn setTimeout in WindowBinding.rs) such that if the first parameter (i.e argv[0]) is a callable JSObject or function, then the WindowMethods::SetTimeout(Function,...) is invoked or if it is a string, then WindowMethods::SetTimeout(DOMString,...) is invoked, and in all other cases a TypeError is thrown.

@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 14, 2014

(Actually, whenever it's not a callable object, the value is converted to a string.)

@mukilan
Copy link
Contributor Author

mukilan commented Nov 14, 2014

@Ms2ger : My bad. I think it is about time I read the overload resolution algorithm ;-)

@guillaumebort
Copy link
Contributor

guillaumebort commented Nov 14, 2014

Ok great, thanks for the info! I will see if I can apply my pending change on top of your work then.

@mukilan mukilan force-pushed the mukilan:timeout-arguments branch from 149e8c7 to 4b2b0d0 Nov 15, 2014
@Ms2ger

This comment has been minimized.

Copy link

Ms2ger commented on 4b2b0d0 Nov 15, 2014

r+

This comment has been minimized.

Copy link

Ms2ger replied Nov 15, 2014

@bors: retry

This comment has been minimized.

Copy link

Ms2ger replied Nov 15, 2014

@bors: retry

This comment has been minimized.

Copy link

jdm replied Nov 15, 2014

@bors: retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented on 4b2b0d0 Nov 15, 2014

saw approval from Ms2ger
at mukilan@4b2b0d0

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 15, 2014

merging mukilan/servo/timeout-arguments = 4b2b0d0 into auto

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 15, 2014

mukilan/servo/timeout-arguments = 4b2b0d0 merged ok, testing candidate = 61705f9

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 15, 2014

saw approval from Ms2ger
at mukilan@4b2b0d0

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 15, 2014

merging mukilan/servo/timeout-arguments = 4b2b0d0 into auto

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 15, 2014

saw approval from Ms2ger
at mukilan@4b2b0d0

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 15, 2014

merging mukilan/servo/timeout-arguments = 4b2b0d0 into auto

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 15, 2014

mukilan/servo/timeout-arguments = 4b2b0d0 merged ok, testing candidate = 43b452f

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 15, 2014

fast-forwarding master to auto = 43b452f

bors-servo pushed a commit that referenced this pull request Nov 15, 2014
bors-servo pushed a commit that referenced this pull request Nov 15, 2014
bors-servo pushed a commit that referenced this pull request Nov 15, 2014
bors-servo pushed a commit that referenced this pull request Nov 15, 2014
@bors-servo bors-servo closed this Nov 15, 2014
@bors-servo bors-servo merged commit 4b2b0d0 into servo:master Nov 15, 2014
1 check passed
1 check passed
default all tests passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.