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

Implement WebIDL attribute PutForwards #5894

Merged
merged 1 commit into from May 7, 2015
Merged

Conversation

nox
Copy link
Contributor

@nox nox commented Apr 29, 2015

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 29, 2015
@hoppipolla-critic-bot
Copy link

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

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.

@nox
Copy link
Contributor Author

nox commented Apr 29, 2015

This does not properly put in scope the forwarded attribute Methods trait, looking into it…

@nox
Copy link
Contributor Author

nox commented Apr 29, 2015

Problem solved.

@jdm
Copy link
Member

jdm commented Apr 30, 2015

Reviewed files:

  • components/script/dom/bindings/codegen/CodegenRust.py @ r3
  • components/script/dom/bindings/codegen/parser/WebIDL.py @ r1

components/script/dom/bindings/codegen/CodegenRust.py, line 2810 [r3] (raw file):
Let's either make use of this or remove it.


components/script/dom/bindings/codegen/CodegenRust.py, line 4285 [r3] (raw file):
Why can't we use attribute_arguments here?


components/script/dom/bindings/codegen/parser/WebIDL.py, line 1002 [r3] (raw file):
Are these changes imported from Gecko? We have a pretty good track record of keeping WebIDL.py up to date with upstream, and we should make use of their work here.



Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Apr 30, 2015

Reviewed files:

  • components/script/dom/testbinding.rs @ r1
  • components/script/dom/webidls/TestBinding.webidl @ r1

Comments from the review on Reviewable.io

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 30, 2015
@nox
Copy link
Contributor Author

nox commented Apr 30, 2015

components/script/dom/bindings/codegen/CodegenRust.py, line 2810 [r3] (raw file):
I forgot to throw it away.


components/script/dom/bindings/codegen/CodegenRust.py, line 4285 [r3] (raw file):
Didn't try, just went directly to the simplest code.


components/script/dom/bindings/codegen/parser/WebIDL.py, line 1002 [r3] (raw file):
I didn't know there was an upstream. Where is it? That should be written in the file.



Comments from the review on Reviewable.io

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Apr 30, 2015
@nox
Copy link
Contributor Author

nox commented Apr 30, 2015

@jdm

The code you linked me to is not ideal IMO, it just uses the JSAPI directly and won't let me get fancy once I implement Reflect (#5881). I want to be able to generate a reflected forwarded setter that will just set the attribute directly through Element::AttributeHandlers and whatnot, without even needing to use the getter.

Note that my changes to WebIDL.py could very well be upstream'd. They are nothing more than new checks and a small refactoring.

@jdm
Copy link
Member

jdm commented Apr 30, 2015

@Ms2ger Does the previous comment sound web-compatible?

@nox
Copy link
Contributor Author

nox commented Apr 30, 2015

I reached this conclusion from:

More generally, I don't see how it would make sense for a reflecting IDL attribute to behave differently from the underlying reflected content attribute.

@Ms2ger
Copy link
Contributor

Ms2ger commented May 1, 2015

I still believe the spec is crazy, but we should still follow it

@nox
Copy link
Contributor Author

nox commented May 1, 2015

@Ms2ger What do you mean by follow it? In which way do I not follow it?

@nox
Copy link
Contributor Author

nox commented May 1, 2015

@Ms2ger The only moment my assumptions aren't true is if the attribute is also [LenientThis], but in this case nothing at all is called anyway because the setter should stop at steps 5 or 7:

  • If validThis is false and the attribute was not specified with the [LenientThis] extended attribute, then throw a TypeError.
  • If validThis is false, then return.

Please show me one example where my assumptions break things.

@Ms2ger
Copy link
Contributor

Ms2ger commented May 1, 2015

Think outside the box :)

<pre>
<script>
function w(s) {
  document.body.firstChild.appendChild(new Text(s + "\n"))
}

var l = document.createElement("link");
w(l.getAttribute("sizes")) // null

l.sizes = "bar";
w(l.getAttribute("sizes")) // bar

Object.defineProperty(l.sizes, "value", {
  get: function() { return "foo" },
  set: function() { w("hi!") }
})
l.sizes = "baz"; // hi!
w(l.getAttribute("sizes")) // bar
</script>
</pre>

@nox
Copy link
Contributor Author

nox commented May 1, 2015

I stand corrected, and you stand insane. :P Will dumb down my code.

@nox
Copy link
Contributor Author

nox commented May 2, 2015

@Ms2ger What if I use JS_GetPropertyDescriptor() and shortcut everything if they have been left untouched?

@Ms2ger
Copy link
Contributor

Ms2ger commented May 2, 2015

Ew.

@jdm
Copy link
Member

jdm commented May 7, 2015

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


Reviewed files:

  • components/script/dom/bindings/codegen/CodegenRust.py @ r4
  • components/script/dom/bindings/codegen/parser/WebIDL.py @ r4

components/script/dom/bindings/codegen/CodegenRust.py, line 2855 [r4] (raw file):
nit: no need for the surrounding ()


Comments from the review on Reviewable.io

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 7, 2015
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels May 7, 2015
@jdm jdm removed the S-awaiting-review There is new code that needs to be reviewed. label May 7, 2015
@jdm jdm added the S-needs-squash Some (or all) of the commits in the PR should be combined. label May 7, 2015
@jdm
Copy link
Member

jdm commented May 7, 2015

-S-awaiting-review +S-needs-squash


Reviewed files:

  • components/script/dom/bindings/codegen/CodegenRust.py @ r5

Comments from the review on Reviewable.io

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 7, 2015
@jdm
Copy link
Member

jdm commented May 7, 2015

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit cc5eee4 has been approved by jdm

@bors-servo
Copy link
Contributor

⌛ Testing commit cc5eee4 with merge 29a43a0...

bors-servo pushed a commit that referenced this pull request May 7, 2015
<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5894)
<!-- Reviewable:end -->
@jdm jdm added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-squash Some (or all) of the commits in the PR should be combined. labels May 7, 2015
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2

@bors-servo bors-servo merged commit cc5eee4 into servo:master May 7, 2015
@nox nox deleted the putforwards branch August 20, 2015 10:47
bors-servo pushed a commit that referenced this pull request Jan 16, 2016
Uncomment some [PutForwards] attributes

Now that PutForwards was implemented in #5894, we can uncomment the attributes where the forwarded property exists on the target interface. Do we have tests for this?

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8005)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this pull request Jan 16, 2016
Uncomment some [PutForwards] attributes

Now that PutForwards was implemented in #5894, we can uncomment the attributes where the forwarded property exists on the target interface. Do we have tests for this?

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8005)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this pull request Jan 16, 2016
Uncomment some [PutForwards] attributes

Now that PutForwards was implemented in #5894, we can uncomment the attributes where the forwarded property exists on the target interface. Do we have tests for this?

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8005)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants