882299 - [webvtt] Implement TextTrackCue::SetLine

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Marcus Saad (:msaad)]

-Validate argument SetLine

Comment 1

[Rick Eyre (:reyre)]

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.

Comment 2

[Caitlin Potter (:caitp)]

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.

Comment 3

[Cameron McCormack (:heycam)]

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.)

Comment 4

[Ralph Giles (:rillian)]

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.

Comment 5

[Cameron McCormack (:heycam)]

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

Comment 6

[Rick Eyre (:reyre)]

Thanks for fixing that and for letting us know Cameron.

Comment 7

[Marcus Saad (:msaad)]

Test cases for this bug will be added on bug903030.

Comment 8

[Marcus Saad (:msaad)]

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?

Comment 9

[Marcus Saad (:msaad)]

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

Comment 10

[Marcus Saad (:msaad)]

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.

Comment 11

[Rick Eyre (:reyre)]

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

Comment 12

[Rick Eyre (:reyre)]

Attachment: Bug 882299 - Implement VTTCue::Line r=smaug,rillian

Comment 13

[Rick Eyre (:reyre)]

Attachment: Bug 882299 - Implement VTTCue::Line r=smaug,rillian

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 15

[Rick Eyre (:reyre)]

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 16

[Ralph Giles (:rillian)]

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?

Comment 17

[Rick Eyre (:reyre)]

Attachment: v3: Implement VTTCue::Line r=smaug,rillian

(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

Comment 18

[Rick Eyre (:reyre)]

Try looks green.

Comment 19

[Rick Eyre (:reyre)]

Attachment: v4: Implement VTTCue::Line r=smaug,rillian

Rebasing against current. Carring forward r=rillian,smaug.

Please land bug 949642 before this.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6532bf066bb4

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6532bf066bb4

Comment 23

[Boris Zbarsky [:bzbarsky]]

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.

Comment 24

[Boris Zbarsky [:bzbarsky]]

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?

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 27

[Rick Eyre (:reyre)]

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.

Comment 28

[Boris Zbarsky [:bzbarsky]]

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.

Comment 29

[Rick Eyre (:reyre)]

(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 :).

Comment 30

[Rick Eyre (:reyre)]

Opened follow up bug 958540.

Comment 31

[Rick Eyre (:reyre)]

Attachment: v5: Implement VTTCue::Line r=smaug,rillian,bz

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

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8360456 [details] [diff] [review]
v5: Implement VTTCue::Line r=smaug,rillian,bz

r=me on the mLineIsAutoKeyword/mLineLong bits.

Comment 33

[Rick Eyre (:reyre)]

Thanks for review Boris.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=dc36222081e6

Comment 34

[Rick Eyre (:reyre)]

Looks green.

Comment 35

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cd28a857f88

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7cd28a857f88