Insert row

[g-k]

Fixes #9269

---

This change is

[highfive]

Heads up! This PR modifies the following files:

• @KiChjang: components/script/dom/webidls/HTMLTableElement.webidl, components/script/dom/htmltableelement.rs

[KiChjang]

Great job! I'm not entirely sure we need all those .expect() there though... @nox or @jdm, do you think an .unwrap() is good enough for these use cases? I'm mostly concerned about polluting the error logs.

---

Reviewed 4 of 5 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

components/script/dom/htmltableelement.rs, line 143 [r2] (raw file):
Fallible<Root<HTMLTableRowElement>>

---

components/script/dom/htmltableelement.rs, line 190 [r2] (raw file):
This doesn't look correct. The new_row should be inserted before the _i_th element, not appended as a child to its parent.

---

Comments from Reviewable

[jdm]

I have no problem with choosing expect over unwrap.

[frewsxcv]

Great job! I'm not entirely sure we need all those .expect() there though...

All the expect()s on AppendChilds should probably use try!, right?

[g-k]

Review status: 4 of 5 files reviewed at latest revision, 2 unresolved discussions.

---

components/script/dom/htmltableelement.rs, line 143 [r2] (raw file):
Done.

---

components/script/dom/htmltableelement.rs, line 190 [r2] (raw file):
d'oh! Fixed.

---

Comments from Reviewable

[g-k]

Let me know if I should update the Option unwrapping to be more concise with try! or unwrap.

[KiChjang]

@g-k I think it's better to use try! here, since the method is explicitly marked as Fallible.

[KiChjang]

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

---

Reviewed 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

components/script/dom/domexception.rs, line 105 [r4] (raw file):
Is there a spec link that defines this UnknownError type?

---

components/script/dom/htmltableelement.rs, line 190 [r2] (raw file):
This is still not fixed; you're inserting the new node before the parent, not before the _i_th row.

---

Comments from Reviewable

[Ms2ger]

I'm sorry, but @KiChjang is wrong in suggesting try!() here. If the logic of the function ensures that the operation will always be successful, we should .expect() or .unwrap().

[bors-servo]

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

[KiChjang]

@bors-servo r-

Sorry, this still needs to be squashed. Your first commit alone will cause build errors.

[bors-servo]

💔 Test failed - linux-rel

[KiChjang]

@bors-servo r+

Thanks!

[bors-servo]

📌 Commit cf425a1 has been approved by KiChjang

[KiChjang]

@bors-servo r-

The test expectations need to be updated.

Ran 4677 tests finished in 357.0 seconds.
  • 4676 ran as expected. 1363 tests skipped.
  • 1 tests had unexpected subtest results

Tests with unexpected results:
  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLTableElement interface: operation insertRow(long)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLTableElement interface: document.createElement("table") must inherit property "insertRow" with the proper type (12)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLTableElement interface: calling insertRow(long) on document.createElement("table") with too few arguments must throw TypeError

[KiChjang]

@bors-servo r+

[bors-servo]

📌 Commit 1d59d87 has been approved by KiChjang

[bors-servo]

⌛ Testing commit 1d59d87 with merge 19a5a9a...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt