Closed Bug 985496 Opened 6 years ago Closed 6 years ago

[Camera][Gecko] Return better error messages to DOM

Categories

(Firefox OS Graveyard :: Gaia::Camera, defect)

ARM
Gonk (Firefox OS)
defect
Not set

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)

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"
Whiteboard: [priority]
Assignee: nobody → mhabicher
Status: NEW → ASSIGNED
Attachment #8405542 - Flags: review?(dhylands)
Attachment #8405542 - Flags: feedback?(jdarcangelo)
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
Attachment #8405542 - Flags: review?(dhylands) → review+
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+
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+
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+
Fix test bustage. Now all I need is an unCLOSED tree.
Attachment #8408493 - Attachment is obsolete: true
Attachment #8414726 - Flags: review+
Flags: needinfo?(mhabicher)
https://hg.mozilla.org/mozilla-central/rev/c26966635e9c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
See Also: → 1111984
You need to log in before you can comment on or make changes to this bug.