Closed
Bug 985496
Opened 10 years ago
Closed 10 years ago
[Camera][Gecko] Return better error messages to DOM
Categories
(Firefox OS Graveyard :: Gaia::Camera, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.0 S1 (9may)
People
(Reporter: mikeh, Assigned: mikeh)
References
Details
(Keywords: dev-doc-needed, Whiteboard: [priority])
Attachments
(1 file, 5 obsolete files)
105.94 KB,
patch
|
mikeh
:
review+
|
Details | Diff | Splinter Review |
A lot of camera errors invoke the CameraErrorCallback with the unhelpful string "FAILURE". We can and should make this more helpful.
Assignee | ||
Comment 1•10 years ago
|
||
This will involve some rationalizing of the error codes returned throughout the camera stack. Some notes (this list is by no means exhaustive): - GonkCameraControl::*Impl() functions should all return: - _NOT_INITIALIZED if the camera hardware is closed - _INVALID_ARG on invalid arguments - _NOT_IMPLEMENTED on features that aren't implemented (this should never happen) - _OUT_OF_MEMORY on OoM conditions (rare) - _NOT_AVAILABLE - _FAILURE on other, general errors - GonkCameraParameters::Set() and ::Get() should return _NOT_IMPLEMENTED if the key value is invalid - they currently return _NOT_AVAILABLE, which is not an accurate description of the error - Fallback*::*() will return _NOT_IMPLEMENTED - some of these currently return _FAILURE - CameraCapabilities::Populate() returns _INVALID_ARG if called without an ICameraControl* - otherwise, it should always return NS_OK - failing to get a recorder-profiles object should be reflected in that capability being missing CameraErrorCallback will see these as: - "not-initialized" - "invalid-argument" - "not-implemented" - "out-of-memory" - "not-available" - "general-failure"
Updated•10 years ago
|
Whiteboard: [priority]
Assignee | ||
Comment 2•10 years ago
|
||
Assignee: nobody → mhabicher
Status: NEW → ASSIGNED
Attachment #8405542 -
Flags: review?(dhylands)
Attachment #8405542 -
Flags: feedback?(jdarcangelo)
Assignee | ||
Comment 3•10 years ago
|
||
This version of the patch applies on top of the one in bug 986024. If you don't have that patch applied, use v1 instead.
Comment 4•10 years ago
|
||
Comment on attachment 8405542 [details] [diff] [review] Rationalize CameraControl error reporting, v1 Review of attachment 8405542 [details] [diff] [review]: ----------------------------------------------------------------- Just some minor comments... ::: dom/camera/CameraControlImpl.cpp @@ +278,1 @@ > const char* context[] = { If this table is supposed to match the UserContext enum, it doesn't. For example, FaceDetection exists in the enum and not here. For that reason, I prefer to see an array of structs, which includes the enum value and the char *. You can then add an assert that context[index].enumVal == index to detect that this table is out of sync with the enum. @@ +290,2 @@ > }; > + if (static_cast<unsigned int>(aContext) < sizeof(context) / sizeof(context[0])) { nit: Since sizeof returns size_t, you should use size_t in the static cast rather than unsigned int @@ +294,2 @@ > } else { > + DOM_CAMERA_LOGE("CameraControlImpl::OnUserError : aContext=%u, aError=0x%x\n", nit: Since aContext is an enum, shouldn't this be using %d ? @@ +314,5 @@ > +#ifdef PR_LOGGING > + const char* context[] = { > + "System Service" > + }; > + if (static_cast<unsigned int>(aContext) < sizeof(context) / sizeof(context[0])) { nit: Since sizeof returns size_t, you should use size_t in the static cast rather than unsigned int @@ +318,5 @@ > + if (static_cast<unsigned int>(aContext) < sizeof(context) / sizeof(context[0])) { > + DOM_CAMERA_LOGW("CameraControlImpl::OnSystemError : aContext='%s' (%u), aError=0x%x\n", > + context[aContext], aContext, aError); > + } else { > + DOM_CAMERA_LOGE("CameraControlImpl::OnSystemError : aContext=%u, aError=0x%x\n", nit: Since aContext is an enum, shouldn't this be using %d ? @@ +352,5 @@ > > nsresult rv = RunImpl(); > if (NS_FAILED(rv)) { > + nsCString msg; > + msg.AppendPrintf("Camera control API(%d) failed with 0x%x", mContext, rv); You could also use: nsPrintfCString msg("Camera control API(%d) failed with 0x%x", mContext, rv); @@ +376,5 @@ > + return NS_OK; > + } > + > + nsCString msg; > + msg.AppendPrintf("Failed to dispatch camera control message (0x%x)", rv); nit: Use nsPrintfCString ::: dom/camera/DOMCameraControl.cpp @@ +1338,5 @@ > + nsString error; > + if (mReportExtendedErrors) { > + switch (aError) { > + case NS_ERROR_INVALID_ARG: > + error = MOZ_UTF16("InvalidArgument"); Why use MOZ_UTF16 rather than NS_LITERAL_STRING ? @@ +1364,5 @@ > + > + default: > + { > + nsCString msg; > + msg.AppendPrintf("Reporting aError=0x%x as generic\n", aError); nit: use nsPrintfCString ::: dom/camera/GonkCameraControl.h @@ +54,5 @@ > void OnTakePictureError(); > void OnNewPreviewFrame(layers::TextureClient* aBuffer); > void OnRecorderEvent(int msg, int ext1, int ext2); > + void OnSystemError(CameraControlListener::SystemContext aWhere, nsresult aError); > + nit: trailing space
Attachment #8405542 -
Flags: review?(dhylands) → review+
Assignee | ||
Comment 5•10 years ago
|
||
Rebased; carrying r+ forward. try-server push: https://tbpl.mozilla.org/?tree=Try&rev=150941c3ba0d
Attachment #8405542 -
Attachment is obsolete: true
Attachment #8405642 -
Attachment is obsolete: true
Attachment #8405542 -
Flags: feedback?(jdarcangelo)
Attachment #8408456 -
Flags: review+
Assignee | ||
Comment 6•10 years ago
|
||
Actually attach the right patch that incorporates review feedback. Should not affect try-server results.
Attachment #8408456 -
Attachment is obsolete: true
Attachment #8408467 -
Flags: review+
Assignee | ||
Comment 7•10 years ago
|
||
Fix fallback implementation breakages. try-server push: https://tbpl.mozilla.org/?tree=Try&rev=8df2432d6d5e
Attachment #8408467 -
Attachment is obsolete: true
Attachment #8408493 -
Flags: review+
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/a73dece39b01
Backed out this in http://hg.mozilla.org/integration/b2g-inbound/rev/5d6a3571f1ab and also bug 987954 for mochitest-5 orange: https://tbpl.mozilla.org/php/getParsedLog.php?id=38522732&tree=B2g-Inbound
Flags: needinfo?(mhabicher)
Assignee | ||
Comment 10•10 years ago
|
||
Fix test bustage. Now all I need is an unCLOSED tree.
Attachment #8408493 -
Attachment is obsolete: true
Attachment #8414726 -
Flags: review+
Flags: needinfo?(mhabicher)
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/c26966635e9c
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c26966635e9c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
Assignee | ||
Updated•10 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•