Bug 138405

Summary: splitText API does not match DOM specification
Product: WebKit Reporter: Shivakumar J M <shiva.jm>
Component: DOMAssignee: 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: P2    
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

Description Shivakumar J M 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.
Comment 1 Shivakumar J M 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
Comment 2 Build Bot 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
Comment 3 Build Bot 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
Comment 4 Darin Adler 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.
Comment 5 Shivakumar J M 2014-11-05 19:48:19 PST
Created attachment 241083 [details]
Patch-Updated

Patch updated with test failures.
Comment 6 Shivakumar J M 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.
Comment 7 Chris Dumez 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.
Comment 8 Shivakumar J M 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.
Comment 9 Chris Dumez 2014-11-05 21:37:55 PST
Comment on attachment 241085 [details]
Patch-Updated-Review2

r=me, thanks.
Comment 10 WebKit Commit Bot 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>
Comment 11 WebKit Commit Bot 2014-11-05 23:53:01 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Chris Dumez 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?
Comment 13 Chris Dumez 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>.
Comment 14 Shivakumar J M 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.