-Validate argument SetLine

We also need to finish the implementation of this. It's currently commented out in the WebIDL and the stub code in TextTrackCue doesn't take into account that it can by a long or a keyword.

According to Ms2ger / bz, the webidl in the proposed bindings is actually not valid (can't have a union of enum and number because strings and numbers are indistinguishable). I've messaged heycam about this and will hopefully hear back at some point, it might mean trying to ask them to remove the "numbers and strings" are indistinguishable thing, somehow, or it might mean finding a way to get the webvtt DOM api fixed. I don't know! So I guess this bug is currently blocking on potato.

Numbers and strings will be distinguishable in the near future -- I'll make the edits to the spec next week. There have been a number of other requests for that functionality, and with the semi-recent changes to how overload resolution is done in Web IDL, it makes sense to allow them to coexist in a union. (Leaving needinfo on me to remind me to update the bug here when I've made the spec change.)

Cait, it's fine to remove yourself from bugs if you're not going to work on them, but closing them without appropriate resolution is inappropriate.

Primitive types and DOMString are now distinguishable in Web IDL; sorry for the delay in getting to that.

Thanks for fixing that and for letting us know Cameron.

Test cases for this bug will be added on bug903030.

I'm trying to get this forward, but I'm stuck at trying to get it recognize my union type, which is longorAutoKeyword, After changing the webidl, and trying to compile, I'm getting an error at Uniontypes.h which is created during compilation time. Can anyone light things up a little for me?

Clarifying things a little bit: The type to be declared is an union between long and an enum named AutoKeyword. Based on https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Union_types , the name should be LongOrAutoKeyword. Errors are:‘AutoKeyword’ was not declared in this scope and ‘AutoKeyword’ was not declared in this scope

Reassigned this to Rick. This bug has been sitting with me for too long and I could not get over it. Will be working on their tests.

Sounds good Marcus. I'll look into this soon. We need this implemented for full VTTCue support.

Comment on attachment 8348253 [details] [diff] [review] Bug 882299 - Implement VTTCue::Line r=smaug,rillian In the future could you create patches with -U 8 to get more context.

Thanks for review Olli. (In reply to Olli Pettay [:smaug] from comment #14) > In the future could you create patches with -U 8 to get more context. Yep, I can do that.

Comment on attachment 8348253 [details] [diff] [review] Bug 882299 - Implement VTTCue::Line r=smaug,rillian Review of attachment 8348253 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/TextTrackCue.h @@ +182,5 @@ > + } > + if (mLine.IsLong()) { > + mLine.SetAsAutoKeyword() = aLine.GetAsAutoKeyword(); > + mReset = true; > + } Do you not need to call CueChanged() here?

(In reply to Ralph Giles (:rillian) from comment #16) > Do you not need to call CueChanged() here? Yes. Good catch! Added. Carrying forward r=rillian,smaug Try: https://tbpl.mozilla.org/?tree=Try&rev=ce24c745dabd

Try looks green.

Rebasing against current. Carring forward r=rillian,smaug. Please land bug 949642 before this.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4e6be6bea5 because something from the push this was in introduced a new intermittent failure on Android: https://tbpl.mozilla.org/php/getParsedLog.php?id=32309229&tree=Mozilla-Inbound

When you go to reland this, note that the patch for bug 952062 requires that SetAs* only be called on a union which is not yet initialized. You may want to not use a WebIDL type to store things internally, or we may need to add public APIs on WebIDL union types to change their type or something... The code in this bug _was_ safe, but only because both int32_t and the AutoKeyword enum have trivial destructors, which is not true of union members in general.

Peter, how would you feel about adding a public Destroy() or DestroySelf() method to unions that does what the dtor does right now and sets type to eUninitialized? Alternately, we could make SetAs* destroy things as needed if the union is already initialized, but that adds some extra branches to the normal binding codepath for initializing unions.... Maybe what we need is actually separate unsafe APIs for binding code and safe ones for other C++ code?

Merged backout to mozilla-central https://hg.mozilla.org/mozilla-central/rev/7a4e6be6bea5

(In reply to Vacation Dec 19 to Jan 1 from comment #24) > Maybe what we need is actually > separate unsafe APIs for binding code and safe ones for other C++ code? This sounds most API user friendly, and least surprising behavior.

I'm thinking the simplest course of action right now is to not use the WebIDL type to store things internally. I'll go ahead and do that now if you agree Boris? I'm not sure what kind of unsafe APIs we would need and how to go about doing that. I'd be happy to take a crack at it if you pointed me in the right direction.

Rick, I wasn't expecting you to change the codegen to add the safe APIs... Please just file a bug on me to do that, and I suspect that using a different type for the internal storage here is simplest for now.

(In reply to Boris Zbarsky [:bz] from comment #28) > Rick, I wasn't expecting you to change the codegen to add the safe APIs... > Please just file a bug on me to do that, and I suspect that using a > different type for the internal storage here is simplest for now. Sounds good Boris :).

Opened follow up bug 958540.

Switched to not using the UnionType to store the value internally in TextTrackCue. Carrying forward r=smaug,rillian.

Comment on attachment 8360456 [details] [diff] [review] v5: Implement VTTCue::Line r=smaug,rillian,bz r=me on the mLineIsAutoKeyword/mLineLong bits.

Thanks for review Boris. Try push: https://tbpl.mozilla.org/?tree=Try&rev=dc36222081e6

Looks green.