882743 - [webvtt] Update WebIDL of TextTrackCue

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Marcus Saad (:msaad)]

Update WebIDL of TextTrackCue with SetterThrows behavior on:

-Vertical [Throws SyntaxError]
-Position [Throws IndexSizeError]
-Size [Throws IndexSizeError]
-Align [Throws SyntaxError]

Comment 1

[Marcus Saad (:msaad)]

Attachment: Patch v1

- Added SetterThrows on WebIDL
- Validating and Throwing on the specified setters
- Changes on WebVTTLoadListener to pass a ErrorResult. I'm not checking for errors that might come back because WebVTTLoadListener::onParsedCue won't be dealing with wrong info at that time.

Comment 2

[Rick Eyre (:reyre)]

Comment on attachment 765156 [details] [diff] [review]
Patch v1

Review of attachment 765156 [details] [diff] [review]:
-----------------------------------------------------------------

Overall it's starting to look good to me :). I think it's fine to keep bug 868519 and this one in one if it's being hard for you to separate them out, like we discussed in our call yesterday.

What that would mean is for you to include those small changes you made in bug 868519 into this one, should be easy as your just deleting things, and then we can just comment on bug 868519 saying that it landed in this one.

::: content/media/TextTrackCue.h
@@ +135,5 @@
> +    if (!aVertical.EqualsLiteral("rl") && !aVertical.EqualsLiteral("lr") && !aVertical.IsEmpty()){
> +      aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> +      return;
> +    }
> +    

Some whitespace here.

@@ +196,5 @@
>      if (mSize == aSize){
>        return;
>      }
>  
> +    if (aSize > 100 && aSize < 0){

I think you want || here.

@@ +200,5 @@
> +    if (aSize > 100 && aSize < 0){
> +      aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
> +      return;
> +    }
> +    

Whitspace here.

@@ +215,5 @@
>    {
>      if (mAlign == aAlign)
>        return;
>  
> +    if (aAlign != TextTrackCueAlign::Start && aAlign != TextTrackCueAlign::Middle

I'm not sure if we need to validate align as it is an enum and we'll only ever receive acceptable values. However, this makes it confusing as in the spec align is actually a DOMString which makes sense to throw an error when setting if we don't get a value we like.

I think this may be fine for now though as the spec is currently in transition to making align and vertical both enums:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=22356

@@ +219,5 @@
> +    if (aAlign != TextTrackCueAlign::Start && aAlign != TextTrackCueAlign::Middle
> +      && aAlign != TextTrackCueAlign::End && aAlign != TextTrackCueAlign::Left 
> +      && aAlign != TextTrackCueAlign::Right){
> +     aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> +     return; 

Whitespace

@@ +221,5 @@
> +      && aAlign != TextTrackCueAlign::Right){
> +     aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> +     return; 
> +    }
> +    

Whitespace

::: content/media/WebVTTLoadListener.cpp
@@ +156,4 @@
>  void
>  WebVTTLoadListener::OnParsedCue(webvtt_cue* aCue)
>  {
> +  ErrorResult rv;

You should define this right before your about to use that as that's the preferred way to do things in Gecko.

@@ +160,2 @@
>    const char* text = webvtt_string_text(&aCue->body);
> +  

Whitespace here.

Comment 3

[Marcus Saad (:msaad)]

Attachment: Patch v2

Rick, I've addressed all of your comments. Hopefully I'm whitespace free this time hehe. Got a nice plugin working.

Sorry about the && and || mistake, that's totally lack of attention. Regarding whether or not we should keep the validation on align, I would suggest we follow what you said. We can wait until the spec is changed, open a new bug and them I'll work on it.

Thanks for the tip about Gecko way of coding, that's always useful to keep the code base uniform!

Comment 4

[Rick Eyre (:reyre)]

Comment on attachment 767282 [details] [diff] [review]
Patch v2

Review of attachment 767282 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

I'll pass this over to Ralph to get reviewed for checkin now.

Should we get it reviewed by a DOM peer as well Ralph?

Comment 5

[Rick Eyre (:reyre)]

> Sorry about the && and || mistake, that's totally lack of attention.
> Regarding whether or not we should keep the validation on align, I would
> suggest we follow what you said. We can wait until the spec is changed, open
> a new bug and them I'll work on it.
> 
> Thanks for the tip about Gecko way of coding, that's always useful to keep
> the code base uniform!

No worries about anything Marcus! Thanks for helping :)

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 767282 [details] [diff] [review]
Patch v2

Review of attachment 767282 [details] [diff] [review]:
-----------------------------------------------------------------

r=me as far at the motivation goes. Please respond on the nits below.

You should also write a commit message for your patch. The first line should be something like:

Bug 882743 - Make TextTrackCue setters throw. r=rillian,???

Then after a blank line include some motivation for the patch. I'd mention that you added validation and throws as required by the WebVTT api spec (with a link), but didn't validate any values where the spec didn't specify an exception. That makes it clear why you removed humph's XXX: validate? comments.

I would like a DOM peer to review this as well. In particular I don't know if it's safe for the setters to Throw() on the generic ErrorResult passed by the WebVTTLoadListener calls. It looks like it just tests mResult, which the caller ignores so there's a silent failure if the invalid cue setting came from the parser instead of js?

::: content/media/TextTrackCue.h
@@ +217,5 @@
>  
> +    if (aAlign != TextTrackCueAlign::Start && aAlign != TextTrackCueAlign::Middle
> +      && aAlign != TextTrackCueAlign::End && aAlign != TextTrackCueAlign::Left
> +      && aAlign != TextTrackCueAlign::Right){
> +      aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);

Is this possible? I guess it would catch bugs in the string-to-enum conversion in the binding code, but you should  be able to just range check against the first and last enum values.

@@ -218,4 @@
>  
>    void SetText(const nsAString& aText)
>    {
> -    // XXXhumph: validate? bug 868519.

Should bug 868519 be closed as a duplicate of this one?

Comment 7

[Caitlin Potter (:caitp)]

If there's a duplicate, this one is the duplicate, not bug 868519

Comment 8

[Marcus Saad (:msaad)]

Attachment: Patch v3

> I would like a DOM peer to review this as well. In particular I don't know
> if it's safe for the setters to Throw() on the generic ErrorResult passed by
> the WebVTTLoadListener calls. It looks like it just tests mResult, which the
> caller ignores so there's a silent failure if the invalid cue setting came
> from the parser instead of js?

Who should I ask for review on this? 

> You should also write a commit message for your patch. The first line should
> be something like:
> 
> Bug 882743 - Make TextTrackCue setters throw. r=rillian,???
> 
> Then after a blank line include some motivation for the patch. I'd mention
> that you added validation and throws as required by the WebVTT api spec
> (with a link), but didn't validate any values where the spec didn't specify
> an exception. That makes it clear why you removed humph's XXX: validate?
> comments.

I hope the patch message is somewhat close to what you suggested.

> ::: content/media/TextTrackCue.h
> @@ +217,5 @@
> >  
> > +    if (aAlign != TextTrackCueAlign::Start && aAlign != TextTrackCueAlign::Middle
> > +      && aAlign != TextTrackCueAlign::End && aAlign != TextTrackCueAlign::Left
> > +      && aAlign != TextTrackCueAlign::Right){
> > +      aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> 
> Is this possible? I guess it would catch bugs in the string-to-enum
> conversion in the binding code, but you should  be able to just range check
> against the first and last enum values.

reyre and I agreed to remove this validation, since the value is being passed as a TextTrackCueAlign always. Moreover, there is a bug opened against the spec to change it to an Enum, which should remove the throwing behavior from the IDL.

> @@ -218,4 @@
> >  
> >    void SetText(const nsAString& aText)
> >    {
> > -    // XXXhumph: validate? bug 868519.
> 
> Should bug 868519 be closed as a duplicate of this one?

Bug 868519 led to changes made on this bug. Since both were touching the same code, reyre and I decided to squash them together into a single one. Given that this bug includes bigger changes and touches more code, we decided to select this one to carry all the changes.

Comment 9

[Marcus Saad (:msaad)]

Comment on attachment 771692 [details] [diff] [review]
Patch v3

Rillian said :
> I would like a DOM peer to review this as well. In particular I don't know
> if it's safe for the setters to Throw() on the generic ErrorResult passed by
> the WebVTTLoadListener calls. It looks like it just tests mResult, which the
> caller ignores so there's a silent failure if the invalid cue setting came
> from the parser instead of js?

bz, I would kindly ask your review on this short change. Shouldn't take more than a couple minutes to review it. 

Thanks

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 771692 [details] [diff] [review]
Patch v3

Yeah, this is just silently swallowing the exceptions from the setters.

Are we by any chance guaranteed that the parser will not produce invalid data?  That is, can we assert that !rv.Failed() after the calls from the parser?

>+    void SetSize(int32_t aSize, ErrorResult& aRv)

Please fix the indent.

Comment 11

[Ralph Giles (:rillian)]

Asserting would be fine. The parse can fail if the source data is invalid, in which case the cue is silently dropped, but returning invalid cue setting after parse would be a bug.

Comment 12

[Marcus Saad (:msaad)]

Attachment: Patch v4

Included MOZ_ASSERT(!rv.Fail()) and fixed formating.

What is the next step to have this landed?!

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 774227 [details] [diff] [review]
Patch v4

This presumably doesn't compile, since there is no ErrorResult::Fail.  I'd appreciate only being asked for review on patches that _might_ work.  ;)

You want Failed(), and you want to assert after every call there that takes an ErrorResult.

The next steps are probably:

1)  Fixing this to compile.
2)  Adding tests for this.
3)  Asking for review.
4)  Once review happens, adding the checkin-needed keyword to the bug.

Comment 15

[Marcus Saad (:msaad)]

Attachment: Patch v5

I would have to say that you presumed wrongly, because normally, when I'm submitting a patch, I rebase to the newest code and compile. I wouldn't waste your precious time with code that doesn't work, that's not my intention here.

I've done the appropriate changes to the previous patch, including MOZ_ASSERTs after every call passing rv, and changed it do Failed() instead of Fail(). 

It's sad when we lose contributors because people make quick judgements and I'm glad that won't happen here because I'm resilient to such knee-jerk criticisms.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 774273 [details] [diff] [review]
Patch v5

> I would have to say that you presumed wrongly

You mean it compiled?  Curious...  How?

r=me on this updated version.

Comment 17

[Marcus Saad (:msaad)]

Attachment: Patch v6

Changes were made to land this patch. It now only includes the validation and idl changes. The changes on WebVTTLoadListener were removed and will be landed when the new patch from reyre integrates the creation of new texttrackcues.

Comment 18

[Rick Eyre (:reyre)]

Comment on attachment 780532 [details] [diff] [review]
Patch v6

Review of attachment 780532 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 19

[Marcus Saad (:msaad)]

Comment on attachment 780532 [details] [diff] [review]
Patch v6

We had to change the patch a little by removing the MOZ.ASSERT while we are not creating the textrackcues anymore. Changes were made to those files and a new patch that reyre is working on should land that soon.

We'll try push it as soon as you review. 

Thanks

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 780532 [details] [diff] [review]
Patch v6

r=me

Comment 21

[Rick Eyre (:reyre)]

Try server: https://tbpl.mozilla.org/?tree=Try&rev=5ce4a94ee8f1

Comment 22

[Rick Eyre (:reyre)]

Looks green marking as checkin for Marcus.

We will land tests for this in bug 903030.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f538bf3eafc3

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

So do we actually want this to throw? If these attributes are turned into enums, setting to an invalid value will *not* throw.

Comment 25

[Rick Eyre (:reyre)]

(In reply to :Ms2ger from comment #24)
> So do we actually want this to throw? If these attributes are turned into
> enums, setting to an invalid value will *not* throw.

We were going with following the spec for now until it's changed over to using enums. I've filed a bug for that to happen. See https://www.w3.org/Bugs/Public/show_bug.cgi?id=22356

Actually.. now that I'm looking at the spec the IDL has changed since the last
time we worked on this. The Direction cue setting is now an enum. However, the portion describing how setting and getting should work hasn't changed and still
says to throw.

We can open a bug to update it after this lands? Or.. ?

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

It's already landed, so a new bug, I guess :)

Comment 27

[Rick Eyre (:reyre)]

(In reply to :Ms2ger from comment #26)
> It's already landed, so a new bug, I guess :)

Sounds good :). New bug is filed as bug 903425

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f538bf3eafc3