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

Fix #6242 testbinding add specialoperations #6284

Merged
merged 1 commit into from Jun 9, 2015
Merged

Conversation

@ghost
Copy link

ghost commented Jun 4, 2015

Review on Reviewable

@highfive
Copy link

highfive commented Jun 4, 2015

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @larsbergstrom (or someone else) soon.

@nox
Copy link
Member

nox commented Jun 5, 2015

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


Review status: all files reviewed, 3 unresolved discussions, all commit checks successful.
Reviewed files:

  • components/script/dom/mod.rs @ r2
  • components/script/dom/testbindingproxy.rs @ r2
  • components/script/dom/webidls/TestBindingProxy.webidl @ r1

components/script/dom/testbindingproxy.rs, line 7 [r2] (raw file):
Nit: remove braces.


components/script/dom/testbindingproxy.rs, line 14 [r2] (raw file):
Nit: the global field is useless.


components/script/dom/webidls/TestBindingProxy.webidl, line 6 [r2] (raw file):
Nit: this is not true, please rephrase.


Comments from the review on Reviewable.io

@nox nox self-assigned this Jun 5, 2015
@nox
Copy link
Member

nox commented Jun 5, 2015

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


Review status: all files reviewed, 4 unresolved discussions, all commit checks successful.
Reviewed files:

  • components/script/dom/testbindingproxy.rs @ r3
  • components/script/dom/webidls/TestBindingProxy.webidl @ r3

components/script/dom/testbindingproxy.rs, line 12 [r3] (raw file):
I just noticed that this is called reflector_ in all other structures and I have no idea why. Will r+ or tell you to change it once someone tells me if it's important or not. @jdm? @Manisheart?


Comments from the review on Reviewable.io

@nox
Copy link
Member

nox commented Jun 5, 2015

-S-awaiting-answer +S-needs-code-changes


Review status: all files reviewed, 4 unresolved discussions, all commit checks successful.


components/script/dom/testbindingproxy.rs, line 12 [r3] (raw file):
@Manishearth just told me it's to avoid clashing with the reflector() method, please rename to reflector_.


Comments from the review on Reviewable.io

@nox
Copy link
Member

nox commented Jun 8, 2015

Squash everything and I'll r+ it. :)

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


Review status: all files reviewed, 4 unresolved discussions, all commit checks successful.
Reviewed files:

  • components/script/dom/testbinding.rs @ r4
  • components/script/dom/testbindingproxy.rs @ r4

Comments from the review on Reviewable.io

@ghost
Copy link
Author

ghost commented Jun 8, 2015

Done!

@nox
Copy link
Member

nox commented Jun 8, 2015

@bors-servo: r+

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


Review status: all files reviewed, 4 unresolved discussions, all commit checks successful.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Jun 8, 2015

📌 Commit 3c60917 has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Jun 8, 2015

Testing commit 3c60917 with merge b968cdf...

bors-servo pushed a commit that referenced this pull request Jun 8, 2015
… r=nox

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

bors-servo commented Jun 8, 2015

💔 Test failed - linux2

@nox
Copy link
Member

nox commented Jun 9, 2015

/XMLHttpRequest/timeout-cors-async.htm
--------------------------------------
PASS expected FAIL XMLHttpRequest: timeout event and cross-origin request
/html/webappapis/scripting/processing-model-2/compile-error-cross-origin.html
-----------------------------------------------------------------------------
NOTRUN expected FAIL window.onerror - compile error in 
NOTRUN expected FAIL window.onerror - compile error in <script src=//www1...> (column)
TIMEOUT [Parent]
/_mozilla/mozilla/interfaces.html
---------------------------------
FAIL Interfaces exposed on the window

You need to patch the second list in /_mozilla/mozilla/interfaces.html and add TestBindingProxy in it.

@nox
Copy link
Member

nox commented Jun 9, 2015

The two other failing tests are probably intermittent.

added testbindingproxy to dom/mod.rs and fixed unused variable warning of testingbindingproxy.rs

removed useless GlobalField, removed brackets use statements with only 1 element and changed the description of TestBindingProxy.webidl

renamed reflector to reflector_ and removed unused import in testbinding.rs
@nox
Copy link
Member

nox commented Jun 9, 2015

@bors-servo: r+

-S-awaiting-review +S-awaiting-merge


Review status: all files reviewed, 4 unresolved discussions, all commit checks successful.
Reviewed files:

  • tests/wpt/mozilla/tests/mozilla/interfaces.html @ r6

Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Jun 9, 2015

📌 Commit 4ada1e9 has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Jun 9, 2015

Testing commit 4ada1e9 with merge 44a4b78...

bors-servo pushed a commit that referenced this pull request Jun 9, 2015
… r=nox

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

bors-servo commented Jun 9, 2015

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

@bors-servo bors-servo merged commit 4ada1e9 into servo:master Jun 9, 2015
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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