Closed
Bug 712134
Opened 12 years ago
Closed 12 years ago
Log messages to console explaining media load algorithm failure
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: cpearce, Assigned: cpearce)
References
Details
Attachments
(1 file, 2 obsolete files)
10.31 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
Pursuant to our conversation in bug 708150, we should log messages to the webconsole explaining why media resource loads are failing to make debugging load failures when loading media from <source> children easier.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #582971 -
Flags: review?(roc)
Assignee | ||
Comment 2•12 years ago
|
||
Test builds at: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/cpearce@mozilla.com-fc49b30b1ffa/ Test page: http://pearce.org.nz/video/error.html
Comment on attachment 582971 [details] [diff] [review] Patch v1: Add logging Review of attachment 582971 [details] [diff] [review]: ----------------------------------------------------------------- I think a helper function is worth having here, say nsHTMLMediaElement::ReportLoadError(char* aMsg, const PRUnichar* aParams = nsnull, PRUint32 aParamCount = 0). Let me bikeshed the messages a bit... ::: content/html/content/src/nsHTMLMediaElement.cpp @@ +313,5 @@ > + nsCOMPtr<nsIHttpChannel> hc = do_QueryInterface(aRequest); > + if (hc) { > + PRUint32 responseStatus = 0; > + hc->GetResponseStatus(&responseStatus); > + if (responseStatus == 404) { Call GetRequestSucceeded instead of checking 404. @@ +318,5 @@ > + element->NotifyLoadError(); > + if (Preferences::GetBool("media.log_load_failures")) { > + nsCAutoString src; > + element->GetCurrentSpec(src); > + NS_ConvertUTF8toUTF16 srcUTF16(src); Just call element->GetCurrentSrc(). @@ +325,5 @@ > + "DOM", > + element->OwnerDoc(), > + nsContentUtils::eDOM_PROPERTIES, > + "MediaLoad404", > + params, ArrayLength(params)); Pass the response status as an extra parameter @@ +766,5 @@ > > // Must have src attribute. > nsAutoString src; > if (!child->GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) { > DispatchAsyncSourceError(child); Why not add a message here? "<source> element missing src attribute" or something like that. @@ +841,5 @@ > + "DOM", > + OwnerDoc(), > + nsContentUtils::eDOM_PROPERTIES, > + "MediaLoadSuspendedPreloadNone", > + params, ArrayLength(params)); I don't think preload="none" needs a message. @@ +2022,5 @@ > if (!decoder) { > + if (Preferences::GetBool("media.log_load_failures")) { > + nsCAutoString src; > + GetCurrentSpec(src); > + NS_ConvertUTF8toUTF16 srcUTF16(src); GetCurrentSrc @@ +2204,5 @@ > { > + if (Preferences::GetBool("media.log_load_failures")) { > + nsCAutoString src; > + GetCurrentSpec(src); > + NS_ConvertUTF8toUTF16 srcUTF16(src); GetCurrentSrc ::: dom/locales/en-US/chrome/dom/dom.properties @@ +137,5 @@ > WithCredentialsSyncXHRWarning=Use of XMLHttpRequest's withCredentials attribute is no longer supported in the synchronous mode in window context. > JSONCharsetWarning=An attempt was made to declare a non-UTF-8 encoding for JSON retrieved using XMLHttpRequest. Only UTF-8 is supported for decoding JSON. > +MediaLoadExhaustedCandidates=Loading of a media element paused because all candidate resources failed to load. > +# LOCALIZATION NOTE: %S is the URL of the media resource which failed to load. > +MediaLoad404=Media load of %S failed, file not found. This should handle any HTTP failure, so needs to change. e.g. Media resource %1$S failed to load (HTTP load failed with status %2$d). @@ +139,5 @@ > +MediaLoadExhaustedCandidates=Loading of a media element paused because all candidate resources failed to load. > +# LOCALIZATION NOTE: %S is the URL of the media resource which failed to load. > +MediaLoad404=Media load of %S failed, file not found. > +# LOCALIZATION NOTE: %S is the URL of the media resource which failed to load. > +MediaLoadInvalidURI=Media load of %S failed, not a valid URI. Media resource %S failed to load (invalid URI). @@ +141,5 @@ > +MediaLoad404=Media load of %S failed, file not found. > +# LOCALIZATION NOTE: %S is the URL of the media resource which failed to load. > +MediaLoadInvalidURI=Media load of %S failed, not a valid URI. > +# LOCALIZATION NOTE: %1$S is the URL of the media resource which failed to load, %2$S is the resource's format/codec type (basically the file type, e.g. MP4,AVI,WMV,MOV etc). > +MediaLoadUnsupportedType=Media load of %1$S failed, resource has an unsupported "type" attribute of %2$S. Media resource %1$S was not loaded (specified type "%2$S" is not supported). @@ +143,5 @@ > +MediaLoadInvalidURI=Media load of %S failed, not a valid URI. > +# LOCALIZATION NOTE: %1$S is the URL of the media resource which failed to load, %2$S is the resource's format/codec type (basically the file type, e.g. MP4,AVI,WMV,MOV etc). > +MediaLoadUnsupportedType=Media load of %1$S failed, resource has an unsupported "type" attribute of %2$S. > +# LOCALIZATION NOTE: %1$S is the URL of the media resource which failed to load, %2$S is the MIME type being sent by the web server. > +MediaLoadUnsupportedMimeType=Media load of %1$S failed, resource is being served with unsupported "Content-Type" HTTP header of "%2$S". Media resource %1$S failed to load (HTTP Content-Type "%2$S" is not supported). @@ +145,5 @@ > +MediaLoadUnsupportedType=Media load of %1$S failed, resource has an unsupported "type" attribute of %2$S. > +# LOCALIZATION NOTE: %1$S is the URL of the media resource which failed to load, %2$S is the MIME type being sent by the web server. > +MediaLoadUnsupportedMimeType=Media load of %1$S failed, resource is being served with unsupported "Content-Type" HTTP header of "%2$S". > +# LOCALIZATION NOTE: %S is the URL of the media resource whose load is being suspended. > +MediaLoadSuspendedPreloadNone=Media load of %S suspended because element is preload="none". Remove @@ +147,5 @@ > +MediaLoadUnsupportedMimeType=Media load of %1$S failed, resource is being served with unsupported "Content-Type" HTTP header of "%2$S". > +# LOCALIZATION NOTE: %S is the URL of the media resource whose load is being suspended. > +MediaLoadSuspendedPreloadNone=Media load of %S suspended because element is preload="none". > +# LOCALIZATION NOTE: %S is the URL of the media resource which failed to load because of error in decoding. > +MediaLoadDecodeError=Media load of %S failed due to error decoding resource. Media resource %S could not be decoded. ::: modules/libpref/src/init/all.js @@ +195,5 @@ > // Whether to autostart a media element with an |autoplay| attribute > pref("media.autoplay.enabled", true); > > +// Log failures in the media load algorithm to the web console. > +pref("media.log_load_failures", true); Let's just use a new category. If it produces too much spew then the Web Console people can filter it out.
Assignee | ||
Comment 4•12 years ago
|
||
* Removed pref. * Removed preload="none" message. * Moved messages to "Media" category. * Added helper nsHTMLMediaElement::ReportLoadError(). * Report HTTP status code on request failure. * Added warning for missing @src on <source>. * Took some of the bikeshed text changes suggestions on board, but I switched to using two sentences (instead of 1 + $specific_error_message in parentheses) and inverted the clause order. i.e. "$specific_failure. Load of media resource $src failed". This seems to be the convention used by existing warnings (at least in layout anyway), and I think it's weird to have parentheses in the error messages as suggested. The $specific_failure text is what's important/most useful!
Attachment #582971 -
Attachment is obsolete: true
Attachment #582971 -
Flags: review?(roc)
Attachment #583612 -
Flags: review?(roc)
Assignee | ||
Comment 5•12 years ago
|
||
Oops, forgot to remove the pref from all.js. :/
Attachment #583612 -
Attachment is obsolete: true
Attachment #583612 -
Flags: review?(roc)
Attachment #583613 -
Flags: review?(roc)
Comment on attachment 583613 [details] [diff] [review] Patch v3 Review of attachment 583613 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/public/nsHTMLMediaElement.h @@ +360,5 @@ > + * aMsg is the localized message identifier, aParams is the parameters to > + * be substituted into the localized message, and aParamCount is the number > + * of parameters in aParams. > + */ > + void ReportLoadError(char* aMsg, const char*
Attachment #583613 -
Flags: review?(roc) → review+
Assignee | ||
Comment 7•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8de05d965033
Target Milestone: --- → mozilla12
Comment 8•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8de05d965033
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•