870678 - convert DOMCameraManager to webidl

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Boris Zbarsky [:bzbarsky]]

You probably want to do this on top of https://bugzilla.mozilla.org/attachment.cgi?id=747721

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: bug 870678 - convert CameraManager to webidl

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: remove nsIDOMCameraManager

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 749746 [details] [diff] [review]
bug 870678 - convert CameraManager to webidl

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

::: dom/camera/DOMCameraManager.cpp
@@ +142,5 @@
> +                              const Optional<nsICameraErrorCallback*>& onError,
> +                             ErrorResult& aRv)
> +{
> +  uint32_t cameraId = 0;  // back (or forward-facing) camera by default
> +  mozilla::idl::CameraSelector selector;

Why not move the CameraSelector dictionary to WebIDL a well? Then you don't need to mess with jsvals at all.

::: dom/camera/GonkCameraManager.cpp
@@ +136,5 @@
> +
> +    if (!cameraName.EqualsLiteral("back")) {
> +      aList.InsertElementAt(0, NS_ConvertUTF8toUTF16(cameraName).get());
> +    } else if (cameraName.EqualsLiteral("front")) {
> +      aList.SetLength(2);

I think we have EnsureLengthAtLeast or something like that

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to :Ms2ger from comment #4)
> Comment on attachment 749746 [details] [diff] [review]
> bug 870678 - convert CameraManager to webidl
> 
> Review of attachment 749746 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/camera/DOMCameraManager.cpp
> @@ +142,5 @@
> > +                              const Optional<nsICameraErrorCallback*>& onError,
> > +                             ErrorResult& aRv)
> > +{
> > +  uint32_t cameraId = 0;  // back (or forward-facing) camera by default
> > +  mozilla::idl::CameraSelector selector;
> 
> Why not move the CameraSelector dictionary to WebIDL a well? Then you don't
> need to mess with jsvals at all.

I was planning to in another patch, but I didn't want to write one massive convert all of dom/camera patch :)

> ::: dom/camera/GonkCameraManager.cpp
> @@ +136,5 @@
> > +
> > +    if (!cameraName.EqualsLiteral("back")) {
> > +      aList.InsertElementAt(0, NS_ConvertUTF8toUTF16(cameraName).get());
> > +    } else if (cameraName.EqualsLiteral("front")) {
> > +      aList.SetLength(2);
> 
> I think we have EnsureLengthAtLeast or something like that

yeah, I think so and that would be better.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 749746 [details] [diff] [review]
bug 870678 - convert CameraManager to webidl

GetListOfCameras looks wrong (though perhaps no more wrong than what's there).  Please rebase on top of the patch in bug 872122 and see what the code there looks like?

>+++ b/dom/webidl/CameraManager.webidl
>+  void getCamera(any options, GetCameraCallback callback, optional
>+CameraErrorCallback errorCallback);

I suggest line-breaking before the "optional" and fixing the indentation.

>+  [Throws]
>+sequence<DOMString> getListOfCameras();

Again, fix the indentation (two more spaces before "sequence").

r- because I'd like to see the fixed getListOfCameras.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 749747 [details] [diff] [review]
remove nsIDOMCameraManager

>+++ b/dom/webidl/CameraManager.webidl

>+            camera: front

I know you just copied this, but it should be: 

  camera: "front"

with the quotes around "front".

>+    /* return a JSON array of camera   identifiers, e.g.

s/JSON //, since it's just an array.

r=me

Comment 8

[Trevor Saunders (:tbsaunde)]

Attachment: convert cameraSelector dict to webidl

Comment 9

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

The patch in bug 872122 has landed on inbound.

Comment 10

[Trevor Saunders (:tbsaunde)]

Attachment: bug 870678 - convert CameraManager to webidl

I believe the TArray stuff is correct, but its kind of scary and I have no idea what tests this stuff if anything so certainly worth a close look

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 750913 [details] [diff] [review]
bug 870678 - convert CameraManager to webidl

>+++ b/dom/camera/DOMCameraManager.cpp
>+nsDOMCameraManager::GetCamera(JSContext* aCx, const JS::Value aOptions,
>+                             ErrorResult& aRv)

The ErrorResult line needs to be indented by one more space.

>+++ b/dom/camera/GonkCameraManager.cpp
>+nsDOMCameraManager::GetListOfCameras(nsTArray<nsString>& aList, ErrorResult& aRv)
>+  if (count <= 0) {
>+    aRv.Throw(NS_ERROR_NOT_AVAILABLE);

We used to return an empty list if count < 1.  Why the behavior change?

>+  aList.SetLength(2);

This lost the comments about _why_ we want to do that.

>+    if (!cameraName.EqualsLiteral("back")) {

The sense of this of this test is reversed.  Please fix that.

>+      aList[0] = NS_ConvertUTF8toUTF16(cameraName).get();

CopyUTF8toUTF16(cameraName, aList[0]);

>+      aList[1] = NS_ConvertUTF8toUTF16(cameraName).get();

And similar here.

>+      aList.InsertElementAt(extraIdx, NS_ConvertUTF8toUTF16(cameraName).get());

CopyUTF8toUTF16(cameraName, *aList.InsertElementAt(extraIdx));

>+  if (gotFront) {
>+    aList.RemoveElementAt(1);
>+  }
>+  if (gotBack) {
>+    aList.RemoveElementAt(0);
>+  }

This part looks wrong to me.  Please fix it to actually do the right thing (like not lose the front and back cameras).  Are those boolean tests just reversed, perhaps?

>+++ b/dom/webidl/CameraManager.webidl

Please do fix the indentation problems from the previous review.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 750369 [details] [diff] [review]
convert cameraSelector dict to webidl

So this applies on top of the other two patches, right?

r=me

Comment 13

[Trevor Saunders (:tbsaunde)]

Attachment: bug 870678 - convert CameraManager to webidl

sorry about the last patch eing totally crazy I think this one is a bit better.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 754988 [details] [diff] [review]
bug 870678 - convert CameraManager to webidl

r=me.  Sorry for the lag...

Comment 15

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c878e510f42a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3565f3f92904
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/831fa9c0c20b

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c878e510f42a
https://hg.mozilla.org/mozilla-central/rev/3565f3f92904
https://hg.mozilla.org/mozilla-central/rev/831fa9c0c20b