This includes TextTrackRegion, TextTrackRegionList, and additions to TextTrack to deal with TextTrackRegions.

Just a note: I'll try to get to this tomorrow, but if not it will be next week. I'm on vacation wednesday-sunday. You should ask a dom peer for review as well; might was well do that in the meantime. Rick, does it make sense to implement TextTrackRegion without the VTTCue changes, or does this imply we're transitioning now?

(In reply to Ralph Giles (:rillian) from comment #2) > Rick, does it make sense to implement TextTrackRegion without the VTTCue > changes, or does this imply we're transitioning now? I think it should be fine. Most of the TextTrackRegion changes will alter TextTrack as the Regions live on the TextTrack in a list so it won't be touching TextTrackCue that much. The only thing we will be adding to TextTrackCue is a RegionID. We can just add it to the TextTrackCue like we have been doing with the other ones until it comes time to abstract the VTTCue class Most of VTTCue is implemented as TextTrackCue already. There is just this bug here 903425 and this 882299 left.

> You should ask a dom peer for review as well; might was well do that in the > meantime. Rick, do you know any dom peer apart from bz - he's on vacation IIRC -who can review this whilst Ralph is away ?

Kyle would you like to do DOM review for this? If not can you recommend someone who would be able too?

I don't think 'like' is the correct word, but I will.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #6) > I don't think 'like' is the correct word, but I will. Heh, what's not fun about reviewing?! Thanks for this.

Comment on attachment 788472 [details] [diff] [review] Implementation Review of attachment 788472 [details] [diff] [review]: ----------------------------------------------------------------- Please address the following spec issues that I filed in your next Gecko patch. https://www.w3.org/Bugs/Public/show_bug.cgi?id=22961 https://www.w3.org/Bugs/Public/show_bug.cgi?id=22962 https://www.w3.org/Bugs/Public/show_bug.cgi?id=22963 Why are you calling these TextTrackRegion... instead of VTTRegion... as the spec says? I didn't review the details of the TextTrackRegion[|List] implementations because they will have to change a fair bit with the spec bugs I filed. ::: content/media/TextTrack.cpp @@ +10,5 @@ > > namespace mozilla { > namespace dom { > > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_4(TextTrack, This is just wrong, but that's not your fault. I'll file a separate bug on it. @@ +86,5 @@ > { > //XXX: Implement Cue changed. Bug 867823. > } > > +void Nit: whitespace at EOL. @@ +90,5 @@ > +void > +TextTrack::SetRegions(TextTrackRegionList* aList) > +{ > + mRegionList = aList; > +} If this is supposed to be read only, this function should be dropped. @@ +95,5 @@ > + > +void > +TextTrack::AddRegion(TextTrackRegion& aRegion) > +{ > + nsAutoString id; nsString. No need for stack allocated storage when you are calling a getter. @@ +99,5 @@ > + nsAutoString id; > + aRegion.GetId(id); > + > + TextTrackRegion* region = mRegionList->GetRegionById(id); > + if (region) { reverse this. if (!region) { AddTextTrackRegion(blah); return; } copy stuff. @@ +110,5 @@ > + > + nsAutoString scroll; > + aRegion.GetScroll(scroll); > + region->SetScroll(scroll); > + You should add a function on TextTrackRegion that does this for you. @@ +121,5 @@ > +void > +TextTrack::RemoveRegion(const TextTrackRegion& aRegion) > +{ > + nsAutoString id; > + aRegion.GetId(id); Same here about the auto string. @@ +126,5 @@ > + > + if (!mRegionList->GetRegionById(id)) { > + ErrorResult rv; > + rv.Throw(NS_ERROR_DOM_NOT_FOUND_ERR); > + return; You need to mark this as throws in the WebIDL so an ErrorResult& is passed in by the binding layer. ::: content/media/TextTrack.h @@ +10,5 @@ > #include "mozilla/dom/TextTrackBinding.h" > #include "mozilla/dom/TextTrackCue.h" > #include "mozilla/dom/TextTrackCueList.h" > +#include "mozilla/dom/TextTrackRegion.h" > +#include "mozilla/dom/TextTrackRegionList.h" Forward declarations should be sufficient. You should not need to include these headers in the .h file. Move them to the .cpp file. ::: content/media/TextTrackRegionList.h @@ +56,5 @@ > +private: > + nsCOMPtr<nsISupports> mParent; > + nsTArray<nsRefPtr<TextTrackRegion> > mTextTrackRegions; > +}; > + nit: whitespace

Thanks for the review! > Why are you calling these TextTrackRegion... instead of VTTRegion... as the > spec says? > It looks like the spec got updated after i posted this patch. See https://dvcs.w3.org/hg/text-tracks/rev/64a941b7e915. > Please address the following spec issues that I filed in your next Gecko > patch. > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=22961 > https://www.w3.org/Bugs/Public/show_bug.cgi?id=22962 > https://www.w3.org/Bugs/Public/show_bug.cgi?id=22963 > I'll update this patch once Bug 905320 lands.

Comment on attachment 788472 [details] [diff] [review] Implementation Cancelling need for review per Kyle's comments.

Addresses earlier review comments and additional spec issues.

Andrew, can you please add a content constructor to the TextTrackRegion object as well. It's in the IDL and we'll need it to hook up vtt.js to Regions in Gecko easily.

The same as previous patch submission which addresses Kyle's earlier review...see comment 8. Apart from fixing a few nits, i added a content constructor to the TextTrackRegion object - thanks Rick for noticing that! :) Try push: https://tbpl.mozilla.org/?tree=Try&rev=4f05c0578b88 I suspect Kyle might have a loaded review queue so switching reviewer.

Comment on attachment 794975 [details] [diff] [review] Impl v3 Review of attachment 794975 [details] [diff] [review]: ----------------------------------------------------------------- Still needs to be reviewed by a DOM peer. I should be able to get to this on Monday.

Comment on attachment 794975 [details] [diff] [review] Impl v3 Review of attachment 794975 [details] [diff] [review]: ----------------------------------------------------------------- In general this looks good. Please address comments and ask for review again. ::: content/media/TextTrackRegion.cpp @@ +25,5 @@ > +{ > + return TextTrackRegionBinding::Wrap(aCx, aScope, this); > +} > + > +/*static*/ already_AddRefed<TextTrackRegion> Is it static, or isn't it? :-) @@ +48,5 @@ > + , mViewportAnchorX(0) > + , mViewportAnchorY(100) > +{ > + mTrack = new TextTrack(aGlobal); > + mId.Assign(NS_LITERAL_STRING("")); I don't think you need this. An nsString should be empty on construction. ::: content/media/TextTrackRegionList.h @@ +8,5 @@ > +#define mozilla_dom_TextTrackRegionList_h > + > +#include "nsAutoPtr.h" > +#include "nsCycleCollectionParticipant.h" > +#include "nsWrapperCache.h" Includes in alphabetical order please. @@ +29,5 @@ > + JS::Handle<JSObject*> aScope) MOZ_OVERRIDE; > + > + TextTrackRegionList(nsISupports* aGlobal); > + > + virtual ~TextTrackRegionList(); Why do you have a virtual dtor here? ::: dom/webidl/TextTrack.webidl @@ +29,5 @@ > > readonly attribute DOMString id; > readonly attribute DOMString inBandMetadataTrackDispatchType; > > + attribute TextTrackMode mode; These are deliberately aligned with the 'attribute' column of the readonly attribution. Please don't change that. Or if it really bugs you, do it for every webidl file in a separate bug. @@ +43,4 @@ > }; > + > + > +partial interface TextTrack { Just merge these attributes with the main interface above. ::: dom/webidl/TextTrackRegion.webidl @@ +14,5 @@ > + attribute DOMString id; > + attribute long lines; > + > + [SetterThrows] > + attribute double width; Please move this above lines so it's in the same order as the spec.

Addresses previous review comments.

Comment on attachment 796954 [details] [diff] [review] Impl v4 Review of attachment 796954 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. r=me with additional nits addressed. ::: content/media/TextTrackRegion.cpp @@ +52,5 @@ > + SetIsDOMBinding(); > +} > + > +TextTrackRegion::~TextTrackRegion() > +{} You shouldn't need to declare and define a dtor at all. If it doesn't do anything, leave it out. ::: dom/webidl/TextTrack.webidl @@ +43,5 @@ > [SetterThrows] > + attribute EventHandler oncuechange; > + > + [Throws] > + void removeRegion(TextTrackRegion region); Again, please put addRegion and removeRegion together.

Comment on attachment 796954 [details] [diff] [review] Impl v4 Review of attachment 796954 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/TextTrackRegion.webidl @@ +24,5 @@ > + attribute double regionAnchorY; > + [SetterThrows] > + attribute double viewportAnchorX; > + [SetterThrows] > + attribute double viewportAnchorY; You have [SetterThrows] on a ton of stuff that doesn't throw. Why?

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #18) > Comment on attachment 796954 [details] [diff] [review] > Impl v4 > > Review of attachment 796954 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/webidl/TextTrackRegion.webidl > @@ +24,5 @@ > > + attribute double regionAnchorY; > > + [SetterThrows] > > + attribute double viewportAnchorX; > > + [SetterThrows] > > + attribute double viewportAnchorY; > > You have [SetterThrows] on a ton of stuff that doesn't throw. Why? They can throw since they use InvalidValue(double aValue, ErrorResult& aRv) to check the passed argument. That throws... do see TextTrackRegion.h, line #173.

Comment on attachment 796954 [details] [diff] [review] Impl v4 Ah, ok. I'll take another look.

Comment on attachment 796954 [details] [diff] [review] Impl v4 Review of attachment 796954 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/TextTrackRegion.cpp @@ +19,5 @@ > + NS_INTERFACE_MAP_ENTRY(nsISupports) > +NS_INTERFACE_MAP_END > + > + > +JSObject* nit: extra newline above ::: content/media/TextTrackRegion.h @@ +76,5 @@ > + > + void SetWidth(double aWidth, ErrorResult& aRv) > + { > + if (!InvalidValue(aWidth, aRv)) > + mWidth = aWidth; brace single line ifs. I didn't bother to call this out everywhere, but please fix them all. @@ +155,5 @@ > + return mId; > + } > + > + > +private: nit: extra \n above @@ +168,5 @@ > + double mViewportAnchorY; > + nsString mScroll; > + > + //Helper to ensure new value is in the range: 0.0% - 100.0%; throws > + //an IndexSizeError otherwise. nit: space after // ::: content/media/TextTrackRegionList.cpp @@ +20,5 @@ > + NS_INTERFACE_MAP_ENTRY(nsISupports) > +NS_INTERFACE_MAP_END > + > + > +JSObject* nit: extra newline above @@ +33,5 @@ > + SetIsDOMBinding(); > +} > + > +TextTrackRegionList::~TextTrackRegionList() > +{} same thing about empty ctors. @@ +46,5 @@ > +TextTrackRegion* > +TextTrackRegionList::GetRegionById(const nsAString& aId) > +{ > + if (aId.IsEmpty()) > + return nullptr; brace the if @@ +52,5 @@ > + for (uint32_t i = 0; i < Length(); ++i) { > + if (aId.Equals(mTextTrackRegions[i]->Id())) > + return mTextTrackRegions[i]; > + } > + return nullptr; nit: newline after } @@ +68,5 @@ > + mTextTrackRegions.RemoveElement(&aRegion); > +} > + > + > +} //namespace dom nit: extra newline above ::: content/media/TextTrackRegionList.h @@ +11,5 @@ > +#include "nsCycleCollectionParticipant.h" > +#include "nsTArray.h" > +#include "nsWrapperCache.h" > + > + nit extra newline above

Nits fixed and pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/574bc42c68c5