media-source/mediasource-is-type-supported.html tests for invalid mime types that dont fail: ------------------------------------------------ FAIL Test invalid MIME format "video/webm" FAIL Test invalid MIME format "video/webm;" FAIL Test invalid MIME format "video/webm;codecs" FAIL Test invalid MIME format "video/webm;codecs=" FAIL Test invalid MIME format "video/webm;codecs="" FAIL Test invalid MIME format "video/webm;codecs=""" FAIL Test invalid mismatch between major type and codec ID "audio/webm;codecs="vp8"" and valid mimetypes that fail but should not: AUDIO/WEBM;CODECS="vorbis"

I've opened https://github.com/w3c/web-platform-tests/issues/2409 as I believe those tests are wrong. The remaining test failing audio/webm;codecs="vp8"" will be fixed here.

Also move the logic inside WebMDecoder, this continue on with the logic used by MP4Decoder

As per RFC 2616 section 3.7, which states "The type, subtype, and parameter attribute names are case-insensitive". So ensure the type and subtype are always lower-case as all our comparisons assume that they are. HTML3 defines a mimetype as per RFC 1521 which has been deprecated by RFC 2045 HTML4 defines a mimetype as per RFC 2045 and 2046 definition, however state that "Content types are case-insensitive." HTML5 defines a valid mimetype is as per RFC 2616 section 3.7, which states "The type, subtype, and parameter attribute names are case-insensitive" So always ensuring that type/subtype are lower case is the most expedient and efficient thing to do (over using case-insensitve comparison everwhere thorough our code)

staged change didn't get committed. sorry for that

Comment on attachment 8695161 [details] [diff] [review] P1. Properly check webm mimetype against codec type. Review of attachment 8695161 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/webm/WebMDecoder.h @@ +22,5 @@ > } > virtual MediaDecoderStateMachine* CreateStateMachine(); > + > + // Returns true if the WebM backend is preffed on. > + static bool IsEnabled(); This doesn't need to be public, right?

(In reply to Chris Pearce (:cpearce) from comment #6) > This doesn't need to be public, right? I was using it for bug 1229605. but since I've removed it. so yes, I could remove it

Comment on attachment 8695164 [details] [diff] [review] P2. Ensure type and subtype of MIME are lowercase. The string being returned here is not necessarily a type, subtype, or param name. In fact, the explicit comment about what this is _supposed_ to be used for shows it being used for a param _value_. At the very least, I would like to see an audit of all consumers here that shows they're OK with this behavior change, plus updates to the documentation that make it clear what the behavior is.

Comment on attachment 8695166 [details] [diff] [review] P3. Update webref test expected results. >Bug 1191833: P3. Update webref test expected results. r=karlt Please document why the results changed in the commit message.

(In reply to Boris Zbarsky [:bz] from comment #8) > Comment on attachment 8695164 [details] [diff] [review] > P2. Ensure type and subtype of MIME are lowercase. > > The string being returned here is not necessarily a type, subtype, or param > name. In fact, the explicit comment about what this is _supposed_ to be > used for shows it being used for a param _value_. The only place I see it called with aParamName being null is by nsContentTypeParser::GetType. i couldn't find confirmation that the param value in the condition it is used (Content-Disposition: inline) that this parameter itself should be lowercase (strong gutfeel that this is the case). All our coverage tests are using lowercase values. Would narrowing the change to cases where aParamName *is* null (rather than *aParamName being 0, e.g parameter is empty) be better? In which case that would narrow the change to nsContentTypeParser::GetType. > > At the very least, I would like to see an audit of all consumers here that > shows they're OK with this behavior change, plus updates to the > documentation that make it clear what the behavior is. fair enough. I did go through most places where it's called and assumed that all really you should be case insensitive. But that's probably not as thorough as you would like. In any case, it's very unfortunate that nsMIMEHeaderParamImpl.cpp being used for other thing than MIME data.

Comment on attachment 8695164 [details] [diff] [review] P2. Ensure type and subtype of MIME are lowercase. Will handle this separately in bug 1230353

> The only place I see it called with aParamName being null is by > nsContentTypeParser::GetType. But my point is that we need to actually look at all the consumers, including extensions, and see what they're using this for, if we're going to change the API on them. It might be OK. Just needs checking. Though see below. > Would narrowing the change to cases where aParamName *is* null (rather than *aParamName > being 0, e.g parameter is empty) be better? As long as you actually audit all the callpaths that can reach this code. But see below. > In which case that would narrow the change to nsContentTypeParser::GetType. Did you check extensions MXR? But also, if we really only care about nsContentTypeParser::GetType, seems to me like we should do the lowercasing _there_ and avoid trying to figure out whether there is collateral damage. I definitely agree that nsContentTypeParser::GetType should only produce lowercase values. That's what necko's internal content-type parsers always do for the actual media type... > it's very unfortunate that nsMIMEHeaderParamImpl.cpp being used for other thing than > MIME data. I'm not sure what you mean by "other thing than MIME data". It's designed to parse arbitrary MIME headers as defined in various MIME RFCs (2045, 2047, 2183, 2231, 5987 at the very least). The HTTP "Content-Type" header is one such header, which happens to have some extra structure (like some parts being defined as case-insensitive, etc), but there are other headers that follow these general syntactic rules but otherwise can have different semantics. In particular, this implementation class and the nsIMIMEHeaderParam interface were initially added for parsing "Content-Disposition" headers. See bug 162765. That's still what most consumers seem to use it for, not surprisingly.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f23035b9e6bd https://hg.mozilla.org/integration/mozilla-inbound/rev/60230ebb71eb

https://hg.mozilla.org/mozilla-central/rev/f23035b9e6bd https://hg.mozilla.org/mozilla-central/rev/60230ebb71eb