DataCue.data should be a copy of the input ArrayBuffer, not a pointer

• Component: Media
• Status: RESOLVED FIXED

[Brendan Long]

This will be clarified in the HTML spec:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=24687

Example:

    buf = new ArrayBuffer(1);
    view = new Uint8Array(buf);
    view[0] = 1;

    cue = new DataCue(1, 2, buf);
    console.log(new Uint8Array(cue.data)[0]); // 1

    view[0] = 0;
    console.log(new Uint8Array(cue.data)[0]); // should still be 1

[Brendan Long]

Created attachment 224366 [details]
Patch

[Eric Carlson]

Comment on attachment 224366 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=224366&action=review

> Source/WebCore/html/track/DataCue.cpp:37
> +DataCue::DataCue(ScriptExecutionContext& context, double start, double end, ArrayBuffer* data)

It doesn't look like "data" can ever be NULL, so this would be better as a const reference: const ArrayBuffer& data

> Source/WebCore/html/track/DataCue.h:41
> +    static PassRefPtr<DataCue> create(ScriptExecutionContext& context, double start, double end, ArrayBuffer* data)

Ditto.

> Source/WebCore/html/track/DataCue.h:50
> +    void setData(ArrayBuffer* data) { m_data = ArrayBuffer::create(data); }

Ditto.

> Source/WebCore/html/track/DataCue.h:54
> +    DataCue(ScriptExecutionContext&, double start, double end, ArrayBuffer*);

Ditto.

[Brendan Long]

(In reply to comment #2)
> It doesn't look like "data" can ever be NULL, so this would be better as a const reference: const ArrayBuffer& data

JSDataCue expects that argument to be a PassRefPtr<ArrayBuffer> or ArrayBuffer* though.

Also, if you pass a non-ArrayBuffer argument, it gets converted to 'null'. I probably need to handle that instead of crashing..

[Brendan Long]

Created attachment 224372 [details]
Patch

[Brendan Long]

(In reply to comment #4)
> Created an attachment (id=224372) [details]
> Patch

This one handles null values correctly. I wonder if it would be better to throw an exception, or return an empty ArrayBuffer. The spec doesn't really say:

http://www.w3.org/html/wg/drafts/html/CR/embedded-content-0.html#datacue

[Brendan Long]

(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=224372) [details] [details]
> > Patch
> 
> This one handles null values correctly. I wonder if it would be better to throw an exception, or return an empty ArrayBuffer. The spec doesn't really say:
> 
> http://www.w3.org/html/wg/drafts/html/CR/embedded-content-0.html#datacue

Since it doesn't end with '?', it seems like an empty ArrayBuffer or exception would be better. I'm reading the WebIDL spec right now to see if this is covered.

[Brendan Long]

Ah:

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

> An ECMAScript value V is converted to an IDL interface type value by running the following algorithm (where I is the interface):
>
>  1. If Type(V) is not Object, then throw a TypeError.
>  2. If V is a platform object that implements I, then return the IDL interface type value that represents a reference to that platform object.
>  3. If V is a user object that is considered to implement I according to the rules in section 4.7, then return the IDL interface type value that represents a reference to that user object.
>  4. Throw a TypeError.

So I need to make this throw a TypeError if the ArrayBuffer arguments are null or not ArrayBuffers. I wonder why the bindings don't do this automatically.

[Brendan Long]

Created attachment 224382 [details]
Patch

Ok, so now it throws a TypeError if you set .data to null or a non-ArrayBuffer.

[WebKit Commit Bot]

Comment on attachment 224382 [details]
Patch

Clearing flags on attachment: 224382

Committed r164227: <http://trac.webkit.org/changeset/164227>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.