Implement HTMLTableRowElement insertCell and deleteCell #7977

frewsxcv · 2015-10-12T04:15:36Z

frewsxcv · 2015-10-12T04:16:09Z

components/script/dom/utils.rs

+use dom::htmlelement::HTMLElement;
+use dom::node::Node;
+use std::iter;
+


So I'm not a huge fan with how these generic functions turned out. If anyone has any suggestions to clean these up, let me know

eefriedman · 2015-10-12T06:23:11Z

components/script/dom/utils.rs

+    }
+
+    let node = NodeCast::from_ref(this);
+    let tr = new_child(node);


It would be a bit simpler to just unconditionally build the child in the caller.

eefriedman · 2015-10-12T06:26:11Z

components/script/dom/utils.rs

+            match last_child.and_then(|node| node.inclusively_preceding_siblings()
+                                                 .filter_map(ElementCast::to_root)
+                                                 .filter(|elem| is_delete_type(elem))
+                                                 .next()) {


This would be a lot simpler in both the caller and callee if you could just write let last_child = get_items().LastItem();. (I know it doesn't exist at the moment, but it could be added.)

eefriedman · 2015-10-12T06:28:59Z

components/script/dom/utils.rs

+                                                 .filter(|elem| is_delete_type(elem))
+                                                 .next()) {
+                Some(element) => element,
+                None => return Ok(()),


https://html.spec.whatwg.org/multipage/tables.html#dom-table-deleterow explicitly says this should throw.

Err, oops, that isn't actually the algorithm you're implementing here... that said, I would tend towards the interpretation that it's an unintentional hole in the deleteRow/deleteCell definitions: "remove the last element in the rows collection" makes no sense if the rows collection is empty.

(For reference, it looks like Chrome throws, but Firefox doesn't.)

Ms2ger · 2015-10-12T11:42:50Z

Do file a spec issue on the hole.

frewsxcv · 2015-10-12T12:42:15Z

Spec issue filed: whatwg/html#248

jdm · 2015-10-13T14:51:19Z

@eefriedman I've noticed you've been doing drive-by reviews in a bunch of PRs recently (thanks!). Would you be interested in doing a complete review of these changes in Reviewable?

eefriedman · 2015-10-13T17:08:29Z

Review status: 0 of 9 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

^{tests/wpt/web-platform-tests/html/semantics/tabular-data/the-tr-element/deleteCell.html, line 52 [r1] (raw file):}
This doesn't match the style of the rest of the test

Comments from the review on Reviewable.io

eefriedman · 2015-10-13T17:08:46Z

Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io

bors-servo · 2015-10-14T20:48:47Z

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

frewsxcv · 2015-10-15T03:22:05Z

^{components/script/dom/utils.rs, line 34 [r1] (raw file):}
Maybe, but this avoids an unnecessary allocation if it returns early

Comments from the review on Reviewable.io

frewsxcv · 2015-10-15T03:22:24Z

^{tests/wpt/web-platform-tests/html/semantics/tabular-data/the-tr-element/deleteCell.html, line 52 [r1] (raw file):}
In what way?

Comments from the review on Reviewable.io

frewsxcv · 2015-10-15T03:25:45Z

I did a little refactoring and got rid of the components/script/dom/util.rs file. Let me know what you think.

eefriedman · 2015-10-15T05:41:23Z

Reviewed 1 of 9 files at r1.
Review status: 4 of 8 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

^{tests/wpt/web-platform-tests/html/semantics/tabular-data/the-tr-element/deleteCell.html, line 52 [r1] (raw file):}
Oh, nevermind, I see what you're trying to do.

Comments from the review on Reviewable.io

bors-servo · 2015-10-15T19:41:26Z

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


        Implement HTMLTableRowElement insertCell and deleteCell

frewsxcv · 2015-10-17T04:09:01Z

Merge conflicts addressed in the latest force push. @eefriedman did you have any other comments?

eefriedman · 2015-10-17T04:19:20Z

Maybe we should follow the predicted direction of whatwg/html#248 ?

Otherwise looks good.

jdm · 2015-10-17T05:19:06Z

I'd rather implement the current spec behaviour and update accordingly after the spec is changed.

jdm · 2015-10-17T05:19:22Z

@bors-servo: r=eefriedman
Thanks @frewsxcv and @eefriedman!

bors-servo · 2015-10-17T05:19:23Z

📌 Commit 1f58169 has been approved by eefriedman

bors-servo · 2015-10-17T05:54:53Z

⌛ Testing commit 1f58169 with merge ef81195...


        Auto merge of #7977 - frewsxcv:htmltablerowelement-insertcell-deletec…

…ell, r=eefriedman Implement HTMLTableRowElement insertCell and deleteCell  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7977)

bors-servo · 2015-10-17T06:20:32Z

💔 Test failed - linux-rel

Ms2ger · 2015-10-17T12:13:05Z

@bors-servo retry

#7870

bors-servo · 2015-10-17T12:13:07Z

⚡ 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 · 2015-10-17T12:46:49Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

highfive added the S-awaiting-review label Oct 12, 2015

frewsxcv reviewed Oct 12, 2015
View changes

eefriedman reviewed Oct 12, 2015
View changes

components/script/dom/utils.rs Outdated

}

let node = NodeCast::from_ref(this);

let tr = new_child(node);

This comment has been minimized.

Sign in to view

Copy link

eefriedman Oct 12, 2015
Contributor

It would be a bit simpler to just unconditionally build the child in the caller.

eefriedman reviewed Oct 12, 2015
View changes

frewsxcv mentioned this pull request Oct 12, 2015

Spec hole in <tbody> 'deleteRow' and <tr> 'deleteCell' whatwg/html#248

Closed

jdm added S-needs-code-changes and removed S-awaiting-review labels Oct 13, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Oct 14, 2015

frewsxcv force-pushed the frewsxcv:htmltablerowelement-insertcell-deletecell branch from abbb54e to c3d8fdc Oct 15, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 15, 2015

frewsxcv force-pushed the frewsxcv:htmltablerowelement-insertcell-deletecell branch from c3d8fdc to 9e79cce Oct 15, 2015

jdm removed the S-needs-rebase label Oct 15, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Oct 15, 2015

Implement HTMLTableRowElement insertCell and deleteCell

Loading status checks…

1f58169

frewsxcv force-pushed the frewsxcv:htmltablerowelement-insertcell-deletecell branch from 9e79cce to 1f58169 Oct 17, 2015

frewsxcv removed the S-needs-rebase label Oct 17, 2015

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 17, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 17, 2015

bors-servo merged commit 1f58169 into servo:master Oct 17, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

bors-servo mentioned this pull request Oct 17, 2015

Introduce trait Castable #8041

Merged

frewsxcv deleted the frewsxcv:htmltablerowelement-insertcell-deletecell branch Oct 17, 2015

SimonSapin removed the S-tests-failed label Jan 27, 2016

servo / servo

Implement HTMLTableRowElement insertCell and deleteCell #7977

Implement HTMLTableRowElement insertCell and deleteCell #7977

frewsxcv commented Oct 12, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Ms2ger commented Oct 12, 2015

frewsxcv commented Oct 12, 2015

jdm commented Oct 13, 2015

eefriedman commented Oct 13, 2015

eefriedman commented Oct 13, 2015

bors-servo commented Oct 14, 2015

frewsxcv commented Oct 15, 2015

frewsxcv commented Oct 15, 2015

frewsxcv commented Oct 15, 2015

eefriedman commented Oct 15, 2015

bors-servo commented Oct 15, 2015

frewsxcv commented Oct 17, 2015

eefriedman commented Oct 17, 2015

jdm commented Oct 17, 2015

jdm commented Oct 17, 2015

bors-servo commented Oct 17, 2015

bors-servo commented Oct 17, 2015

bors-servo commented Oct 17, 2015

Ms2ger commented Oct 17, 2015

bors-servo commented Oct 17, 2015

bors-servo commented Oct 17, 2015

servo / servo

Join GitHub today

Implement HTMLTableRowElement insertCell and deleteCell #7977

Implement HTMLTableRowElement insertCell and deleteCell #7977

Conversation

frewsxcv commented Oct 12, 2015

This comment has been minimized.

frewsxcv Oct 12, 2015

This comment has been minimized.

eefriedman Oct 12, 2015

This comment has been minimized.

eefriedman Oct 12, 2015

This comment has been minimized.

eefriedman Oct 12, 2015

This comment has been minimized.

eefriedman Oct 12, 2015

Ms2ger commented Oct 12, 2015

frewsxcv commented Oct 12, 2015

jdm commented Oct 13, 2015

eefriedman commented Oct 13, 2015

eefriedman commented Oct 13, 2015

bors-servo commented Oct 14, 2015

frewsxcv commented Oct 15, 2015

frewsxcv commented Oct 15, 2015

frewsxcv commented Oct 15, 2015

eefriedman commented Oct 15, 2015

bors-servo commented Oct 15, 2015

frewsxcv commented Oct 17, 2015

eefriedman commented Oct 17, 2015

jdm commented Oct 17, 2015

jdm commented Oct 17, 2015

bors-servo commented Oct 17, 2015

bors-servo commented Oct 17, 2015

bors-servo commented Oct 17, 2015

Ms2ger commented Oct 17, 2015

bors-servo commented Oct 17, 2015

bors-servo commented Oct 17, 2015