Closed Bug 1099390 Opened 5 years ago Closed 5 years ago

[Camera][Gecko] Add reason to onClosed callback

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S1 (5dec)

People

(Reporter: mikeh, Assigned: mikeh)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [ft:media] [2.2 target])

Attachments

(1 file, 7 obsolete files)

e.g. "Released", "SystemFailure"
See Also: → 1097835
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.
Initialize the 'close' event with a correct value.
Attachment #8524847 - Attachment is obsolete: true
Attachment #8524847 - Flags: review?(aosmond)
Attachment #8524851 - Flags: review?(aosmond)
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)
Target Milestone: --- → 2.1 S9 (21Nov)
Attached patch DOM-specific changes, v2.2 (obsolete) — Splinter Review
Attachment #8524886 - Flags: review?(bzbarsky)
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+
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+
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
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.