[Camera][Gecko] Add reason to onClosed callback

RESOLVED FIXED in 2.2 S1 (5dec)

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mikeh, Assigned: mikeh)

Tracking

({dev-doc-needed})

unspecified
2.2 S1 (5dec)
ARM
Gonk (Firefox OS)
dev-doc-needed
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ft:media] [2.2 target])

Attachments

(1 attachment, 7 obsolete attachments)

e.g. "Released", "SystemFailure"
See Also: → bug 1097835
Created attachment 8524688 [details] [diff] [review]
Add 'reason' to onClosed callback/close event, v1

try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=aa41ab9ea3d2
Assignee: nobody → mhabicher
Status: NEW → ASSIGNED
Attachment #8524688 - Flags: review?(aosmond)
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.
(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.
Created attachment 8524847 [details] [diff] [review]
Add 'reason' to onClosed callback/close event, v2

try repush: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6accfb317326
Attachment #8524688 - Attachment is obsolete: true
Attachment #8524688 - Flags: review?(aosmond)
Attachment #8524847 - Flags: review?(aosmond)
Created attachment 8524851 [details] [diff] [review]
Add 'reason' to onClosed callback/close event, v2.1

Initialize the 'close' event with a correct value.
Attachment #8524847 - Attachment is obsolete: true
Attachment #8524847 - Flags: review?(aosmond)
Attachment #8524851 - Flags: review?(aosmond)
Created attachment 8524856 [details] [diff] [review]
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
Attachment #8524851 - Attachment is obsolete: true
Attachment #8524851 - Flags: review?(aosmond)
Attachment #8524856 - Flags: review?(aosmond)

Updated

4 years ago
Target Milestone: --- → 2.1 S9 (21Nov)
Created attachment 8524886 [details] [diff] [review]
DOM-specific changes, v2.2
Attachment #8524886 - Flags: review?(bzbarsky)
Created attachment 8525014 [details] [diff] [review]
Add 'reason' to onClosed callback/close event, v2.3

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b281161526fa
Attachment #8525014 - Flags: review?(aosmond)
Attachment #8524856 - Attachment is obsolete: true
Attachment #8524856 - Flags: review?(aosmond)
Comment on attachment 8524886 [details] [diff] [review]
DOM-specific changes, v2.2

r=me
Attachment #8524886 - Flags: review?(bzbarsky) → review+
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?
Attachment #8525014 - Flags: review?(aosmond) → review+
Created attachment 8526135 [details] [diff] [review]
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
Attachment #8524886 - Attachment is obsolete: true
Attachment #8525014 - Attachment is obsolete: true
Attachment #8526135 - Flags: review+
Created attachment 8526253 [details] [diff] [review]
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
Attachment #8526135 - Attachment is obsolete: true
Attachment #8526253 - Flags: review+

Updated

4 years ago
Whiteboard: [ft:media] [2.2 target]
Target Milestone: 2.1 S9 (21Nov) → 2.2 S1 (5dec)
https://hg.mozilla.org/mozilla-central/rev/63b54f5f6877
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.