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

Add DeleteRow method #12594

Merged
merged 1 commit into from Jul 27, 2016
Merged

Add DeleteRow method #12594

merged 1 commit into from Jul 27, 2016

Conversation

@GuillaumeGomez
Copy link
Contributor

GuillaumeGomez commented Jul 25, 2016 •
edited by larsbergstrom

This change is Reviewable
@highfive
Copy link

highfive commented Jul 25, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/webidls/HTMLTableElement.webidl, components/script/dom/htmltableelement.rs, components/script/dom/htmlcollection.rs
@highfive
Copy link

highfive commented Jul 25, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
components/script/dom/htmltableelement.rs Outdated
return Err(Error::IndexSize);
}
// Step 3.
rows.sections.remove(index as usize);

This comment has been minimized.

Sign in to view
Copy link
@GuillaumeGomez

GuillaumeGomez Jul 25, 2016

Author Contributor

I'm not sure it does remove the row as expected. If someone could confirm please?

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Jul 26, 2016

Member

Write a test?
components/script/dom/htmltableelement.rs Outdated
@@ -338,6 +348,22 @@ impl HTMLTableElementMethods for HTMLTableElement {
Ok(new_row)
}

// https://html.spec.whatwg.org/multipage/tables.html#dom-table-deleterow

This comment has been minimized.

Sign in to view
Copy link
@KiChjang

KiChjang Jul 25, 2016

Member

Tidy will fail on this line.

This comment has been minimized.

Sign in to view
Copy link
@GuillaumeGomez

GuillaumeGomez Jul 25, 2016

Author Contributor

Why?
@jdm
Copy link
Member

jdm commented Jul 26, 2016

How does tests/wpt/web-platform-tests/html/semantics/tabular-data/the-tbody-element/deleteRow.html fare with these changes?
@jdm
Copy link
Member

jdm commented Jul 26, 2016

Or maybe tests/wpt/web-platform-tests/dom/nodes/getElementsByClassName-21.htm instead.
@KiChjang
Copy link
Member

KiChjang commented Jul 26, 2016

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Jul 26, 2016

⌛ Trying commit 0c8410e with merge e372307...
bors-servo added a commit that referenced this pull request Jul 26, 2016
@bors-servo
Auto merge of #12594 - GuillaumeGomez:remove_row, r=<try>
Loading status checks…
e372307 
Add DeleteRow method

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12594)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 26, 2016

💔 Test failed - linux-rel
@KiChjang
Copy link
Member

KiChjang commented Jul 26, 2016 

Ran 6788 tests finished in 805.0 seconds.
  • 6787 ran as expected. 1998 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 deleteRow(long)

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

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

Looks like no real WPT tests have been fixed by this patch...
@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 26, 2016

This doesn't actually remove the element from the dom tree, does it?
@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Jul 26, 2016

@Ms2ger: It should now. I'm testing if it does it correctly.
@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Jul 26, 2016

I added tests now as well. I think it's ready.
tests/wpt/web-platform-tests/dom/nodes/remove-row.html Outdated
@@ -0,0 +1,50 @@
<!doctype html>
<meta charset="utf-8">
<title></title>

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Jul 26, 2016

Member

nit: Add a title here
@jdm jdm added the S-fails-travis label Jul 26, 2016
@jdm
Copy link
Member

jdm commented Jul 26, 2016

Travis says the MANIFEST.json changes aren't quite right; fix it with ./mach update-manifest.
@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Jul 26, 2016

Updated.
@GuillaumeGomez
Add DeleteRow method
Loading status checks…
cf9fd7e
tests/wpt/metadata/MANIFEST.json
@@ -15226,6 +15226,10 @@
"url": "/dom/nodes/prepend-on-Document.html"
},
{
"path": "dom/nodes/remove-row.html",

This comment has been minimized.

Sign in to view
Copy link
@Ms2ger

Ms2ger Jul 27, 2016

Contributor

This test should go in its proper place: tests/wpt/web-platform-tests/html/semantics/tabular-data/the-table-element/.
@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Jul 27, 2016

Updated but the Manifest.JSON file looks a bit weird.
@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 27, 2016

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jul 27, 2016

📌 Commit cf9fd7e has been approved by Ms2ger
@highfive highfive assigned Ms2ger and unassigned metajack Jul 27, 2016
@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 27, 2016

@bors-servo p=11
@nox nox removed the S-fails-travis label Jul 27, 2016
bors-servo added a commit that referenced this pull request Jul 27, 2016
@bors-servo
Auto merge of #12594 - GuillaumeGomez:remove_row, r=Ms2ger
Loading status checks…
fe3b0f2 
Add DeleteRow method

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12594)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 27, 2016

⌛ Testing commit cf9fd7e with merge fe3b0f2...
@bors-servo
Copy link
Contributor

bors-servo commented Jul 27, 2016

💔 Test failed - linux-rel
@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 27, 2016

@bors-servo retry p=0
@bors-servo
Copy link
Contributor

bors-servo commented Jul 27, 2016

⌛ Testing commit cf9fd7e with merge 9693295...
bors-servo added a commit that referenced this pull request Jul 27, 2016
@bors-servo
Auto merge of #12594 - GuillaumeGomez:remove_row, r=Ms2ger
Loading status checks…
9693295 
Add DeleteRow method

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12594)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 27, 2016

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev
@bors-servo bors-servo merged commit cf9fd7e into servo:master Jul 27, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Jul 27, 2016
Merged
@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:remove_row branch Jul 27, 2016
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
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
@GuillaumeGomez @highfive @jdm @KiChjang @bors-servo @Ms2ger @emilio @metajack @nox
You can’t perform that action at this time.