Status

()

defect
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

({dev-doc-complete, verified1.9.1})

Trunk
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Posted patch fix (obsolete) — Splinter Review
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...
Attachment #352670 - Flags: superreview?(bzbarsky)
Attachment #352670 - Flags: review?(bzbarsky)
Whiteboard: [needs review]

Updated

11 years ago
Attachment #352670 - Flags: review?(chris.double) → review+
Posted patch fix v2 (obsolete) — Splinter Review
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
Attachment #352670 - Attachment is obsolete: true
Attachment #352687 - Flags: superreview?(bzbarsky)
Attachment #352687 - Flags: review?(bzbarsky)
Attachment #352670 - Flags: superreview?(bzbarsky)
Attachment #352670 - Flags: review?(bzbarsky)
Attachment #352687 - Flags: superreview?(bzbarsky)
Attachment #352687 - Flags: superreview+
Attachment #352687 - Flags: review?(bzbarsky)
Attachment #352687 - Flags: review+
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).
Whiteboard: [needs review] → [needs landing]
(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.
Posted patch fix v3Splinter Review
With comments addressed.
Attachment #352687 - Attachment is obsolete: true
Attachment #352960 - Flags: superreview+
Attachment #352960 - Flags: review+
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.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite+
Keywords: dev-doc-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 approval]
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.
Attachment #352960 - Flags: approval1.9.1?
Attachment #352960 - Flags: approval1.9.1? → approval1.9.1+
Whiteboard: [needs 191 approval] → [needs 191 landing]
Pushed f8f9a28ff1ed to 1.9.1
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.