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

Partially implement getter of Document #5921

Merged
merged 5 commits into from May 14, 2015
Merged

Conversation

@nox
Copy link
Member

nox commented May 2, 2015

Review on Reviewable

@nox
Copy link
Member Author

nox commented May 2, 2015

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented May 2, 2015

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

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
Member Author

nox commented May 2, 2015

The two commits "make id attribute use an atom" should be "make name attribute use an atom", will reword once reviewed.

@jdm
Copy link
Member

jdm commented May 2, 2015

Why is the last commit removing WPT test files?

@nox
Copy link
Member Author

nox commented May 2, 2015

Because I'm stupid and fail at git-rm'ing.

@nox nox force-pushed the nox:document-getter branch from 59d5669 to 644fd81 May 2, 2015
@nox
Copy link
Member Author

nox commented May 2, 2015

@jdm I restored the tests and removed the expected results as they now pass (with my WPT PR).

@bors-servo
Copy link
Contributor

bors-servo commented May 5, 2015

The latest upstream changes (presumably #5935) made this pull request unmergeable. Please resolve the merge conflicts.

@nox nox force-pushed the nox:document-getter branch from 644fd81 to ca94e9c May 5, 2015
@jdm
Copy link
Member

jdm commented May 6, 2015

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


Reviewed files:

  • components/script/dom/document.rs @ r5
  • components/script/dom/htmlappletelement.rs @ r2
  • components/script/dom/htmlbodyelement.rs @ r1
  • components/script/dom/htmlbuttonelement.rs @ r1
  • components/script/dom/htmldialogelement.rs @ r1
  • components/script/dom/htmlfieldsetelement.rs @ r1
  • components/script/dom/htmlformelement.rs @ r4
  • components/script/dom/htmlimageelement.rs @ r3
  • components/script/dom/htmlmetaelement.rs @ r1
  • components/script/dom/htmlobjectelement.rs @ r1
  • components/script/dom/htmloptgroupelement.rs @ r1
  • components/script/dom/htmloptionelement.rs @ r1
  • components/script/dom/htmlselectelement.rs @ r1
  • components/script/dom/macros.rs @ r1, r2
  • components/script/dom/virtualmethods.rs @ r2, r4
  • components/script/dom/webidls/Document.webidl @ r5
  • components/script/dom/webidls/HTMLAppletElement.webidl @ r2
  • tests/wpt/metadata/html/dom/documents/dom-tree-accessors/nameditem-01.html.ini @ r5
  • tests/wpt/metadata/html/dom/documents/dom-tree-accessors/nameditem-03.html.ini @ r5
  • tests/wpt/metadata/html/dom/documents/dom-tree-accessors/nameditem-04.html.ini @ r5
  • tests/wpt/metadata/html/dom/documents/dom-tree-accessors/nameditem-06.html.ini @ r5
  • tests/wpt/metadata/html/dom/interfaces.html.ini @ r2
  • tests/wpt/metadata/html/dom/reflection-obsolete.html.ini @ r2

components/script/dom/document.rs, line 93 [r5] (raw file):
nit: std::sync comes after std::ptr


components/script/dom/document.rs, line 1607 [r5] (raw file):
Why this block?


components/script/dom/webidls/Document.webidl, line 84 [r5] (raw file):
nit: leave the extra space after the //


components/script/dom/webidls/HTMLAppletElement.webidl, line 15 [r2] (raw file):
nit: it's preferable to leave the spacing as-is and just remove the //.


Comments from the review on Reviewable.io

@nox nox force-pushed the nox:document-getter branch from ca94e9c to 22d9cc3 May 6, 2015
@nox
Copy link
Member Author

nox commented May 6, 2015

components/script/dom/document.rs, line 1607 [r5] (raw file):
Because elements borrows name.


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented May 6, 2015

-S-needs-code-changes +S-needs-squash


Comments from the review on Reviewable.io

@nox nox force-pushed the nox:document-getter branch from 22d9cc3 to 1c8c0ca May 6, 2015
@jdm
Copy link
Member

jdm commented May 6, 2015

@bors-servo
Copy link
Contributor

bors-servo commented May 6, 2015

📌 Commit 1c8c0ca has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented May 6, 2015

Testing commit 1c8c0ca with merge d23d077...

bors-servo pushed a commit that referenced this pull request May 6, 2015
<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5921)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 6, 2015

💔 Test failed - mac1

@jdm
Copy link
Member

jdm commented May 6, 2015

/html/dom/documents/dom-tree-accessors/nameditem-06.html
--------------------------------------------------------
FAIL If there are two imgs, nothing should be returned. (id)
@jdm jdm added S-tests-failed and removed S-awaiting-merge labels May 6, 2015
@nox
Copy link
Member Author

nox commented May 6, 2015

@jdm It needs web-platform-tests/wpt#1801, was WPT updated?

@jdm
Copy link
Member

jdm commented May 6, 2015

Whoops; not yet.

@bors-servo
Copy link
Contributor

bors-servo commented May 6, 2015

The latest upstream changes (presumably #5800) made this pull request unmergeable. Please resolve the merge conflicts.

@nox nox force-pushed the nox:document-getter branch from 1c8c0ca to 0e8bd5d May 14, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented May 14, 2015

@bors-servo: r=jdm

@bors-servo
Copy link
Contributor

bors-servo commented May 14, 2015

📌 Commit 0e8bd5d has been approved by jdm

bors-servo pushed a commit that referenced this pull request May 14, 2015
<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5921)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 14, 2015

Testing commit 0e8bd5d with merge a635a8f...

@bors-servo
Copy link
Contributor

bors-servo commented May 14, 2015

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

@bors-servo bors-servo merged commit 0e8bd5d into servo:master May 14, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@nox nox deleted the nox:document-getter branch Aug 20, 2015
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

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