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::hasAttributes #10762

Merged
merged 1 commit into from Apr 22, 2016
Merged

Implement Element::hasAttributes #10762

merged 1 commit into from Apr 22, 2016

Conversation

@canova
Copy link
Member

canova commented Apr 20, 2016

Fixes #10748 .
Implement Element::hasAttributes. I'm not sure if tests are enough. I'm open to suggestion :)

This change is Reviewable
@highfive
Copy link

highfive commented Apr 20, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/element.rs, components/script/dom/webidls/Element.webidl
tests/wpt/web-platform-tests/dom/nodes/Element-hasAttributes.html Outdated

}, 'element.hasAttributes() must return true when the element has attribute.');

</script>

This comment has been minimized.

Sign in to view
Copy link
@frewsxcv

frewsxcv Apr 20, 2016

Member

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

// https://dom.spec.whatwg.org/#dom-element-hasattributes
fn HasAttributes(&self) -> bool {
self.attrs.borrow().len() > 0

This comment has been minimized.

Sign in to view
Copy link
@frewsxcv

frewsxcv Apr 20, 2016

Member

Alternatively:

!self.attrs.borrow().is_empty()
@canova
Copy link
Member Author

canova commented Apr 21, 2016

@frewsxcv changed, thanks!
@nox
Copy link
Member

nox commented Apr 21, 2016

There should be failing tests when running tests/wpt/web-platform-tests/dom/interfaces.html and tests/wpt/web-platform-tests/html/dom/interfaces.html. Please update the corresponding test expectations:

./mach test-wpt --log-raw raw.log /dom/interfaces.html /html/dom/interfaces.html
./mach update-wpt --no-patch raw.log

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

Reviewed 4 of 4 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable
@bors-servo
Copy link
Contributor

bors-servo commented Apr 21, 2016

☔ The latest upstream changes (presumably #10777) made this pull request unmergeable. Please resolve the merge conflicts.
@canova canova force-pushed the canova:has_attributes branch from 0530961 to f6f4749 Apr 22, 2016
@canova
Copy link
Member Author

canova commented Apr 22, 2016

@nox Updated, thanks for help :)
@jdm jdm removed the S-needs-rebase label Apr 22, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Apr 22, 2016

☔ The latest upstream changes (presumably #10727) made this pull request unmergeable. Please resolve the merge conflicts.
@nox
Copy link
Member

nox commented Apr 22, 2016

Do you need help rebasing it?
@canova canova force-pushed the canova:has_attributes branch from f6f4749 to 36619fc Apr 22, 2016
@canova
Implement Element::hasAttributes
Loading status checks…
47f43d4
@canova canova force-pushed the canova:has_attributes branch from 36619fc to 47f43d4 Apr 22, 2016
@canova
Copy link
Member Author

canova commented Apr 22, 2016

Sorry for delay. Rebased it, thanks!
@nox
Copy link
Member

nox commented Apr 22, 2016

No problem, that's a very short delay. :)

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Apr 22, 2016

📌 Commit 47f43d4 has been approved by nox
@bors-servo
Copy link
Contributor

bors-servo commented Apr 22, 2016

⌛ Testing commit 47f43d4 with merge 8163347...
bors-servo added a commit that referenced this pull request Apr 22, 2016
@bors-servo
Auto merge of #10762 - canaltinova:has_attributes, r=nox
Loading status checks…
8163347 
Implement Element::hasAttributes

Fixes #10748 .
Implement Element::hasAttributes. I'm not sure if tests are enough. I'm open to suggestion :)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10762)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 22, 2016

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 47f43d4 into servo:master Apr 22, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@canova canova deleted the canova:has_attributes branch Apr 23, 2016
@notriddle notriddle mentioned this pull request May 19, 2016
Closed
4 of 4 tasks complete
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
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@canova @highfive @nox @bors-servo @frewsxcv @jdm
You can’t perform that action at this time.