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

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

(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 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 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

The patch in bug 872122 has landed on inbound.

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 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 on attachment 750369 [details] [diff] [review] convert cameraSelector dict to webidl So this applies on top of the other two patches, right? r=me

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

Comment on attachment 754988 [details] [diff] [review] bug 870678 - convert CameraManager to webidl r=me. Sorry for the lag...

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

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