Comment on attachment 106812[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=106812&action=review
As this is adding bindings code which is easily testable, please include the tests which test the things you are adding in this patch.
> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:634
> +#if ENABLE(VIDEO_TRACK)
> +JSValue JSDOMWindow::textTrackCue(ExecState* exec) const
> +{
> + return getDOMConstructor<JSTextTrackCueConstructor>(exec, this);
> +}
> +#endif
This does not need to be custom.
> Source/WebCore/bindings/js/JSHTMLMediaElementCustom.cpp:52
> + MarkedArgumentBuffer list;
> + for (size_t i = 0; i < textTracks->size(); i++)
> + list.append(toJS(exec, globalObject(), (*textTracks)[i].get()));
> + return constructArray(exec, list);
This will return a normal array, which is mutable. The spec says this should be "a platform array object for objects of type TextTrack that is fixed length and read only". It also says "The same object must be returned each time the attribute is accessed", which this does not.
> Source/WebCore/bindings/js/JSTextTrackCueCustom.cpp:50
> + RefPtr<TextTrackCue> cue;
This variable is not used until much farther down and is not used in the outer scope.
> Source/WebCore/bindings/js/JSTextTrackCueCustom.cpp:52
> + if (exec->argumentCount()) {
This would probably be cleaner as an early return.
> Source/WebCore/bindings/js/JSWorkerContextCustom.cpp:115
> +JSValue JSWorkerContext::textTrackCue(ExecState* exec) const
> +{
> + return getDOMConstructor<JSTextTrackCueConstructor>(exec, this);
> +}
> +#endif
Is this code meant to work with Workers?
> Source/WebCore/page/DOMWindow.idl:606
> + attribute [JSCCustomGetter] TextTrackCueConstructor TextTrackCue; // Usable with the new operator
There is no reason to mark this Custom. The Constructor suffix should be enough.
Comment on attachment 106812[details]
Patch
Thanks for the review, Sam!
View in context: https://bugs.webkit.org/attachment.cgi?id=106812&action=review>> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:634
>> +#endif
>
> This does not need to be custom.
You are right. Thanks!
>> Source/WebCore/bindings/js/JSHTMLMediaElementCustom.cpp:52
>> + return constructArray(exec, list);
>
> This will return a normal array, which is mutable. The spec says this should be "a platform array object for objects of type TextTrack that is fixed length and read only". It also says "The same object must be returned each time the attribute is accessed", which this does not.
Yes, this is true. I'm removing this from this patch for now until I have a better understanding of how it should be implemented.
>> Source/WebCore/bindings/js/JSTextTrackCueCustom.cpp:50
>> + RefPtr<TextTrackCue> cue;
>
> This variable is not used until much farther down and is not used in the outer scope.
Done. Moved down.
>> Source/WebCore/bindings/js/JSTextTrackCueCustom.cpp:52
>> + if (exec->argumentCount()) {
>
> This would probably be cleaner as an early return.
Done.
>> Source/WebCore/bindings/js/JSWorkerContextCustom.cpp:115
>> +#endif
>
> Is this code meant to work with Workers?
No, sorry for the snafu. Gone now.
>> Source/WebCore/page/DOMWindow.idl:606
>> + attribute [JSCCustomGetter] TextTrackCueConstructor TextTrackCue; // Usable with the new operator
>
> There is no reason to mark this Custom. The Constructor suffix should be enough.
Done.
Created attachment 107572[details]
addressing Sam's comment and dropping textTracks attribute
I decided to drop the textTracks attribute in HTMLMediaElement from this patch because I am unsure how to handle the 'platform array object' in the spec (and haven't received concrete answers about it) and there is some chance this aspect of the spec may change soon anywase.
Created attachment 108958[details]
changing TextTrack::mode from unsigned short to unsigned int
For some reason, when the gtk build auto generates WebKitDOMTextTrack.cpp, it uses g_value_get_ushort() for the mode property. As far as I can tell, g_value_get_ushort doesn't exist in <glib-object.h> or any other current resource I could find. So, I have changed 'mode' to be an unsigned int instead of an unsigned short. I am unsure if this is an appropriate solution, please advise.
Created attachment 109074[details]
changing unsigned short conversion in CodeGeneratorGObject.pm
Perhaps a better solution is to not allow CodeGeneratorGObject.pm to generate the non-existent g_value_get_ushort() function. I've changed it to instead generate g_value_get_uint(), which does exist, when it sees the type 'unsigned short'. I think this is a ligit change because it already converts 'short' to 'g_value_get_int()', so converting unsigned short to uint seems reasonable.
Alright, I think this patch is finally ready for review!
Summary of changes for ease of reviewing:
Original intent: add MutableTextTrack.idl, TextTrack.idl, TextTrackCue.idl, and TextTrackCueList.idl as well as track-related JavaScript accessors for HTMLMediaElement and HTMLTrackElement.
Changes needed to make this change:
The gtk build uses the writable 'attribute unsigned short mode' from TextTrack.idl to generate a non-existent function g_value_get_ushort() in CodeGeneratorGObject.pm. I've changed it to instead generate g_value_get_uint(), which does exist, when it sees the 'unsigned short' type. I think this is a ligit change because it already converts 'short' to 'int', so converting 'unsigned short' to 'uint' seems reasonable.
Thanks!
Comment on attachment 110220[details]
updating with EnabledAtRuntime
View in context: https://bugs.webkit.org/attachment.cgi?id=110220&action=review> Source/WebCore/bindings/js/JSTextTrackCueCustom.cpp:39
> +EncodedJSValue JSC_HOST_CALL JSTextTrackCueConstructor::constructJSTextTrackCue(ExecState* exec)
> +{
Do we really need a custom constructor? We can create fancy constructors in IDL now.
> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:245
> - "unsigned short", "ushort");
> + "unsigned short", "uint");
Really?
> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2874
> return 1 if $type eq "IDBTransaction";
> return 1 if $type eq "FileReader";
> return 1 if $type eq "FileWriter";
> + return 1 if $type eq "TextTrackCue";
We really shouldn't be adding more things to this list. We should get this information from the ActiveDOMObject IDL attribute.
> Source/WebCore/html/TextTrackCue.idl:39
> + Conditional=VIDEO_TRACK,
> + EnabledAtRuntime=webkitVideoTrack,
> + CanBeConstructed,
> + CustomConstructFunction,
> + ConstructorParameters=6,
> + V8CustomConstructor
This doesn't have the ActiveDOMObject attribute even though you hard coded it into the code generator. Are you sure this object needs to be an activeDOMobject?
Comment on attachment 110220[details]
updating with EnabledAtRuntime
View in context: https://bugs.webkit.org/attachment.cgi?id=110220&action=review
Thanks abarth for the review. I've created a couple of separate patches to clean this one up and once those land, I'll put up a new patch here.
https://bugs.webkit.org/show_bug.cgi?id=70267https://bugs.webkit.org/show_bug.cgi?id=70268>> Source/WebCore/bindings/js/JSTextTrackCueCustom.cpp:39
>> +{
>
> Do we really need a custom constructor? We can create fancy constructors in IDL now.
Yes and no. You are right, we don't need a custom constructor for V8, but we still need one for JS. Thanks for pointing me to the new fancy IDL constructors options. That will make this next patch much cleaner!
>> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:245
>> + "unsigned short", "uint");
>
> Really?
Yep. Unfortunately, GetGValueTypeName() in this CodeGenerator is used when encountering the writable 'attribute unsigned short mode' in TextTrack.idl. This results in g_value_get_ushort() being generated (line 386), which is a function that does not exist! I've created a separate patch to address the issue: https://bugs.webkit.org/show_bug.cgi?id=70267>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2874
>> + return 1 if $type eq "TextTrackCue";
>
> We really shouldn't be adding more things to this list. We should get this information from the ActiveDOMObject IDL attribute.
Good call, I'll remove it.
>> Source/WebCore/html/TextTrackCue.idl:39
>> + V8CustomConstructor
>
> This doesn't have the ActiveDOMObject attribute even though you hard coded it into the code generator. Are you sure this object needs to be an activeDOMobject?
Right again! I had originally thought it needed to be in order to have a scriptExecutionContext, but I now see the error in my ways :). Thanks!
Comment on attachment 111530[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=111530&action=review
Now that some other CodeGenerator* patches have landed and more auto-generation is possible, this is a much cleaner patch. Enjoy!
> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2853
> + return 0 if $interfaceName eq "TextTrack";
Note: This is needed due to the CustomToJS on TextTrack.idl. V8 can auto-generate this, but JS cannot yet.
Created attachment 111644[details]
removing JSTextTrackCueCustom.cpp
Neat! Looks like the Constructor as a function is now working in JSC and so [CustomConstructFunction] and JSTextTrackCueCustom.cpp are no longer needed.
Comment on attachment 111644[details]
removing JSTextTrackCueCustom.cpp
View in context: https://bugs.webkit.org/attachment.cgi?id=111644&action=review> Source/WebCore/ChangeLog:3
> + Adding all JS-facing access points for <track>.
Nit: this title isn't completely correct, the event handler and textTracks attributes (at least) are still missing.
> Source/WebCore/html/MutableTextTrack.idl:38
> + void addCue(in TextTrackCue cue);
> + void removeCue(in TextTrackCue cue);
These can both throw InvalidStateError.
Comment on attachment 111644[details]
removing JSTextTrackCueCustom.cpp
View in context: https://bugs.webkit.org/attachment.cgi?id=111644&action=review
Thanks for the review. New patch with your suggestions is on the way.
>> Source/WebCore/ChangeLog:3
>> + Adding all JS-facing access points for <track>.
>
> Nit: this title isn't completely correct, the event handler and textTracks attributes (at least) are still missing.
Good point. Changed.
>> Source/WebCore/html/MutableTextTrack.idl:38
>> + void removeCue(in TextTrackCue cue);
>
> These can both throw InvalidStateError.
Added. Thanks.
Comment on attachment 111644[details]
removing JSTextTrackCueCustom.cpp
View in context: https://bugs.webkit.org/attachment.cgi?id=111644&action=review> Source/WebCore/bindings/js/JSTextTrackCustom.cpp:34
> +using namespace JSC;
If you're using namespace JSC, you don't need all the JSC:: below.
> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2853
> + return 0 if $interfaceName eq "TextTrack";
This should be an IDL attribute. :)
> Source/WebCore/html/HTMLTrackElement.h:51
> + PassRefPtr<TextTrack> track() const;
This should probably return a raw pointer.
2011-09-08 16:19 PDT, Anna Cavender
2011-09-15 17:47 PDT, Anna Cavender
2011-09-15 17:59 PDT, Anna Cavender
2011-09-22 15:22 PDT, Anna Cavender
2011-09-23 16:57 PDT, Anna Cavender
2011-09-23 17:12 PDT, Anna Cavender
2011-09-26 13:57 PDT, Anna Cavender
2011-09-26 17:29 PDT, Anna Cavender
2011-09-27 14:59 PDT, Anna Cavender
2011-09-27 21:15 PDT, Anna Cavender
2011-09-28 14:06 PDT, Anna Cavender
2011-09-29 09:01 PDT, Anna Cavender
2011-10-07 14:49 PDT, Anna Cavender
2011-10-18 17:03 PDT, Anna Cavender
2011-10-19 10:39 PDT, Anna Cavender
2011-10-19 14:35 PDT, Anna Cavender
2011-10-19 14:39 PDT, Anna Cavender
2011-10-19 16:06 PDT, Anna Cavender