Summary: | Update HTMLTableRowElement.rowIndex to behave according to the specification | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||
Component: | DOM | Assignee: | Chris Dumez <cdumez> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | buildbot, cdumez, commit-queue, esprehn+autocc, gyuyoung.kim, rniwa, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
URL: | https://html.spec.whatwg.org/multipage/tables.html#dom-tr-rowindex | ||||||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2015-09-04 16:16:42 PDT
Created attachment 261702 [details]
Patch
Comment on attachment 261702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261702&action=review > Source/WebCore/ChangeLog:25 > + The code is refactored a bit to leverage HTMLTableElement.rows(), as > + per the specification: > + "return the index of the tr element in that table element's rows collection" > + > + Previously, we would duplicate the table traversal logic. This > + simplifies the code a bit. Is the new implementation more or less efficient than the old one? I understand that it’s simpler. > Source/WebCore/html/HTMLTableRowElement.cpp:59 > + auto* parent = parentNode(); Maybe consider using parentElement here instead of parentNode. > Source/WebCore/html/HTMLTableRowElement.cpp:73 > + unsigned rowCount = rows->length(); I would have named this local variable "length". Comment on attachment 261702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261702&action=review > Source/WebCore/ChangeLog:3 > + tr.rowIndex should return 0 when tr is the only direct child of a table element I don’t see any tests that match this description of the bug. The one test changing its result is not about rowIndex returning 0. Comment on attachment 261702 [details] Patch Attachment 261702 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/194779 New failing tests: fast/table/rowindex.html Created attachment 261705 [details]
Archive of layout-test-results from ews102 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 261702 [details] Patch Attachment 261702 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/194809 New failing tests: fast/table/rowindex.html Created attachment 261707 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 261713 [details]
Patch
Comment on attachment 261702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261702&action=review >> Source/WebCore/ChangeLog:3 >> + tr.rowIndex should return 0 when tr is the only direct child of a table element > > I don’t see any tests that match this description of the bug. The one test changing its result is not about rowIndex returning 0. It is though. We filed this bug specifically because of this test that was failing. The test looks like this: """ test(function() { var row = document.createElement("table") .appendChild(document.createElement("tr")); assert_equals(row.rowIndex, 0); }); """ It just doesn't show in the -expected.txt. >> Source/WebCore/ChangeLog:25 >> + simplifies the code a bit. > > Is the new implementation more or less efficient than the old one? I understand that it’s simpler. It should be equivalent in terms of performance. If performance is a problem, I am sure we can make HTMLTableRowsCollection a bit faster. >> Source/WebCore/html/HTMLTableRowElement.cpp:59 >> + auto* parent = parentNode(); > > Maybe consider using parentElement here instead of parentNode. Not the first time we discuss this but parentElement() will do an unnecessary isElementNode() check. Also, knowing the parent is an Element is not going to make the is<HTMLTableElement>() check any faster. >> Source/WebCore/html/HTMLTableRowElement.cpp:73 >> + unsigned rowCount = rows->length(); > > I would have named this local variable "length". Ok. Created attachment 261714 [details]
Patch
Comment on attachment 261714 [details] Patch Clearing flags on attachment: 261714 Committed r190099: <http://trac.webkit.org/changeset/190099> All reviewed patches have been landed. Closing bug. This made fast/table/rowindex.html fail on Windows. Chris, could you please take a look? Comment on attachment 261714 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261714&action=review > LayoutTests/ChangeLog:12 > + * platform/mac/fast/table/rowindex-expected.txt: Looks like there are also many other rowindex-expected files: platform/efl/fast/table/rowindex-expected.png platform/efl/fast/table/rowindex-expected.txt platform/gtk/fast/table/rowindex-expected.png platform/gtk/fast/table/rowindex-expected.txt platform/ios-simulator/fast/table/rowindex-expected.txt platform/ios-simulator-wk2/fast/table/rowindex-expected.txt platform/mac/fast/table/rowindex-expected.png platform/win/fast/table/rowindex-expected.txt What should we do about all of those!? (In reply to comment #15) > Comment on attachment 261714 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=261714&action=review > > > LayoutTests/ChangeLog:12 > > + * platform/mac/fast/table/rowindex-expected.txt: > > Looks like there are also many other rowindex-expected files: > > platform/efl/fast/table/rowindex-expected.png > platform/efl/fast/table/rowindex-expected.txt > platform/gtk/fast/table/rowindex-expected.png > platform/gtk/fast/table/rowindex-expected.txt > platform/ios-simulator/fast/table/rowindex-expected.txt > platform/ios-simulator-wk2/fast/table/rowindex-expected.txt > platform/mac/fast/table/rowindex-expected.png > platform/win/fast/table/rowindex-expected.txt > > What should we do about all of those!? I will rebaseline them. (In reply to comment #16) > (In reply to comment #15) > > Comment on attachment 261714 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=261714&action=review > > > > > LayoutTests/ChangeLog:12 > > > + * platform/mac/fast/table/rowindex-expected.txt: > > > > Looks like there are also many other rowindex-expected files: > > > > platform/efl/fast/table/rowindex-expected.png > > platform/efl/fast/table/rowindex-expected.txt > > platform/gtk/fast/table/rowindex-expected.png > > platform/gtk/fast/table/rowindex-expected.txt > > platform/ios-simulator/fast/table/rowindex-expected.txt > > platform/ios-simulator-wk2/fast/table/rowindex-expected.txt > > platform/mac/fast/table/rowindex-expected.png > > platform/win/fast/table/rowindex-expected.txt > > > > What should we do about all of those!? > > I will rebaseline them. <http://trac.webkit.org/changeset/190116> |