A lot of camera errors invoke the CameraErrorCallback with the unhelpful string "FAILURE". We can and should make this more helpful.

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"

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 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

Rebased; carrying r+ forward. try-server push: https://tbpl.mozilla.org/?tree=Try&rev=150941c3ba0d

Actually attach the right patch that incorporates review feedback. Should not affect try-server results.

Fix fallback implementation breakages. try-server push: https://tbpl.mozilla.org/?tree=Try&rev=8df2432d6d5e

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

Fix test bustage. Now all I need is an unCLOSED tree.