469247 - Implement canPlayType

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is pretty easy.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Creates a little nsMIMEParser helper class to avoid pain, and refactors some existing code to use it. I'll add doublec as a reviewer for the video stuff...

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Slight cleanup:
-- Add RFC references in comments
-- Change CANPLAY_PROBABLY to CANPLAY_YES internally, it's clearer
-- Make CodecListContains take an nsAString argument, we don't need to do a UTF8 conversion on the codec name

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 352687 [details] [diff] [review]
fix v2

>+++ b/content/base/public/nsContentUtils.h
>-private:

Why that change?  Doesn't seem necessary.

>+class nsMIMEParser {

Maybe nsMIMEHeaderParser?  I guess not all of these are headers... OK, lets stick with this naming.

>+++ b/content/base/src/nsContentUtils.cpp
>+nsMIMEParser::nsMIMEParser(const nsAString& aString)
>+  nsCOMPtr<nsIMIMEHeaderParam> mimeHdrParser =
>+    do_GetService("@mozilla.org/network/mime-hdrparam;1");
>+  mimeHdrParser.swap(mService);

CallGetService?

>+++ b/content/base/src/nsScriptLoader.cpp

I like the changes here.  ;)

>+++ b/content/html/content/public/nsHTMLMediaElement.h
>+  // If it returns true, then it also returns a null-terminated list
>+  // of supported codecs in *aSupportedCodecs, and a null-terminated list
>+  // of codecs that *may* be supported in *aMaybeSupportedCodecs.

Document that the caller doesn't need to free those lists?  That is, that this not an XPCOM-like getter thing.

>+++ b/content/html/content/src/nsHTMLMediaElement.cpp
> static const char gOggTypes[][16] = {
...
>+static const char* gOggCodecs[] = {

I guess you can't use |static const char [][7]| because you need to use nsnull for the terminator?

>+CodecListContains(const char** aCodecs, const nsAString& aCodec)
>+{
>+  for (PRInt32 i = 0; aCodecs[i]; ++i) {
>+    if (aCodec.EqualsASCII(aCodecs[i]))

Could also do:

  for (; *aCodecs; ++aCodecs) {
    if (aCodec.EqualsASCII(*aCodecs))

I think it ends up compiling a bit more efficiently last I checked, but we probably don't care much here.  Use whichever you think is more readable, I guess.

>+static CanPlayStatus GetCanPlay(const nsAString& aType, nsAString& aMIMEType)

>+  // See http://www.rfc-editor.org/rfc/rfc4281.txt for the description
>+  // of the 'codedcs' parameter

s/codedcs/codecs/

>+    nsDependentSubstring token(tokenizer.nextToken());

Could also use:

  nsSubstring & token = tokenizer.nextToken();

I think.  Either way's good.

>+  if (expectMoreTokens)
>+    return CANPLAY_NO;

Is that already covered in the RFC, or worth documenting?

Rest looks good.  r+sr=bzbarsky with the nits (esp the private thing in nsContentUtils.h).

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #3)
> I guess you can't use |static const char [][7]| because you need to use nsnull
> for the terminator?

I could use an empty string, but it seems like overkill.

> >+CodecListContains(const char** aCodecs, const nsAString& aCodec)
> >+{
> >+  for (PRInt32 i = 0; aCodecs[i]; ++i) {
> >+    if (aCodec.EqualsASCII(aCodecs[i]))
> 
> Could also do:
> 
>   for (; *aCodecs; ++aCodecs) {
>     if (aCodec.EqualsASCII(*aCodecs))
> 
> I think it ends up compiling a bit more efficiently last I checked, but we
> probably don't care much here.  Use whichever you think is more readable, I
> guess.

I find the former more readable.

> >+  if (expectMoreTokens)
> >+    return CANPLAY_NO;
> 
> Is that already covered in the RFC, or worth documenting?

I belive it's already covered in the RFC. The comma must be followed by an 'id-simple', which is defined as
   id-simple   := element
               ; "." reserved as hierarchy delimiter
   element     := 1*octet-sim

1*octet-sim requires at least one octet-sim.

The other comments are all addressed in the next patch.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v3

With comments addressed.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed 8ffb3d8bd8b8 to trunk. I refactored the tests so that they work correctly if Ogg and/or Wave backends are disabled.

This API needs to be documented. It matches the current spec.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 352960 [details] [diff] [review]
fix v3

We should take this for 1.9.1 IMHO. I didn't make the bug blocking, since it's a newly-specced <video>/<audio> API feature and we could be forgiven for not supporting it. However, it's important to help people write script libraries that use <video>/<audio> and want to fall back to Flash or something else if their preferred formats are not supported directly by the browser.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 352960 [details] [diff] [review]
fix v3

a191=beltzner

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed f8f9a28ff1ed to 1.9.1

Comment 10

[Eric Shepherd [:sheppy]]

Documented here:

https://developer.mozilla.org/En/NsIDOMHTMLMediaElement#canPlayType()

Comment 11

[Aaron Train [:aaronmt]]

Verified FIXED on latest 1.9.1 branch, trunk and b99

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090608 Minefield/3.6a1pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b99) Gecko/20090604 Firefox/3.5b99

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090608 Shiretoko/3.5pre