Created attachment 106812 [details] Patch

Comment on attachment 106812 [details] Patch Attachment 106812 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9628206

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 107575 [details] updating patch, that last one didn't apply

Comment on attachment 107575 [details] updating patch, that last one didn't apply Attachment 107575 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9685810

Created attachment 108410 [details] attempt to fix gtk build

Comment on attachment 108410 [details] attempt to fix gtk build Attachment 108410 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9811039

Created attachment 108561 [details] adding JSTextTrackCustom.cpp

Comment on attachment 108561 [details] adding JSTextTrackCustom.cpp Attachment 108561 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9844051

Created attachment 108562 [details] adding JSTextTrackCustom.cpp, fix typo

Comment on attachment 108562 [details] adding JSTextTrackCustom.cpp, fix typo Attachment 108562 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9831308

Comment on attachment 108562 [details] adding JSTextTrackCustom.cpp, fix typo Attachment 108562 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9839307

Created attachment 108718 [details] another attempt to fix gtk build

Comment on attachment 108718 [details] another attempt to fix gtk build Attachment 108718 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9880079

Created attachment 108762 [details] another try at gtk build fix

Comment on attachment 108762 [details] another try at gtk build fix Attachment 108762 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9877228

Created attachment 108905 [details] another attempt to fix gtk build

Comment on attachment 108905 [details] another attempt to fix gtk build Attachment 108905 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9882448

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.

Comment on attachment 108958 [details] changing TextTrack::mode from unsigned short to unsigned int Attachment 108958 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9888034

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.

Comment on attachment 109074 [details] changing unsigned short conversion in CodeGeneratorGObject.pm Attachment 109074 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9888423

Comment on attachment 109074 [details] changing unsigned short conversion in CodeGeneratorGObject.pm Attachment 109074 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9888421

Created attachment 109165 [details] Changing OFF->DISABLED per spec and updating WebKitDOMTestObj.cpp

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!

Created attachment 110220 [details] updating with EnabledAtRuntime

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=70267 https://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!

Created attachment 111530 [details] Patch

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.

Comment on attachment 111530 [details] Patch Attachment 111530 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10143299

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.

Created attachment 111676 [details] adding exceptions

Created attachment 111678 [details] fix little typo

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.

Comment on attachment 111678 [details] fix little typo Note nit above.

Thanks abarth! Landing now with your suggestions (except for the bit about the IDL attribute, that sounds like another patch altogether :) )

Created attachment 111689 [details] Patch for landing

Comment on attachment 111689 [details] Patch for landing Clearing flags on attachment: 111689 Committed r97926: <http://trac.webkit.org/changeset/97926>

All reviewed patches have been landed. Closing bug.