TimeUnits.h defines a TimeIntervals class, which defines operators to convert to and from dom::TimeRanges. This means TimeUnits.h #includes "mozilla/dom/TimeRanges.h", and since dom::TimeRanges is a DOM binding object that means it needs to include "nsWrapperCache.h" which includes DOM bindings stuff which eventually ends up pulling in a whole lot of JavaScript definitions and cycle collection stuff. This greatly increases the include graph, so makes importing TimeUnits.h into Servo much harder, and will also increase the C++ build time. It would greatly reduce the amount of stuff I need to import into Servo for media playback if I can move the TimeIntervals to dom::TimeRanges out of TimeUnits.h and into TimeRanges.h. This also separates the concerns neatly; the code to convert the Gecko media TimeIntervals to Gecko DOM bindings is in Gecko's DOM bindings, and Servo will likely need something equivalent for its own DOM bindings.

Comment on attachment 8899675 [details] Bug 1392498 - Move TimeIntervals to dom::TimeRanges conversion into TimeRanges class. https://reviewboard.mozilla.org/r/170972/#review176204 ::: dom/html/HTMLMediaElement.cpp:2870 (Diff revision 1) > already_AddRefed<TimeRanges> > HTMLMediaElement::Seekable() const > { > - RefPtr<TimeRanges> ranges = new TimeRanges(ToSupports(OwnerDoc())); > - if (mDecoder) { > - mDecoder->GetSeekable().ToTimeRanges(ranges); > + media::TimeIntervals seekable = > + mDecoder ? mDecoder->GetSeekable() : media::TimeIntervals(); > + RefPtr<TimeRanges> ranges = new TimeRanges(ToSupports(OwnerDoc()), seekable); we're in the dom namespace here, there seems to be a lack of consistency in this code, where sometimes you see dom::TimeRanges and other times TimeRanges only. Can you make it consistent everywhere please? ::: dom/html/HTMLMediaElement.cpp (Diff revision 1) > HTMLMediaElement::Buffered() const > { > - RefPtr<TimeRanges> ranges = new TimeRanges(ToSupports(OwnerDoc())); > - if (mDecoder) { > - media::TimeIntervals buffered = mDecoder->GetBuffered(); > + media::TimeIntervals buffered = > + mDecoder ? mDecoder->GetBuffered() : media::TimeIntervals(); > + RefPtr<TimeRanges> ranges = new TimeRanges(ToSupports(OwnerDoc()), buffered); > - if (!buffered.IsInvalid()) { the code isn't strictly equivalent, there's no check for IsInvalid() Though I think this was a bogus test, one that would always be true. AFAIK, only the OggReader could generate invalid buffer ranges, and is no longer there ::: dom/html/TimeRanges.h:60 (Diff revision 1) > void Union(const TimeRanges* aOtherRanges, double aTolerance); > > // Mutate this TimeRange to be the intersection of this and aOtherRanges. > void Intersection(const TimeRanges* aOtherRanges); > > - virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override; > + JSObject* WrapObject(JSContext* aCx, this seems to be out of scope for the title of this change... So please, either create a new bug, or amend the commit comment describing this change. (though it was silly to make those members virtual, when the TimeRanges class is declared final) ::: dom/html/TimeRanges.h:70 (Diff revision 1) > uint32_t Length() const > { > return mRanges.Length(); > } > > - virtual double Start(uint32_t aIndex, ErrorResult& aRv); > + double Start(uint32_t aIndex, ErrorResult& aRv) const; same here ::: dom/html/TimeRanges.cpp:42 (Diff revision 1) > + : TimeRanges(aParent) > +{ > + if (aTimeIntervals.IsInvalid()) { > + return; > + } > + for (media::TimeIntervals::IndexType i = 0; i < aTimeIntervals.Length(); you could use the iterators there instead. for (const media::TimeInterval& interval : aTimeIntervals) { Add(interval.mStart.ToSeconds(), interval.mEnd.ToSeconds()); } ::: dom/media/gtest/TestIntervalSet.cpp (Diff revision 1) > - media::TimeIntervals i2 = media::TimeIntervals::FromTimeRanges(tr); > - CheckTimeRanges(tr, i2); > + RefPtr<dom::TimeRanges> tr2 = new dom::TimeRanges(tr->ToTimeIntervals()); > + CheckTimeRanges(tr2, i1); > - > - media::TimeIntervals i3; > - // operator=(TimeRanges*) > - i3 = tr; still think you should have gtest for the operators and implement those operators. Or indicate as such in the commit comment ::: dom/media/mediasource/SourceBuffer.cpp:117 (Diff revision 1) > } > bool rangeChanged = true; > media::TimeIntervals intersection = mTrackBuffersManager->Buffered(); > MSE_DEBUGV("intersection=%s", DumpTimeRanges(intersection).get()); > if (mBuffered) { > - media::TimeIntervals currentValue(mBuffered); > + media::TimeIntervals currentValue(mBuffered->ToTimeIntervals()); would have been nicer with an operator=(dom::TimeRanges)

Thanks for the fast review. I've uploaded visualizations of the before/after snapshot of the include graphs, to help make the impact of the change here clear.

Comment on attachment 8899675 [details] Bug 1392498 - Move TimeIntervals to dom::TimeRanges conversion into TimeRanges class. https://reviewboard.mozilla.org/r/170972/#review178338 ::: dom/html/HTMLMediaElement.cpp (Diff revision 1) > HTMLMediaElement::Buffered() const > { > - RefPtr<TimeRanges> ranges = new TimeRanges(ToSupports(OwnerDoc())); > - if (mDecoder) { > - media::TimeIntervals buffered = mDecoder->GetBuffered(); > + media::TimeIntervals buffered = > + mDecoder ? mDecoder->GetBuffered() : media::TimeIntervals(); > + RefPtr<TimeRanges> ranges = new TimeRanges(ToSupports(OwnerDoc()), buffered); > - if (!buffered.IsInvalid()) { I moved the IsInvalid() check into the TimeRanges constructor, so I think the code is equivalent. We'll still generate an empty TimeRanges here in the invalid case. ::: dom/html/TimeRanges.h:70 (Diff revision 1) > uint32_t Length() const > { > return mRanges.Length(); > } > > - virtual double Start(uint32_t aIndex, ErrorResult& aRv); > + double Start(uint32_t aIndex, ErrorResult& aRv) const; This needed to become const for ToTimeIntervals() to be const. Otherwise ToTimeIntervals() would need to be non-const, which is yucky. ::: dom/media/gtest/TestIntervalSet.cpp (Diff revision 1) > - media::TimeIntervals i2 = media::TimeIntervals::FromTimeRanges(tr); > - CheckTimeRanges(tr, i2); > + RefPtr<dom::TimeRanges> tr2 = new dom::TimeRanges(tr->ToTimeIntervals()); > + CheckTimeRanges(tr2, i1); > - > - media::TimeIntervals i3; > - // operator=(TimeRanges*) > - i3 = tr; Sorry, these operators greatly complicate reusing this code outside of Gecko. ::: dom/media/mediasource/SourceBuffer.cpp:117 (Diff revision 1) > } > bool rangeChanged = true; > media::TimeIntervals intersection = mTrackBuffersManager->Buffered(); > MSE_DEBUGV("intersection=%s", DumpTimeRanges(intersection).get()); > if (mBuffered) { > - media::TimeIntervals currentValue(mBuffered); > + media::TimeIntervals currentValue(mBuffered->ToTimeIntervals()); Sorry, these operators greatly complicate reusing this code outside of Gecko.

Pushed by cpearce@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ac74aef901ed Move TimeIntervals to dom::TimeRanges conversion into TimeRanges class. r=jya