Placeholder for //XXX comments in TextTrackCue.h: - Validate arguement to SetStartTime - Validate arguement to SetEndTime

Also SetLine, SetPosition, SetSize, SetText.

I kind need more information on this. I've read the draft, did what I could extract from there. There are some parts that need to be figured out such as : - Line can receive a keyword auto, right now we only accept a double. - How to throw IndexSizeError ?

I'm not sure if we actually want to throw errors at this point. I know Dave put that in as a stub, but since then nowhere in the spec is it telling us to throw. The WebIDL for WebVTTCue doesn't have any of the attributes throwing either. I'm thinking we should just validate that they are a certain value and if they don't match one of the accepted values we don't do anything. For the values that are percentages we can do something like if the value being set is > 100 or < 0 then they just become 100 or 0 respectively.

I'm proposing a new patch based on comment #3 -Returning when values are not in the expected range or if it is > 100 or < 0 then they just become 100 or 0 respectively. -Added calls to CueChanged()

Comment on attachment 761193 [details] [diff] [review] proposed patch v2 Review of attachment 761193 [details] [diff] [review]: ----------------------------------------------------------------- You have a lot of whitespace please get rid of that too. ::: content/media/TextTrackCue.h @@ +108,4 @@ > if (mEndTime == aEndTime) > return; > + > + if (aEndTime <= mStartTime) Tabbing issue here @@ +193,4 @@ > if (mPosition == aPosition) > return; > + > + if (aPosition > 100 || aPosition < 0) Tabbing issue here.

We probably want to validate the vertical field as well. I would think it would have to be either 'lr', 'rl', or 'none'? In which case it would be horizontal writing mode? The spec doesn't make it clear how the writing direction maps to the WEBVTT API.

Also, it looks like something happened with the diff where you have four sets of TextTrackCue.h changes that are identical in your file.

Also, please use { } even for simple if statements. That's the preferred way in Gecko.

To amend my comment above -- vertical should be an empty string in the case of horizontal text. http://dev.w3.org/html5/webvtt/#webvtt-api -- scroll down to writing direction values.

I'm also thinking we should separate out the "line" property into a separate bug as that isn't even implemented yet i.e. it's still commented out in the WebIDL and the code we have for it in TextTrackCue is really just a stub. We can keep the scope of this bug to adding validation to our current properties. So you don't have to worry about that right now Marcus.

Addressed comments: -{} on all if statements -Opened a new bug for the implementation of line setter -Validation on Vertical to verify values such as "lr", "rl" and empty.

Comment on attachment 761571 [details] [diff] [review] proposed patch v3 Review of attachment 761571 [details] [diff] [review]: ----------------------------------------------------------------- I think your diffing wrong as well. Your diffing against the changes you made in your first patch instead of what is in m-c. ::: content/media/TextTrackCue.h @@ +91,4 @@ > return; > + } > + > + if (aStartTime < 0){ This shouldn't be here. According to the spec we don't care if start time is negative. Hixie confirmed this to. If it ends up causing bugs in the future we can open a new bug. @@ +110,4 @@ > return; > + } > + > + if (aEndTime < mStartTime){ Shouldn't be here too. Same reasons as before. @@ +184,1 @@ > if (!mSnapToLines && aLine > 100){ I don't think we need to do this validation. The interaction between snapToLines and line happens when the value is being interpreted not when it's being set. @@ +211,1 @@ > if (aPosition > 100){ We can instead have if (aPosition < 0 || aPosition > 100) We also need to throw an IndexSizeError in this case. @@ +235,4 @@ > return; > + } > + > + if (aSize < 0 ){ This logic can be the same as SetPositions -- it also needs to throw an IndexSizeError.

Sorry for the little mess, but connection here on this end is not behaving properly. I've addressed your comments on this Rick. The only thing left is the SetterThrows that still needs to be implemented and will follow on bug882743.

You diffed wrong. You diffed against your last patch instead of against m-c.

Very simple changes. The bulk validation is done on bug882743, where SetterThrows is introduced and properly throwing. I guess this is the correct diff, if it's not, you can kill me.

Comment on attachment 765131 [details] [diff] [review] proposed patch v5 Review of attachment 765131 [details] [diff] [review]: ----------------------------------------------------------------- We can probably just include this into the patch for the webidl update. See bug 882743 for details.