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 HTMLTableCellElement::CellIndex #7829

Merged
merged 1 commit into from Oct 14, 2015
Merged

Implement HTMLTableCellElement::CellIndex #7829

merged 1 commit into from Oct 14, 2015

Conversation

@frewsxcv
Copy link
Member

frewsxcv commented Oct 2, 2015

Extracted from #6936

Review on Reviewable
@frewsxcv frewsxcv force-pushed the frewsxcv:htmltablecellelement-cellindex branch from 82d7cfa to e7b869f Oct 2, 2015
components/script/dom/htmltablecellelement.rs Outdated
if let Some(tr) = NodeCast::from_ref(self).GetParentNode() {
if let Some(tr) = HTMLTableRowElementCast::to_root(tr) {
let this = Some(Root::from_ref(ElementCast::from_ref(self)));
for i in 0..tr.Cells().Length() {

This comment has been minimized.

Sign in to view
@Ms2ger

Ms2ger Oct 2, 2015

Contributor

Using Cells and Item kinda sucks. Why not use children().position()? That should be a lot more efficient, and because we compare to self, would still be correct.
@frewsxcv frewsxcv force-pushed the frewsxcv:htmltablecellelement-cellindex branch 2 times, most recently from 68bdef5 to 8d4ce78 Oct 3, 2015
@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 3, 2015

components/script/dom/htmltablecellelement.rs, line 91 [r1] (raw file):
Good idea. Let me know how it looks now.

Comments from the review on Reviewable.io
@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 5, 2015

Reviewed 3 of 4 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

components/script/dom/htmltablecellelement.rs, line 94 [r2] (raw file):
I'm sorry, I just realized this isn't correct when the parent has non-th/td children. Please add a test, both with text/comment nodes and element nodes.

Comments from the review on Reviewable.io
@frewsxcv frewsxcv force-pushed the frewsxcv:htmltablecellelement-cellindex branch 2 times, most recently from c252fc5 to d335e85 Oct 6, 2015
@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 6, 2015

How's that?
@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 6, 2015

Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

tests/wpt/web-platform-tests/html/semantics/tabular-data/attributes-common-to-td-and-th-elements/cellIndex.html, line 48 [r3] (raw file):
Looking great. Can you add another test where you insert some CharacterData nodes rather than the div in this test?

Comments from the review on Reviewable.io
@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 6, 2015

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

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io
@bors-servo
Copy link
Contributor

bors-servo commented Oct 6, 2015

☔ The latest upstream changes (presumably #7874) made this pull request unmergeable. Please resolve the merge conflicts.
@frewsxcv frewsxcv force-pushed the frewsxcv:htmltablecellelement-cellindex branch from d335e85 to 4df2de4 Oct 6, 2015
@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 6, 2015

tests/wpt/web-platform-tests/html/semantics/tabular-data/attributes-common-to-td-and-th-elements/cellIndex.html, line 48 [r3] (raw file):
Done

Comments from the review on Reviewable.io
@jdm jdm added S-fails-tidy and removed S-needs-rebase labels Oct 6, 2015
@frewsxcv
Implement HTMLTableCellElement::CellIndex
Loading status checks…
899f1ca 
Extracted from #6936
@frewsxcv frewsxcv force-pushed the frewsxcv:htmltablecellelement-cellindex branch from 4df2de4 to 899f1ca Oct 6, 2015
@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 6, 2015

Tidy issues addressed.
@frewsxcv frewsxcv removed the S-fails-tidy label Oct 6, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 14, 2015

@bors-servo r+
-S-awaiting-review +S-awaiting-merge

Reviewed 2 of 2 files at r4.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io
@bors-servo
Copy link
Contributor

bors-servo commented Oct 14, 2015

📌 Commit 899f1ca has been approved by Ms2ger
@bors-servo
Copy link
Contributor

bors-servo commented Oct 14, 2015

⌛ Testing commit 899f1ca with merge 55769b2...
bors-servo pushed a commit that referenced this pull request Oct 14, 2015
bors-servo
Auto merge of #7829 - frewsxcv:htmltablecellelement-cellindex, r=Ms2ger
Loading status checks…
55769b2 
Implement HTMLTableCellElement::CellIndex

Extracted from #6936

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

bors-servo commented Oct 14, 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 899f1ca into servo:master Oct 14, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Oct 14, 2015
Merged
@frewsxcv frewsxcv deleted the frewsxcv:htmltablecellelement-cellindex branch Oct 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Ms2ger Ms2ger

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@frewsxcv @Ms2ger @bors-servo @jdm @highfive
You can’t perform that action at this time.