I need this code in the decodeAudioData implementation, so let's put it in a shared class called DecoderTraits or something.

Comment on attachment 684279 [details] [diff] [review] Patch (v1) Review of attachment 684279 [details] [diff] [review]: ----------------------------------------------------------------- You sir, have a knack for writing patches that I'm half way through writing myself! r=cpearce with the following changes. ::: content/html/content/src/nsHTMLMediaElement.cpp @@ -2375,1 @@ > if (status == CANPLAY_NO) It would be good if you could push the logic that uses supportedCodecs down into DecoderTraits::CanHandleMediaType() as well. Then you don't need to return the codecs list either. ::: content/media/DecoderTraits.cpp @@ +172,5 @@ > + > +bool > +DecoderTraits::IsH264Type(const nsACString& aType) > +{ > + for (uint32_t i = 0; i < ArrayLength(gH264Types); ++i) { All these loops over these arrays can be refactored into a single function. Basically CodecListContains from nsHTMLMediaElement.cpp. @@ +266,5 @@ > +} > +#endif > + > +/* static */ > +bool DecoderTraits::ShouldHandleMediaType(const char* aMIMEType) This can simply be: #ifdef MOZ_WAVE if (IsWaveType(aMIMEType)) { // $Explanation_comment... return false; } #endif return CanHandleMediaStatus(...) != CANPLAY_NO. ::: content/media/DecoderTraits.h @@ +34,5 @@ > + // false here even if CanHandleMediaType would return true. > + static bool ShouldHandleMediaType(const char* aMIMEType); > + > +#ifdef MOZ_RAW > + static const char gRawTypes[2][16]; None of the g*Types and g*Codecs need to be exposed public; they can be declared static inside DecoderTraits.cpp.

Comment on attachment 684279 [details] [diff] [review] Patch (v1) Review of attachment 684279 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/nsHTMLMediaElement.cpp @@ -2375,1 @@ > if (status == CANPLAY_NO) Hmm, this turned out to be a much more complicated patch than I expected, and I haven't managed to get it right. I'll submit that as a separate patch. ::: content/media/DecoderTraits.cpp @@ +172,5 @@ > + > +bool > +DecoderTraits::IsH264Type(const nsACString& aType) > +{ > + for (uint32_t i = 0; i < ArrayLength(gH264Types); ++i) { This depends on the previous part. @@ +266,5 @@ > +} > +#endif > + > +/* static */ > +bool DecoderTraits::ShouldHandleMediaType(const char* aMIMEType) Hmm, I don't follow how these are equivalent? ::: content/media/DecoderTraits.h @@ +34,5 @@ > + // false here even if CanHandleMediaType would return true. > + static bool ShouldHandleMediaType(const char* aMIMEType); > + > +#ifdef MOZ_RAW > + static const char gRawTypes[2][16]; Right, I'll do that.

Comment on attachment 684279 [details] [diff] [review] Patch (v1) Review of attachment 684279 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/DecoderTraits.cpp @@ +266,5 @@ > +} > +#endif > + > +/* static */ > +bool DecoderTraits::ShouldHandleMediaType(const char* aMIMEType) These are equivalent because CanHandleMediaType() returns != CANPLAY_NO when one of the Is*Type() functions return true. Whereas ShouldHandleMediaType() (as you've written it) returns true when one of the Is*Type() functions return true. Except for Wave. Does that make sense?

(In reply to Ehsan Akhgari [:ehsan] from comment #3) > Comment on attachment 684279 [details] [diff] [review] > Patch (v1) > > Review of attachment 684279 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/html/content/src/nsHTMLMediaElement.cpp > @@ -2375,1 @@ > > if (status == CANPLAY_NO) > > Hmm, this turned out to be a much more complicated patch than I expected, > and I haven't managed to get it right. I'll submit that as a separate patch. Actually, this didn't turn out to be very difficult!

(In reply to Chris Pearce (:cpearce) from comment #5) > Comment on attachment 684279 [details] [diff] [review] > Patch (v1) > > Review of attachment 684279 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/DecoderTraits.cpp > @@ +266,5 @@ > > +} > > +#endif > > + > > +/* static */ > > +bool DecoderTraits::ShouldHandleMediaType(const char* aMIMEType) > > These are equivalent because CanHandleMediaType() returns != CANPLAY_NO when > one of the Is*Type() functions return true. > > Whereas ShouldHandleMediaType() (as you've written it) returns true when one > of the Is*Type() functions return true. Except for Wave. > > Does that make sense? Yes. Will do.

https://hg.mozilla.org/integration/mozilla-inbound/rev/abfcf6bd97a4 https://hg.mozilla.org/integration/mozilla-inbound/rev/2020b35ea3b4 https://hg.mozilla.org/integration/mozilla-inbound/rev/27e1c4fba7e6 https://hg.mozilla.org/integration/mozilla-inbound/rev/37626d0f43fc (I addressed each major review comment in its own patch in order to keep the patches simple.)

https://hg.mozilla.org/mozilla-central/rev/abfcf6bd97a4 https://hg.mozilla.org/mozilla-central/rev/2020b35ea3b4 https://hg.mozilla.org/mozilla-central/rev/27e1c4fba7e6 https://hg.mozilla.org/mozilla-central/rev/37626d0f43fc