Closed
Bug 469247
Opened 12 years ago
Closed 12 years ago
Implement canPlayType
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
VERIFIED
FIXED
People
(Reporter: roc, Assigned: roc)
Details
(Keywords: dev-doc-complete, verified1.9.1)
Attachments
(1 file, 2 obsolete files)
24.77 KB,
patch
|
roc
:
review+
roc
:
superreview+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
This is pretty easy.
Assignee | ||
Comment 1•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Attachment #352670 -
Flags: review?(chris.double)
Assignee | ||
Updated•12 years ago
|
Whiteboard: [needs review]
Updated•12 years ago
|
Attachment #352670 -
Flags: review?(chris.double) → review+
Assignee | ||
Comment 2•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Attachment #352687 -
Flags: review+
![]() |
||
Updated•12 years ago
|
Attachment #352687 -
Flags: superreview?(bzbarsky)
Attachment #352687 -
Flags: superreview+
Attachment #352687 -
Flags: review?(bzbarsky)
Attachment #352687 -
Flags: review+
![]() |
||
Comment 3•12 years ago
|
||
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).
Assignee | ||
Updated•12 years ago
|
Whiteboard: [needs review] → [needs landing]
Assignee | ||
Comment 4•12 years ago
|
||
(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.
Assignee | ||
Comment 5•12 years ago
|
||
With comments addressed.
Attachment #352687 -
Attachment is obsolete: true
Attachment #352960 -
Flags: superreview+
Attachment #352960 -
Flags: review+
Assignee | ||
Comment 6•12 years ago
|
||
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
Closed: 12 years ago
Flags: in-testsuite+
Keywords: dev-doc-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 approval]
Assignee | ||
Comment 7•12 years ago
|
||
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?
Updated•12 years ago
|
Attachment #352960 -
Flags: approval1.9.1? → approval1.9.1+
Comment 8•12 years ago
|
||
Comment on attachment 352960 [details] [diff] [review] fix v3 a191=beltzner
Assignee | ||
Updated•12 years ago
|
Whiteboard: [needs 191 approval] → [needs 191 landing]
Assignee | ||
Comment 9•12 years ago
|
||
Pushed f8f9a28ff1ed to 1.9.1
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
Comment 10•12 years ago
|
||
Documented here: https://developer.mozilla.org/En/NsIDOMHTMLMediaElement#canPlayType()
Keywords: dev-doc-needed → dev-doc-complete
Comment 11•12 years ago
|
||
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
Keywords: fixed1.9.1 → verified1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•