RESOLVED FIXED 138405
splitText API does not match DOM specification 
https://bugs.webkit.org/show_bug.cgi?id=138405
Summary splitText API does not match DOM specification
Shivakumar J M
Reported 2014-11-04 21:50:35 PST
splitText API does not match as per DOM specification, as per latest Spec: https://w3c.github.io/dom/#interface-text, change splitText() argument to mandatory.
Attachments
Patch (3.48 KB, patch)
2014-11-05 01:44 PST, Shivakumar J M 		darin: review-
darin: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (651.65 KB, application/zip)
2014-11-05 03:12 PST, Build Bot 		no flags
Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (507.69 KB, application/zip)
2014-11-05 03:26 PST, Build Bot 		no flags
Details
Patch-Updated (5.35 KB, patch)
2014-11-05 19:48 PST, Shivakumar J M 		no flags
DetailsFormatted DiffDiff
Patch-Updated-Review2 (5.77 KB, patch)
2014-11-05 21:34 PST, Shivakumar J M 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Shivakumar J M
Comment 1 2014-11-05 01:44:22 PST
Created attachment 241013 [details] Patch splitText API does not match as per DOM specification, modifed as per spec and added new tests
Build Bot
Comment 2 2014-11-05 03:12:38 PST
Created attachment 241019 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Build Bot
Comment 3 2014-11-05 03:26:46 PST
Created attachment 241021 [details] Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Darin Adler
Comment 4 2014-11-05 09:07:30 PST
Comment on attachment 241013 [details] Patch The test fast/dom/non-numeric-values-numeric-parameters.html, or at least its expected results, has to be changed, too.
Shivakumar J M
Comment 5 2014-11-05 19:48:19 PST
Created attachment 241083 [details] Patch-Updated Patch updated with test failures.
Shivakumar J M
Comment 6 2014-11-05 19:49:55 PST
(In reply to comment #4) > Comment on attachment 241013 [details] > Patch > > The test fast/dom/non-numeric-values-numeric-parameters.html, or at least > its expected results, has to be changed, too. Dear Darin, Thanks, updated the patch with test results.
Chris Dumez
Comment 7 2014-11-05 20:02:57 PST
Comment on attachment 241083 [details] Patch-Updated View in context: https://bugs.webkit.org/attachment.cgi?id=241083&action=review The web-exposed behavior change seems fine as it matches the specification and the behavior other major browsers but please take into consideration my comments. > Source/WebCore/ChangeLog:7 > + This matches the behavior of both Firefox 33 and Chrome 38. Please state so in your Changelog as this is more important than matching the specification. Also, please indicate in the Changelog which web-exposed changes you are making: 1. Make the offset argument mandatory (and thus throw if it is omitted) 2. No longer throw if the offset argument is negative (and wraps to a valid index) > LayoutTests/fast/dom/Text/splitText.html:14 > +shouldThrow('text.splitText()'); Please specify the second argument to shouldThrow (exception name) > LayoutTests/fast/dom/Text/splitText.html:15 > +shouldThrow('text.splitText(999)'); ditto. > LayoutTests/fast/dom/Text/splitText.html:16 > +shouldThrow('text.splitText(-1)'); ditto.
Shivakumar J M
Comment 8 2014-11-05 21:34:31 PST
Created attachment 241085 [details] Patch-Updated-Review2 Updated change log and splitText.html test file sa per comments.
Chris Dumez
Comment 9 2014-11-05 21:37:55 PST
Comment on attachment 241085 [details] Patch-Updated-Review2 r=me, thanks.
WebKit Commit Bot
Comment 10 2014-11-05 23:52:56 PST
Comment on attachment 241085 [details] Patch-Updated-Review2 Clearing flags on attachment: 241085 Committed r175688: <http://trac.webkit.org/changeset/175688>
WebKit Commit Bot
Comment 11 2014-11-05 23:53:01 PST
All reviewed patches have been landed. Closing bug.
Chris Dumez
Comment 12 2014-11-06 00:02:41 PST
Comment on attachment 241085 [details] Patch-Updated-Review2 View in context: https://bugs.webkit.org/attachment.cgi?id=241085&action=review I'll fix these nits in a follow-up patch as I missed them during my earlier review. > LayoutTests/fast/dom/Text/splitText.html:1 > +<html> Missing Doctype. > LayoutTests/fast/dom/Text/splitText.html:5 > +<head> 2 heads?
Chris Dumez
Comment 13 2014-11-06 00:07:29 PST
(In reply to comment #12) > Comment on attachment 241085 [details] > Patch-Updated-Review2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241085&action=review > > I'll fix these nits in a follow-up patch as I missed them during my earlier > review. > > > LayoutTests/fast/dom/Text/splitText.html:1 > > +<html> > > Missing Doctype. > > > LayoutTests/fast/dom/Text/splitText.html:5 > > +<head> > > 2 heads? Fixed nits in <http://trac.webkit.org/changeset/175689>.
Shivakumar J M
Comment 14 2014-11-06 00:36:40 PST
(In reply to comment #13) > (In reply to comment #12) > > Comment on attachment 241085 [details] > > Patch-Updated-Review2 > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=241085&action=review > > > > I'll fix these nits in a follow-up patch as I missed them during my earlier > > review. > > > > > LayoutTests/fast/dom/Text/splitText.html:1 > > > +<html> > > > > Missing Doctype. > > > > > LayoutTests/fast/dom/Text/splitText.html:5 > > > +<head> > > > > 2 heads? > > Fixed nits in <http://trac.webkit.org/changeset/175689>. Dear Chris, Thanks, for adding patch, which i missed.
Note You need to log in before you can comment on or make changes to this bug.