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

Implement Element.getAttributeNames #9064

Merged
merged 2 commits into from Feb 26, 2016
Merged

Implement Element.getAttributeNames #9064

merged 2 commits into from Feb 26, 2016

Conversation

@nox
Copy link
Member

nox commented Dec 24, 2015

Review on Reviewable
components/script/dom/element.rs Outdated
@@ -1201,6 +1201,11 @@ impl ElementMethods for Element {
self.attr_list.or_init(|| NamedNodeMap::new(&window_from_node(self), self))
}

// https://dom.spec.whatwg.org/#interface-element
fn GetAttributeNames(&self) -> Vec<DOMString> {

This comment has been minimized.

Sign in to view
Copy link
@Manishearth

Manishearth Dec 24, 2015

Member

https://dom.spec.whatwg.org/#dom-element-getattributenames
@Manishearth
Copy link
Member

Manishearth commented Dec 24, 2015

r=me with the updated spec link
@nox nox force-pushed the nox:them-attr-names branch from 8268e70 to d1db0a4 Dec 24, 2015
@nox
Copy link
Member Author

nox commented Dec 24, 2015

@bors-servo r=Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Dec 24, 2015

📌 Commit d1db0a4 has been approved by Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Dec 24, 2015

⌛ Testing commit d1db0a4 with merge b890d23...
bors-servo added a commit that referenced this pull request Dec 24, 2015
@bors-servo
Auto merge of #9064 - nox:them-attr-names, r=Manishearth
b890d23 
Implement Element.getAttributeNames

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

bors-servo commented Dec 24, 2015

💔 Test failed - mac-rel-wpt
@eefriedman
Copy link
Contributor

eefriedman commented Dec 24, 2015 

  │ FAIL [expected PASS] Element interface: element must inherit property "setAttributeNode" with the proper type (20)
  │   → assert_inherits: property "setAttributeNode" not found in prototype chain

etc. Looks like the expected results just need to be updated.
@nox
Copy link
Member Author

nox commented Dec 24, 2015

@eefriedman Yes. I'm looking into idlharness.js to not include the index in the test name (there is no need for it, overloaded operations share only one property).
@nox
Copy link
Member Author

nox commented Dec 25, 2015

Blocked by w3c/testharness.js#170.
@nox nox removed the S-tests-failed label Jan 8, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Jan 28, 2016

☔ The latest upstream changes (presumably #9061) made this pull request unmergeable. Please resolve the merge conflicts.
@nox
Remove a fat arrow from /dom/nodes/attributes.html
329a5b0
@nox nox force-pushed the nox:them-attr-names branch from d1db0a4 to 915c6d8 Feb 26, 2016
@nox
Copy link
Member Author

nox commented Feb 26, 2016

I changed the indices in the expectation file instead of staying blocked on my testharness.js PR.

@bors-servo r=Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Feb 26, 2016

📌 Commit 915c6d8 has been approved by Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Feb 26, 2016

⌛ Testing commit 915c6d8 with merge cdf5eca...
bors-servo added a commit that referenced this pull request Feb 26, 2016
@bors-servo
Auto merge of #9064 - nox:them-attr-names, r=Manishearth
cdf5eca 
Implement Element.getAttributeNames

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

bors-servo commented Feb 26, 2016

💔 Test failed - linux-rel
@KiChjang
Copy link
Member

KiChjang commented Feb 26, 2016 

Ran 4606 tests finished in 229.0 seconds.
  • 4604 ran as expected. 1374 tests skipped.
  • 2 tests had unexpected subtest results

Tests with unexpected results:
  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  │ FAIL [expected PASS] Element interface: document.createElement("noscript") must inherit property "removeAttributeNode" with the proper type (22)
  │   → assert_inherits: property "removeAttributeNode" not found in prototype chain
  │ FAIL [expected PASS] Element interface: document.createElement("noscript") must inherit property "query" with the proper type (34)
  │   → assert_inherits: property "query" not found in prototype chain
  │ FAIL [expected PASS] Element interface: document.createElement("noscript") must inherit property "queryAll" with the proper type (35)
  │   → assert_inherits: property "queryAll" not found in prototype chain
  │ 
  │ IdlInterface.prototype.test_interface_of/<@http://web-platform.test:8000/resources/idlharness.js:1467:25
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1381:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  │ IdlInterface.prototype.test_interface_of@http://web-platform.test:8000/resources/idlharness.js:1461:1
  │ IdlInterface.prototype.test_object@http://web-platform.test:8000/resources/idlharness.js:1373:9
  │ IdlArray.prototype.test/<@http://web-platform.test:8000/resources/idlharness.js:353:17
  │ IdlArray.prototype.test@http://web-platform.test:8000/resources/idlharness.js:351:13
  └ window.onload@http://web-platform.test:8000/html/dom/interfaces.html:237:3

  ▶ Unexpected subtest result in /websockets/cookies/005.html:
  └ PASS [expected FAIL] WebSockets: setting HttpOnly cookies in ws response, checking ws request
@jdm
Copy link
Member

jdm commented Feb 26, 2016

There's no reason the websocket test should be passing with this change :( Probably file that as a new intermittent pass.
@nox
Implement Element.getAttributeNames
Loading status checks…
b8ae589
@nox nox force-pushed the nox:them-attr-names branch from 915c6d8 to b8ae589 Feb 26, 2016
@nox
Copy link
Member Author

nox commented Feb 26, 2016

@bors-servo r=Manishearth

I changed the expectations and filed #9772.
@bors-servo
Copy link
Contributor

bors-servo commented Feb 26, 2016

📌 Commit b8ae589 has been approved by Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Feb 26, 2016

⌛ Testing commit b8ae589 with merge 9186819...
bors-servo added a commit that referenced this pull request Feb 26, 2016
@bors-servo
Auto merge of #9064 - nox:them-attr-names, r=Manishearth
Loading status checks…
9186819 
Implement Element.getAttributeNames

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

bors-servo commented Feb 26, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit b8ae589 into servo:master Feb 26, 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

@Manishearth Manishearth

Labels
A-content/dom C-has-manual-testcase S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@nox @Manishearth @bors-servo @eefriedman @KiChjang @jdm @highfive
You can’t perform that action at this time.