1099390 - [Camera][Gecko] Add reason to onClosed callback

Status: RESOLVED FIXED

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

Description

[Mike Habicher [:mikeh] (high bugzilla latency)]

e.g. "Released", "SystemFailure"

Comment 1

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Add 'reason' to onClosed callback/close event, v1

try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=aa41ab9ea3d2

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8524688 [details] [diff] [review]
Add 'reason' to onClosed callback/close event, v1

Review of attachment 8524688 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good aside from that below. CameraClosedEvent is the only thing I have reservations about.

::: dom/camera/DOMCameraControl.cpp
@@ +1276,5 @@
> +          default:
> +            DOM_CAMERA_LOGE("Unhandled hardware close reason, 0x%x\n", aReason);
> +            MOZ_ASSERT_UNREACHABLE("Unanticipated reason for hardware close");
> +            return;
> +        }

While it is consistent with the callback, this would be more consistent with events to make this an ErrorEvent + error code directly instead of a custom event.

@@ +1603,5 @@
> +          cb->Call(ignored);
> +        }
> +        if (promise) {
> +          promise->MaybeResolve(JS::UndefinedHandleValue);
> +        }

Elsewhere we resolve the problem first where possible. I think it should happen before the callback for consistency.

::: dom/camera/test/test_bug1099390.html
@@ +82,5 @@
> +          resolve();
> +        }
> +      };
> +
> +      cam.release(onSuccess, onError).then(onResolve, onError);

None of the event/promise based tests actually use the callbacks. The idea was that we could just delete the test/callback/* when the APIs were removed.

@@ +101,5 @@
> +        }; // onPreviewStateChange
> +        cam.addEventListener('previewstatechange', onPreviewStateChange);
> +      }; // onSuccess()
> +
> +      navigator.mozCameras.getCamera(whichCamera, config).then(onSuccess, onError);

Nice use of promises, but you never reject any of the promises in the case of an error, only fail the test via ok(false) :). It should work fine, but I'm not sure if that was an oversight.

::: dom/camera/test/test_camera_hardware_init_failure.html
@@ +131,5 @@
> +          );
> +        };
> +
> +        d.camera.onClosed = onClosedCallback;
> +        d.camera.addEventListener('close', onClosedEvent);

Similar comments here with the callback dependencies in the event tests.

::: dom/webidl/CameraControl.webidl
@@ +255,5 @@
>    /* the event dispatched when the camera hardware is closed
>       by the underlying framework, e.g. when another app makes a more
>       recent call to get the camera.
>  
>       contains no event-specific data. */

Update comments for onclose event to say it includes error information.

Comment 3

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Andrew Osmond [:aosmond] from comment #2)

> ::: dom/camera/DOMCameraControl.cpp
> @@ +1276,5 @@
> > +          default:
> > +            DOM_CAMERA_LOGE("Unhandled hardware close reason, 0x%x\n", aReason);
> > +            MOZ_ASSERT_UNREACHABLE("Unanticipated reason for hardware close");
> > +            return;
> > +        }
> 
> While it is consistent with the callback, this would be more consistent with
> events to make this an ErrorEvent + error code directly instead of a custom
> event.

As we discussed on IRC, this isn't necessarily an error condition -- it's a status update.

> @@ +101,5 @@
> > +        }; // onPreviewStateChange
> > +        cam.addEventListener('previewstatechange', onPreviewStateChange);
> > +      }; // onSuccess()
> > +
> > +      navigator.mozCameras.getCamera(whichCamera, config).then(onSuccess, onError);
> 
> Nice use of promises, but you never reject any of the promises in the case
> of an error, only fail the test via ok(false) :). It should work fine, but
> I'm not sure if that was an oversight.

It's on purpose. The test will eventually timeout and fail.

Comment 4

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Add 'reason' to onClosed callback/close event, v2

try repush: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6accfb317326

Comment 5

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Add 'reason' to onClosed callback/close event, v2.1

Initialize the 'close' event with a correct value.

Comment 6

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Add 'reason' to onClosed callback/close event, v2.2

Add missing equals sign. :/

try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7be163ba6117

Comment 7

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: DOM-specific changes, v2.2

Comment 8

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Add 'reason' to onClosed callback/close event, v2.3

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b281161526fa

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8524886 [details] [diff] [review]
DOM-specific changes, v2.2

r=me

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8525014 [details] [diff] [review]
Add 'reason' to onClosed callback/close event, v2.3

Review of attachment 8525014 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with below resolved or rebutted.

::: dom/camera/DOMCameraControl.cpp
@@ +1275,5 @@
> +
> +          default:
> +            DOM_CAMERA_LOGE("Unhandled hardware close reason, 0x%x\n", aReason);
> +            MOZ_ASSERT_UNREACHABLE("Unanticipated reason for hardware close");
> +            return;

If by some chance we do make a mistake and it gets into the customer's hands with an unknown error code, surely the sane thing to do is to still notify the app of an error rather than silently (from the user perspective) drop it?

::: dom/camera/GonkCameraControl.cpp
@@ +1853,5 @@
> +
> +  nsresult rv = StopInternal();
> +  if (rv != NS_ERROR_NOT_INITIALIZED) {
> +    OnHardwareStateChange(CameraControlListener::kHardwareClosed, NS_OK);
> +  }

I am probably missing something obvious here, but if we returned NS_ERROR_NOT_INITIALIZED to OnHardwareStateChange, couldn't we handle it the same as NS_OK in that function, and then we wouldn't need a special case in DOMCameraControl::OnUserError to handle this?

::: dom/camera/test/callback/test_bug1099390.html
@@ +62,5 @@
> +        SimpleTest.finish();
> +      }
> +    };
> +
> +    cam.addEventListener('close', onClosed);

Shouldn't this use the onClosed callback instead of the event?

Comment 11

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Add a 'reason' to onClosed callback/close event, v2.4 r=aosmond,bz

Incorporate review feedback, carry r+en forward.

re-try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=64da662e827c

Comment 12

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Add a 'reason' to onClosed callback/close event, v2.5 r=aosmond,bz

Fix typo in test script.

re-try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=19c9ff9e8b45

Comment 13

[Mike Habicher [:mikeh] (high bugzilla latency)]

https://hg.mozilla.org/integration/b2g-inbound/rev/63b54f5f6877

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/63b54f5f6877