Make CharacterData::insertData() / deleteData() / replaceData() arguments mandatory

Make CharacterData::insertData() / deleteData() / replaceData() arguments mandatory

As per the specification, CharacterData::insertData() / deleteData() / replaceData() arguments
should be mandatory:
http://dom.spec.whatwg.org/#characterdata

This behavior is consistent with IE10 and Firefox.

BUG=266813
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155345

[do-not-use, 2013-08-01 12:42:10]

https://codereview.chromium.org/21542002/diff/1/Source/core/dom/CharacterData...
File Source/core/dom/CharacterData.idl (right):

https://codereview.chromium.org/21542002/diff/1/Source/core/dom/CharacterData...
Source/core/dom/CharacterData.idl:29: [RaisesException] void
insertData([IsIndex] unsigned long offset, DOMString data);
Note that I kept the [IsIndex] extended attributes even though this is no longer
according to spec:
http://dom.spec.whatwg.org/#dom-characterdata-replacedata

This is out of scope for this patch but I would like to remove them in a
follow-up patch.

Since the type is unsigned long, passing a negative value should simply get
converted using toUint32() as per the Web IDL specification (step 4 in
http://dev.w3.org/2006/webapi/WebIDL/#es-unsigned-long).

The [IsIndex] extended attribute should probably completely disappear in the
near future.

[haraken, 2013-08-01 12:48:16]

LGTM. Let's wait for an approval from an API owner.

Regarding [IsIndex], I agree with removing it. Also see this bug:
https://code.google.com/p/chromium/issues/detail?id=237739&thanks=237739&ts=1...

[tkent, 2013-08-01 13:08:05]

lgtm as API owner.

[do-not-use, 2013-08-01 13:31:40]

On 2013/08/01 12:48:16, haraken wrote:
> LGTM. Let's wait for an approval from an API owner.
> 
> Regarding [IsIndex], I agree with removing it. Also see this bug:
>
https://code.google.com/p/chromium/issues/detail?id=237739&thanks=237739&ts=1...

haraken, FYI, it looks like we have a bug in the generated bindings which
renders [IsIndex] useless :(

Here is the generated code in V8CharacterData::insertDataMethod():
    V8TRYCATCH_VOID(int, offset, toUInt32(args[0]));
    if (UNLIKELY(offset < 0)) {
        setDOMException(IndexSizeError, args.GetIsolate());
        return;
    }

offset cannot be negative since we already called toUInt32() on it. This may be
a recent regression? Note that this IDL file still uses the old perl script.
Is it worth fixing since we want to remove [IsIndex]? (I haven't check to see if
there is any remaining valid use of [IsIndex]).

Note that it throws in my example not because of [IsIndex] but because -1 wraps
to 4294967295, which is greater than the data length. It does not throw if I do
text.insertData(-4294967294, 'test') in my test (wraps to a valid offset: 2).

[haraken, 2013-08-01 13:35:08]

On 2013/08/01 13:31:40, Christophe Dumez wrote:
> On 2013/08/01 12:48:16, haraken wrote:
> > LGTM. Let's wait for an approval from an API owner.
> > 
> > Regarding [IsIndex], I agree with removing it. Also see this bug:
> >
>
https://code.google.com/p/chromium/issues/detail?id=237739&thanks=237739&ts=1...
> 
> haraken, FYI, it looks like we have a bug in the generated bindings which
> renders [IsIndex] useless :(
> 
> Here is the generated code in V8CharacterData::insertDataMethod():
>     V8TRYCATCH_VOID(int, offset, toUInt32(args[0]));
>     if (UNLIKELY(offset < 0)) {
>         setDOMException(IndexSizeError, args.GetIsolate());
>         return;
>     }
> 
> offset cannot be negative since we already called toUInt32() on it. This may
be
> a recent regression? Note that this IDL file still uses the old perl script.
> Is it worth fixing since we want to remove [IsIndex]? (I haven't check to see
if
> there is any remaining valid use of [IsIndex]).
> 
> Note that it throws in my example not because of [IsIndex] but because -1
wraps
> to 4294967295, which is greater than the data length. It does not throw if I
do
> text.insertData(-4294967294, 'test') in my test (wraps to a valid offset: 2).

oh... as far as I tested four months ago, it was throwing exceptions. Would you
bisect where the regression happens? If it's old enough, it might make sense to
keep the current (and correct) behavior.

Either way, we have to add test cases for the behavior.

[arv (Not doing code reviews), 2013-08-01 13:47:44]

LGTM

https://codereview.chromium.org/21542002/diff/6001/LayoutTests/fast/dom/chara...
File LayoutTests/fast/dom/characterdata-api-arguments.html (right):

https://codereview.chromium.org/21542002/diff/6001/LayoutTests/fast/dom/chara...
LayoutTests/fast/dom/characterdata-api-arguments.html:18:
shouldThrow("text.appendData()", "'TypeError: Not enough arguments'"); //
Missing argument.
useless comment

[do-not-use, 2013-08-01 14:14:10]

On 2013/08/01 13:35:08, haraken wrote:
> On 2013/08/01 13:31:40, Christophe Dumez wrote:
> > On 2013/08/01 12:48:16, haraken wrote:
> > > LGTM. Let's wait for an approval from an API owner.
> > > 
> > > Regarding [IsIndex], I agree with removing it. Also see this bug:
> > >
> >
>
https://code.google.com/p/chromium/issues/detail?id=237739&thanks=237739&ts=1...
> > 
> > haraken, FYI, it looks like we have a bug in the generated bindings which
> > renders [IsIndex] useless :(
> > 
> > Here is the generated code in V8CharacterData::insertDataMethod():
> >     V8TRYCATCH_VOID(int, offset, toUInt32(args[0]));
> >     if (UNLIKELY(offset < 0)) {
> >         setDOMException(IndexSizeError, args.GetIsolate());
> >         return;
> >     }
> > 
> > offset cannot be negative since we already called toUInt32() on it. This may
> be
> > a recent regression? Note that this IDL file still uses the old perl script.
> > Is it worth fixing since we want to remove [IsIndex]? (I haven't check to
see
> if
> > there is any remaining valid use of [IsIndex]).
> > 
> > Note that it throws in my example not because of [IsIndex] but because -1
> wraps
> > to 4294967295, which is greater than the data length. It does not throw if I
> do
> > text.insertData(-4294967294, 'test') in my test (wraps to a valid offset:
2).
> 
> oh... as far as I tested four months ago, it was throwing exceptions. Would
you
> bisect where the regression happens? If it's old enough, it might make sense
to
> keep the current (and correct) behavior.
> 
> Either way, we have to add test cases for the behavior.

Well, [IsIndex] is covered by bindings tests but it seems the problem has been
there for a very long time. I do not understand yet why it worked for you 4
months ago.

See for e.g. this commit from 19 months ago:
http://trac.webkit.org/changeset/104526

It added a bindings test for [IsIndex] to
bindings/scripts/test/TestEventTarget.idl. If you look at the generated code:
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/test/V8/...

in particular itemCallback():
53	    EXCEPTION_BLOCK(int, index, toUInt32(MAYBE_MISSING_PARAMETER(args, 0,
MissingIsUndefined)));
54	    if (UNLIKELY(index < 0)) {
55	        ec = INDEX_SIZE_ERR;
56	        goto fail;
57	    }

So it was already calling toUInt32 19 months ago according to this.

[arv (Not doing code reviews), 2013-08-01 14:33:24]

FWIW, IsIndex is not part of WebIDL so we might as well just remove it.

Also, the following only throws if ToNumber throws [1]

void foo(unsigned long bar);

http://www.w3.org/TR/WebIDL/#es-unsigned-long

[1] ToNumber calls ToPrimitive which may throw if valueOf/toString throws
or the object doesn't have valueOf or toString


On Thu, Aug 1, 2013 at 10:14 AM, <ch.dumez@sisa.samsung.com> wrote:

> On 2013/08/01 13:35:08, haraken wrote:
>
>> On 2013/08/01 13:31:40, Christophe Dumez wrote:
>> > On 2013/08/01 12:48:16, haraken wrote:
>> > > LGTM. Let's wait for an approval from an API owner.
>> > >
>> > > Regarding [IsIndex], I agree with removing it. Also see this bug:
>> > >
>> >
>>
>
> https://code.google.com/p/**chromium/issues/detail?id=**
>
237739&thanks=237739&ts=**1367566666<https://code.google.com/p/chromium/issues/detail?id=237739&thanks=237739&ts=1367566666>
>
>> >
>> > haraken, FYI, it looks like we have a bug in the generated bindings
>> which
>> > renders [IsIndex] useless :(
>> >
>> > Here is the generated code in V8CharacterData::**insertDataMethod():
>> >     V8TRYCATCH_VOID(int, offset, toUInt32(args[0]));
>> >     if (UNLIKELY(offset < 0)) {
>> >         setDOMException(**IndexSizeError, args.GetIsolate());
>> >         return;
>> >     }
>> >
>> > offset cannot be negative since we already called toUInt32() on it.
>> This may
>> be
>> > a recent regression? Note that this IDL file still uses the old perl
>> script.
>> > Is it worth fixing since we want to remove [IsIndex]? (I haven't check
>> to
>>
> see
>
>> if
>> > there is any remaining valid use of [IsIndex]).
>> >
>> > Note that it throws in my example not because of [IsIndex] but because
>> -1
>> wraps
>> > to 4294967295, which is greater than the data length. It does not throw
>> if I
>> do
>> > text.insertData(-4294967294, 'test') in my test (wraps to a valid
>> offset:
>>
> 2).
>
>  oh... as far as I tested four months ago, it was throwing exceptions.
>> Would
>>
> you
>
>> bisect where the regression happens? If it's old enough, it might make
>> sense
>>
> to
>
>> keep the current (and correct) behavior.
>>
>
>  Either way, we have to add test cases for the behavior.
>>
>
> Well, [IsIndex] is covered by bindings tests but it seems the problem has
> been
> there for a very long time. I do not understand yet why it worked for you 4
> months ago.
>
> See for e.g. this commit from 19 months ago:
>
http://trac.webkit.org/**changeset/104526<http://trac.webkit.org/changeset/10...
>
> It added a bindings test for [IsIndex] to
> bindings/scripts/test/**TestEventTarget.idl. If you look at the generated
> code:
> http://trac.webkit.org/**browser/trunk/Source/WebCore/**
>
bindings/scripts/test/V8/**V8TestEventTarget.cpp?rev=**104526<http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/test/V8/V8TestEventTarget.cpp?rev=104526>
>
> in particular itemCallback():
> 53          EXCEPTION_BLOCK(int, index,
toUInt32(MAYBE_MISSING_**PARAMETER(args,
> 0,
> MissingIsUndefined)));
> 54          if (UNLIKELY(index < 0)) {
> 55              ec = INDEX_SIZE_ERR;
> 56              goto fail;
> 57          }
>
> So it was already calling toUInt32 19 months ago according to this.
>
>
https://codereview.chromium.**org/21542002/<https://codereview.chromium.org/2...
>



-- 
erik

[haraken, 2013-08-01 15:21:52]

Sorry about the confusion (I couldn't well remember what I was doing 4 months
ago). Then it would make sense to remove [IsIndex] with test cases.

[do-not-use, 2013-08-01 15:25:19]

So the generated code looks like:
    V8TRYCATCH_VOID(int, offset, toUInt32(args[0]));
    if (UNLIKELY(offset < 0)) {
        setDOMException(IndexSizeError, args.GetIsolate());
        return;
    }

I believe I found why this sometimes throws.

Say you pass -1, toUInt32(-1) gets called and it returns 4294967295.
Then we store the return value in an int but since the value does not fit in an
int, we wrap and we end up with a negative value :)

[haraken, 2013-08-01 15:36:49]

On 2013/08/01 15:25:19, Christophe Dumez wrote:
> So the generated code looks like:
>     V8TRYCATCH_VOID(int, offset, toUInt32(args[0]));
>     if (UNLIKELY(offset < 0)) {
>         setDOMException(IndexSizeError, args.GetIsolate());
>         return;
>     }
> 
> I believe I found why this sometimes throws.
> 
> Say you pass -1, toUInt32(-1) gets called and it returns 4294967295.
> Then we store the return value in an int but since the value does not fit in
an
> int, we wrap and we end up with a negative value :)

Good point. It looks like we might want to move this discussion to a separate
thread.

[do-not-use, 2013-08-01 15:39:07]

On 2013/08/01 15:36:49, haraken wrote:
> On 2013/08/01 15:25:19, Christophe Dumez wrote:
> > So the generated code looks like:
> >     V8TRYCATCH_VOID(int, offset, toUInt32(args[0]));
> >     if (UNLIKELY(offset < 0)) {
> >         setDOMException(IndexSizeError, args.GetIsolate());
> >         return;
> >     }
> > 
> > I believe I found why this sometimes throws.
> > 
> > Say you pass -1, toUInt32(-1) gets called and it returns 4294967295.
> > Then we store the return value in an int but since the value does not fit in
> an
> > int, we wrap and we end up with a negative value :)
> 
> Good point. It looks like we might want to move this discussion to a separate
> thread.

Agreed, I posted my comment on
https://code.google.com/p/chromium/issues/detail?id=237739

I believe we can land this CL as it is and take care of [IsIndex] later.

[haraken, 2013-08-01 15:44:34]

> I believe we can land this CL as it is and take care of [IsIndex] later.

Yes.

[commit-bot: I haz the power, 2013-08-01 15:47:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/21542002/22002

[commit-bot: I haz the power, 2013-08-01 17:23:50]

Change committed as 155345