Bug 138405

Summary:

splitText API does not match DOM specification

Product:

WebKit

Reporter:

Shivakumar J M <shiva.jm>

Component:

DOM

Assignee:

Shivakumar J M <shiva.jm>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, buildbot, cdumez, commit-queue, darin, gyuyoung.kim, koivisto, ossy, rniwa, ryuan.choi

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	darin: review-, darin: commit-queue-
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2	none
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion	none
Patch-Updated	none
Patch-Updated-Review2	none

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-	Details Formatted Diff Diff
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	Details Formatted Diff Diff
Patch-Updated-Review2 (5.77 KB, patch) 2014-11-05 21:34 PST, Shivakumar J M	no flags	Details Formatted Diff Diff
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.