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

Implementing getAttributeNode() and similar methods. #8068

Merged
merged 1 commit into from Oct 19, 2015
Merged

Implementing getAttributeNode() and similar methods. #8068

merged 1 commit into from Oct 19, 2015

Conversation

@martiansideofthemoon
Copy link
Contributor

martiansideofthemoon commented Oct 18, 2015

Attempting to solve #8066 Does it look good so far @Manishearth ?

Review on Reviewable
@nox
Copy link
Member

nox commented Oct 18, 2015

These should return attributes (Option<Root<Attr>>), not their values (Option<DOMString>).
components/script/dom/webidls/Element.webidl Outdated
@@ -37,6 +37,10 @@ interface Element : Node {
DOMString? getAttribute(DOMString name);
[Pure]
DOMString? getAttributeNS(DOMString? namespace, DOMString localName);
[Pure]
DOMString? getAttributeNode(DOMString name);

This comment has been minimized.

Sign in to view
Copy link
@Manishearth

Manishearth Oct 18, 2015

Member

This is incorrect, see https://dom.spec.whatwg.org/#element

  Attr? getAttributeNode(DOMString name);
  Attr? getAttributeNodeNS(DOMString? namespace, DOMString localName);
@martiansideofthemoon
Copy link
Contributor Author

martiansideofthemoon commented Oct 18, 2015

@Manishearth Hope it is better now
@frewsxcv frewsxcv removed the S-fails-tidy label Oct 18, 2015
@Manishearth
Copy link
Member

Manishearth commented Oct 18, 2015

Looks good to me. You need to update the test expectations like you did in #8038

(Also, squash the commits when done. git fetch origin; git rebase -i origin/master, and change all picks except the first to fixup. Then do git push -f .... origin might be something else, it should be the name of the remote pointing to this repo. For future reference, git commit --amend is a way to edit commits without creating new ones)
@martiansideofthemoon
Copy link
Contributor Author

martiansideofthemoon commented Oct 19, 2015

@Manishearth Well I am having issues with changing these .ini files. I always get permission issues unless I run sudo su before editing them. Also, the automatic method here doesn't seem to help. It took an hour to execute and I get a Permission Denied for every test file on running git diff or git commit.

Had to do a manual update like in #8038 , so I might have removed some extra test expectations.

Did the squashing. Will use --amend next time
@martiansideofthemoon martiansideofthemoon force-pushed the martiansideofthemoon:my-code-fix branch from 2552a76 to 6a84d0e Oct 19, 2015
@Manishearth
Copy link
Member

Manishearth commented Oct 19, 2015

You seem to have done the checkout under the wrong user, perhaps?
@Manishearth
Copy link
Member

Manishearth commented Oct 19, 2015

(Also, the r=manishearth isn't necessary)
@Manishearth
Copy link
Member

Manishearth commented Oct 19, 2015

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

bors-servo commented Oct 19, 2015

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

bors-servo commented Oct 19, 2015

⌛ Testing commit 6a84d0e with merge 56745cd...
bors-servo pushed a commit that referenced this pull request Oct 19, 2015
bors-servo
Auto merge of #8068 - martiansideofthemoon:my-code-fix, r=Manishearth
56745cd 
Implementing getAttributeNode() and similar methods

Attempting to solve #8066 Does it look good so far @Manishearth ?

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

martiansideofthemoon commented Oct 19, 2015

@Manishearth sorry, didn't get what you meant by "checkout under the wrong user".
I had run the ./mach command to run wpt tests using sudo, if that makes any difference.
@Manishearth
Copy link
Member

Manishearth commented Oct 19, 2015

yeah; you shouldn't have done that 😄 . Servo doesn't need sudo aside from apt-geting dependencies

Try sudo chown -R <username> tests/wpt.
@bors-servo
Copy link
Contributor

bors-servo commented Oct 19, 2015

💔 Test failed - linux-rel
@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 19, 2015 


/dom/interfaces.html
--------------------
PASS expected FAIL Element interface: operation getAttributeNode(DOMString)
PASS expected FAIL Element interface: operation getAttributeNodeNS(DOMString,DOMString)

You'll need to update those expectations; see tests/wpt/README.md.
@Manishearth
Copy link
Member

Manishearth commented Oct 19, 2015

You can use the log from http://build.servo.org/builders/linux-rel/builds/569/steps/shell_1/logs/test-wpt.log to update.
@martiansideofthemoon
Implementing getAttributeNode() and similar methods.
Loading status checks…
3971b8c
@martiansideofthemoon martiansideofthemoon force-pushed the martiansideofthemoon:my-code-fix branch from 6a84d0e to 3971b8c Oct 19, 2015
@martiansideofthemoon
Copy link
Contributor Author

martiansideofthemoon commented Oct 19, 2015

@Manishearth chown seems to have fixed all issues 😄 Removed two additional test expectations.
@Manishearth
Copy link
Member

Manishearth commented Oct 19, 2015

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

bors-servo commented Oct 19, 2015

📌 Commit 3971b8c has been approved by Manishearth
@martiansideofthemoon
Copy link
Contributor Author

martiansideofthemoon commented Oct 19, 2015

@Manishearth What instructions am I allowed to give @bors-servo ?
@Manishearth
Copy link
Member

Manishearth commented Oct 19, 2015

None of them 😄

bors only listens to reviewers. This might change in the future.
@bors-servo
Copy link
Contributor

bors-servo commented Oct 19, 2015

⌛ Testing commit 3971b8c with merge 2e308e9...
bors-servo pushed a commit that referenced this pull request Oct 19, 2015
bors-servo
Auto merge of #8068 - martiansideofthemoon:my-code-fix, r=Manishearth
Loading status checks…
2e308e9 
Implementing getAttributeNode() and similar methods

Attempting to solve #8066 Does it look good so far @Manishearth ?

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

bors-servo commented Oct 19, 2015

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

Manishearth commented Oct 19, 2015

@bors-servo retry #7870
@bors-servo
Copy link
Contributor

bors-servo commented Oct 19, 2015

⚡ Previous build results for android, gonk, linux-dev, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only linux-rel...
@bors-servo
Copy link
Contributor

bors-servo commented Oct 19, 2015

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 3971b8c into servo:master Oct 19, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@martiansideofthemoon martiansideofthemoon deleted the martiansideofthemoon:my-code-fix branch Oct 20, 2015
@martiansideofthemoon martiansideofthemoon changed the title Implementing getAttributeNode() and similar methods Implementing getAttributeNode() and similar methods. Resolves #8066 Oct 20, 2015
@martiansideofthemoon martiansideofthemoon changed the title Implementing getAttributeNode() and similar methods. Resolves #8066 Implementing getAttributeNode() and similar methods. Oct 20, 2015
@martiansideofthemoon
Copy link
Contributor Author

martiansideofthemoon commented Oct 20, 2015

Resolves #8066
fitzgen added a commit to fitzgen/servo that referenced this pull request Nov 28, 2015
@fitzgen
Implement setAttributeNode and setAttributeNodeNS
b8abc16 
This commit implements setAttributeNode and setAttributeNodeNS, as described
here: https://dom.spec.whatwg.org/#dom-element-setattributenode.

See also servo#8068.
@fitzgen fitzgen mentioned this pull request Nov 28, 2015
Closed
fitzgen added a commit to fitzgen/servo that referenced this pull request Nov 29, 2015
@fitzgen
Implement removeAttributeNode
856f96a 
This commit implement removeAttributeNode, as described here:
https://dom.spec.whatwg.org/#dom-element-removeattributenode

See also servo#8724 and servo#8068.
@fitzgen fitzgen mentioned this pull request Nov 29, 2015
Closed
nox added a commit to nox/servo that referenced this pull request Feb 26, 2016
@fitzgen @nox
Implement removeAttributeNode
d28c9a1 
This commit implement removeAttributeNode, as described here:
https://dom.spec.whatwg.org/#dom-element-removeattributenode

See also servo#8724 and servo#8068.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@martiansideofthemoon @nox @Manishearth @bors-servo @Ms2ger @SimonSapin @frewsxcv @highfive
You can’t perform that action at this time.