Implement deleteRow and insertRow for table section elements

[frewsxcv]

Continued from #6936

[nox]

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

---

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

---

^{components/script/dom/htmltablesectionelement.rs, line 12 [r1] (raw file):}
I would rather use Error instead of its constructors.

---

^{components/script/dom/htmltablesectionelement.rs, line 81 [r1] (raw file):}
This means traversing the rows twice, doesn't it?

---

^{components/script/dom/htmltablesectionelement.rs, line 92 [r1] (raw file):}
Couldn't you just check that index is equal to -1 here?

---

^{components/script/dom/htmltablesectionelement.rs, line 109 [r1] (raw file):}
I would rather get the last row directly if index is -1, this avoids a redundant traversal of the collection.

---

Comments from the review on Reviewable.io

[frewsxcv]

^{components/script/dom/htmltablesectionelement.rs, line 109 [r1] (raw file):}

get the last row directly

What would be the best way of doing this? Make HTMLCollection (.Rows()) a double ended iterator and call next_back() on it?

---

Comments from the review on Reviewable.io

[nox]

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

---

^{components/script/dom/htmltablesectionelement.rs, line 109 [r1] (raw file):}
node.GetLastChild().inclusively_preceding_siblings().map_filter(HTMLTableRowElement::to_root).next()?

---

Comments from the review on Reviewable.io

[frewsxcv]

Addressed all the comments.

[frewsxcv]

Hm, it actually doesn't work in one case. I'll add a regression test.

[frewsxcv]

Okay, force pushed. Let me know how that looks.

[eefriedman]

The explicit for loop and indexing is a little difficult to understand at first glance... maybe something like this would be more clear? Or am I going too far down the path of iterator chaining?

    let element = self.Rows()
                      .elements_iter()
                      .map(|r| Some(r)))
                      .chain(iter::once(None))
                      .nth(index);
    match element {
        None => Err(Error::IndexSize),
        Some(None) => {
            try!(self_node.AppendChild(NodeCast::from_ref(tr.r())));
            Ok(HTMLElementCast::from_root(tr))
        }
        Some(element) => {
            try!(self_node.InsertBefore(NodeCast::from_ref(tr.r()),
                                        Some(NodeCast::from_root(element)).r()));
            Ok(HTMLElementCast::from_root(tr))
        }
    }

[frewsxcv]

So I saw your solution, but decided I wasn't a fan of the nested Options, so I changed it up a little bit. Check out my latest force push and let me know what you think

[eefriedman]

Looks good.

[nox]

-S-awaiting-review +I-spec-unclear +S-needs-code-changes

---

Reviewed 3 of 4 files at r2.
Review status: 5 of 7 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

---

^{components/script/dom/htmlcollection.rs, line 163 [r2] (raw file):}
This should be named elements(), like children() on Node.

---

^{components/script/dom/htmltablesectionelement.rs, line 86 [r2] (raw file):}
We usually call that just node.

---

^{components/script/dom/htmltablesectionelement.rs, line 87 [r2] (raw file):}
We usually do "tr".to_owned(). So many ways to make String values, we should file an issue to make that consistent.

---

^{components/script/dom/htmltablesectionelement.rs, line 109 [r2] (raw file):}
We can use InsertBefore in both cases. What about that:

let before_child = self.Rows().elements().nth(index).and_then(|row|{
    NodeCast::from_ref(&row).GetNextSibling()
});
try!(node.InsertBefore(NodeCast::from_ref(&tr), before_child.r());
Ok(HTMLElementCast::from_root(tr))

---

^{components/script/dom/htmltablesectionelement.rs, line 119 [r2] (raw file):}
You are not supposed to throw an error if you pass -1 and there are no rows, reading the spec:

If index is less than −1 or greater than the number of elements in the rows collection, the method must throw an IndexSizeError exception.

Not sure if that's intended or not. If it is, you can make all that code a if let expression.

---

Comments from the review on Reviewable.io

[nox]

Review status: 5 of 7 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

---

^{components/script/dom/htmltablesectionelement.rs, line 109 [r2] (raw file):}
Err, I forgot about the case where there not enough rows, disregard the snippet and focus on the InsertBefore part. :)

---

Comments from the review on Reviewable.io

[nox]

Err, didn't review the latest revision… Fortunately, my remarks still stand.

---

Reviewed 1 of 4 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

---

Comments from the review on Reviewable.io

[frewsxcv]

Comments have been addressed.

[nox]

@bors-servo r+

---

Reviewed 4 of 4 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

Comments from the review on Reviewable.io

[bors-servo]

📌 Commit 3d383f2 has been approved by nox

[bors-servo]

⌛ Testing commit 3d383f2 with merge 292dbfe...

[bors-servo]

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