aosmond suggested this is idea in IRC while discussing bug 1051774 -- creating a bug to track it. Basically, expose an interface that allows AOSP ICamera methods to be passed to JS, so that the test script can inspect and manipulate settings that are otherwise not available on the emulator. The gist is something like: ---- .webidl [pref="camera.control.test.js-hooks.enabled"] callback CameraControlHardwareTakePicture = int (); [pref="camera.control.test.js-hooks.enabled", Constructor(CameraControl)] interface CameraControlHardware { attribute CameraControlHardwareTakePicture? takePictureHook; }; ---- .js var test = new CameraControlHardware(camera); test.takePictureHook = function() { // do something return 0; // OK }; camera.takePicture(...); It may be desirable to have pre- and post-method hooks, or to provide a way to invoke the original AOSP ICamera method from the callback: ---- .webidl [pref="camera.control.test.js-hooks.enabled"] callback CameraControlHardwareTakePicture = int (); [pref="camera.control.test.js-hooks.enabled", Constructor(CameraControl)] interface CameraControlHardware { attribute CameraControlHardwareTakePicture? takePictureHook; int takePictureOriginal(); }; ---- .js var test = new CameraControlHardware(camera); test.takePictureHook = function() { // do something before var rv = takePictureOriginal(); // do something after return rv; }; camera.takePicture(...);

Comment on attachment 8563475 [details] [diff] [review] Part 1. Fix clearing of camera preferences. Review of attachment 8563475 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

Comment on attachment 8563476 [details] [diff] [review] Part 2. Implement DOM and JavaScript facing components. Review of attachment 8563476 [details] [diff] [review]: ----------------------------------------------------------------- Aside from a few nits, this looks good to me, though someone with experience implementing JS-WebIDLs should look it over. I suspect that might be bz. ::: dom/camera/CameraTestHardware.js @@ +11,5 @@ > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://gre/modules/DOMRequestHelper.jsm"); > + > +const MOZ_CAMERATESTHW_CONTRACTID = "@mozilla.org/cameratesthardware;1"; > +const MOZ_CAMERATESTHW_CID = Components.ID("{eabe71c5-5843-4dcf-8147-939d2cae0da1}"); //FIXME FIXME? @@ +154,5 @@ > + this._delegate('stopFaceDetection'); > + }, > + > + fireFacesDetected: function(faces) { > + /* This bit of trickier works around the fact that we can't have "This bit of trickier..."? @@ +198,5 @@ > + detach: function() { > + this._mock = null; > + }, > + > + _delegate: function(prop) { Please bump _delegate() up to the top of this file, so that someone reading this for the first time knows about it. ::: dom/camera/test/camera_common.js @@ +228,5 @@ > + self.camera = null; > + } > + }); > + > + //TODO: uncomment once CameraTest deprecated TODO? @@ +229,5 @@ > + } > + }); > + > + //TODO: uncomment once CameraTest deprecated > + //ise(SpecialPowers.sanityCheck(), "foo", "SpecialPowers passed sanity check"); In or out? @@ +292,5 @@ > + _run: function() { > + var test = this._tests.shift(); > + var self = this; > + if (test) { > + ok(true, test.name + ' started'); I think this can just be info(test.name + " started"); @@ +328,5 @@ > + try { > + testPromise = test.cb(); > + if (!isDefinedObj(testPromise)) { > + testPromise = Promise.resolve(); > + //throw new Error(test.name + ' callback did not return promise'); In or out? @@ +374,5 @@ > + > + var self = this; > + return cameraManager.getCamera(name, config).then( > + function(p) { > + ok(true, 'got camera'); ok(p, ...) will confirm that we actually got a camera object -- if that would be useful. @@ +454,5 @@ > + _expError: function(msg) { > + ok(false, msg); > + return Promise.reject(); > + }, > + nit: trailing whitespace.

Comment on attachment 8563478 [details] [diff] [review] Part 3. Implement Gonk wrappers. Review of attachment 8563478 [details] [diff] [review]: ----------------------------------------------------------------- This looks mostly good, but I have a few questions. ::: dom/camera/GonkCameraHwMgr.cpp @@ +242,5 @@ > sp<GonkCameraHardware> cameraHardware; > if (test.EqualsASCII("hardware")) { > NS_WARNING("Using test Gonk hardware layer"); > cameraHardware = new TestGonkCameraHardware(aTarget, aCameraId, camera); > + } else if (test.EqualsASCII("jshardware")) { This test is also in JsTestGonkCameraHardware.cpp. Does it need to be in both places? ::: dom/camera/JsTestGonkCameraHardware.cpp @@ +1,2 @@ > +/* > + * Copyright (C) 2013-2014 Mozilla Foundation -2015 @@ +28,5 @@ > +using namespace android; > +using namespace mozilla; > +using namespace mozilla::dom; > + > +static nsMainThreadPtrHandle<MozCameraTestHardware> mPendingTestHw; Static variables start with 's'. @@ +36,5 @@ > +public: > + NS_DECL_ISUPPORTS > + NS_DECL_NSIDOMEVENTLISTENER > + > + explicit JsTestGonkCameraHardwareListener(nsGonkCameraControl* aTarget, nsIThread* aCameraThread) Only single-arg ctors need 'explicit': http://www.parashift.com/c++-faq/explicit-ctors.html @@ +330,5 @@ > + > + nsRefPtr<MozCameraTestHardware> hw = MozCameraTestHardware::Constructor(global, aCx, aRv); > + if (hw) { > + // Save the pointer so that we can capture it when the native > + // wrapper gets created created. @@ +343,5 @@ > + > +nsresult > +JsTestGonkCameraHardwareImpl::Dispatch(ControlMessage* aRunnable) > +{ > + MutexAutoLock lock(mMutex); Blank line after object declaration, please. @@ +377,5 @@ > + : ControlMessage(aTestHw) > + { } > + > + protected: > + nsresult NS_METHODIMP ? @@ +387,5 @@ > + return rv.ErrorCode(); > + } > + > + EventListenerManager* manager = mTestHw->mTestHw->GetOrCreateListenerManager(); > + if (!manager) { NS_WARN_IF(...) @@ +392,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + mTestHw->mDomListener = new JsTestGonkCameraHardwareListener(mTestHw->mTarget, mTestHw->mCameraThread); > + if (!mTestHw->mDomListener) { NS_WARN_IF(...) @@ +415,5 @@ > + : ControlMessage(aTestHw) > + { } > + > + protected: > + nsresult NS_METHODIMP ? @@ +419,5 @@ > + nsresult > + RunImpl() MOZ_OVERRIDE > + { > + ErrorResult rv; > + mTestHw->mTestHw->AutoFocus(rv); Yeah, I see what you mean. Is the second mTestHw the JS object? Maybe mJSTestWrapper? or mJSTestInstance? or mJSTestHandler? or...? @@ +425,5 @@ > + } > + }; > + > + DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__); > + if (NS_FAILED(Dispatch(new Delegate(this)))) { I appreciate that NS_FAILED is a good macro, but nonetheless I'm inclined to say that you should do: nsresult rv = Dispatch(new Delegate(this)); if (NS_FAILED(rv)) { ... Throw an NS_WARN_IF in there for good measure. @@ +442,5 @@ > + : ControlMessage(aTestHw) > + { } > + > + protected: > + nsresult NS_METHODIMP, etc. @@ +452,5 @@ > + } > + }; > + > + DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__); > + if (NS_FAILED(Dispatch(new Delegate(this)))) { And ditto, etc. @@ +587,5 @@ > + DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__); > + Dispatch(new Delegate(this)); > +} > + > +class JsTestGonkCameraHardwareImpl::PushParametersDelegate : public ControlMessage Is this class used outside of PushParameters()? or can it be stuffed in there? @@ +599,5 @@ > +protected: > + nsresult > + RunImpl() MOZ_OVERRIDE > + { > + if (!mParams) { NS_WARN_IF(...) @@ +604,5 @@ > + return NS_ERROR_INVALID_ARG; > + } > + > + ErrorResult rv; > + DOM_CAMERA_LOGI("Push test parameters: %s\n", (const char *)*mParams); C++, please: static_cast<const char*>(mParams) @@ +605,5 @@ > + } > + > + ErrorResult rv; > + DOM_CAMERA_LOGI("Push test parameters: %s\n", (const char *)*mParams); > + mTestHw->mTestHw->PushParameters(NS_ConvertASCIItoUTF16((const char *)*mParams), rv); Ditto. @@ +617,5 @@ > +JsTestGonkCameraHardwareImpl::PushParameters(const GonkCameraParameters& aParams) > +{ > + DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__); > + String8 s = aParams.Flatten(); > + if (NS_FAILED(Dispatch(new PushParametersDelegate(this, &s)))) { nsresult rv = ... if (NS_FAILED(rv)) { ... @@ +623,5 @@ > + } > + return OK; > +} > + > +class JsTestGonkCameraHardwareImpl::PullParametersDelegate : public ControlMessage Is this class used outside of PullParameters()? or can it be stuffed in there? @@ +660,5 @@ > + if (NS_FAILED(rv)) { > + return rv; > + } > + > + String8 s(NS_LossyConvertUTF16toASCII(as).get()); How does this work? The Dispatch() runs asynchronously, no? @@ +681,5 @@ > +JsTestGonkCameraHardwareImpl::PullParameters(CameraParameters& aParams) > +{ > + DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__); > + nsString as; > + nsresult rv = Dispatch(new PullParametersDelegate(this, &as)); \o/ @@ +686,5 @@ > + if (NS_FAILED(rv)) { > + as.Truncate(); > + } > + > + String8 s(NS_LossyConvertUTF16toASCII(as).get()); But also, how does this work? ::: dom/camera/JsTestGonkCameraHardware.h @@ +1,2 @@ > +/* > + * Copyright (C) 2014 Mozilla Foundation -2015 @@ +34,5 @@ > + uint32_t aCameraId, > + const sp<Camera>& aCamera); > + > + static already_AddRefed<mozilla::dom::MozCameraTestHardware> > + CreateTestHardware(nsIGlobalObject* aGlobal, JSContext *aCx, mozilla::ErrorResult& aRv); What is the difference between Create() and CreateTestHardware()? Add comments to document them? @@ +36,5 @@ > + > + static already_AddRefed<mozilla::dom::MozCameraTestHardware> > + CreateTestHardware(nsIGlobalObject* aGlobal, JSContext *aCx, mozilla::ErrorResult& aRv); > + > + virtual ~JsTestGonkCameraHardware() {} Since this has a static factory that returns a ref-counted object, the dtor should probably be protected too. Also, the other camera stack files use { }. @@ +43,5 @@ > + JsTestGonkCameraHardware(mozilla::nsGonkCameraControl* aTarget, > + uint32_t aCameraId, > + const sp<Camera>& aCamera) > + : GonkCameraHardware(aTarget, aCameraId, aCamera) > + {} { } ::: dom/camera/JsTestGonkCameraHardwareImpl.h @@ +1,2 @@ > +/* > + * Copyright (C) 2014 Mozilla Foundation -2015 @@ +21,5 @@ > +#include "nsAutoPtr.h" > +#include "nsIDOMEventListener.h" > +#include "mozilla/dom/CameraTestHardwareBinding.h" > + > +namespace android { Should these be in the mozilla namespace?

Comment on attachment 8563479 [details] [diff] [review] Part 4. Update test cases to use new test hardware. Review of attachment 8563479 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the questions/nits below addressed. ::: dom/camera/test/test_bug1022766.html @@ +11,5 @@ > <video id="viewfinder" width="200" height="200" autoplay></video> > <img src="#" alt="This image is going to load" id="testimage"/> > <script class="testbody" type="text/javascript;version=1.7"> > > +var suite = new CameraTestSuite(window, document); Does the ctor need 'window' and 'document'? Assuming it always needs them, can it not grab them internally? @@ +16,3 @@ > > +suite.test('bug-1022766', function() { > + function triggerAutoFocus(p) { What is 'p'? ::: dom/camera/test/test_camera_fake_parameters.html @@ +118,5 @@ > + [ "hdr" ].forEach(function(mode) { > + ok(cap.sceneModes.indexOf(mode) == -1, "Scene mode '" + mode + "' is not present"); > + try { > + cam.sceneMode = mode; > + } catch(e) { Okay to leave the catch case empty? @@ +302,5 @@ > + return suite.getCamera() > + .then(resolve, suite.rejectGetCamera); > +}); > + > +suite.test('bug-1054803', function() { Does doing it this way fix bug 1069725 ? ::: dom/camera/test/test_camera_hardware_auto_focus_moving_cb.html @@ +43,5 @@ > > </script> > </body> > > </html> Well that's much more concise. Well done. ::: dom/camera/test/test_camera_hardware_failures.html @@ +50,5 @@ > + .catch(suite.rejectGetCamera) > + .then(startAutoFocusFail) > + .then(resolveAutoFocusFail, suite.rejectAutoFocus) > + .then(startAutoFocusError) > + .then(suite.expRejectAutoFocus, rejectAutoFocusError) 'exp' --> 'expect' or 'expected', whichever makes more grammatical sense. Also: we discussed this in IRC, and it would be helpful to whomever comes after us to have at least one very detailed explanation of how this all works. ::: dom/camera/test/test_camera_hardware_init_failure.html @@ +40,5 @@ > + ok(true, 'onSuccess called correctly for second getCamera request'); > + } > + > + return cameraManager.getCamera(whichCamera) > + .then(suite.expRejectGetCamera, rejectGetCamera) ...expect[ed]...

Update patch header.

(In reply to Mike Habicher [:mikeh] from comment #6) > Comment on attachment 8563476 [details] [diff] [review] > Part 2. Implement DOM and JavaScript facing components. > > Review of attachment 8563476 [details] [diff] [review]: > ----------------------------------------------------------------- > > Aside from a few nits, this looks good to me, though someone with experience > implementing JS-WebIDLs should look it over. I suspect that might be bz. > > ::: dom/camera/CameraTestHardware.js > @@ +11,5 @@ > > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > > +Cu.import("resource://gre/modules/DOMRequestHelper.jsm"); > > + > > +const MOZ_CAMERATESTHW_CONTRACTID = "@mozilla.org/cameratesthardware;1"; > > +const MOZ_CAMERATESTHW_CID = Components.ID("{eabe71c5-5843-4dcf-8147-939d2cae0da1}"); //FIXME I think I fixed this already but I updated the GUID in any case to make sure. > @@ +154,5 @@ > > + this._delegate('stopFaceDetection'); > > + }, > > + > > + fireFacesDetected: function(faces) { > > + /* This bit of trickier works around the fact that we can't have > > "This bit of trickier..."? Done and updated comments inside of this file all over. > @@ +198,5 @@ > > + detach: function() { > > + this._mock = null; > > + }, > > + > > + _delegate: function(prop) { > > Please bump _delegate() up to the top of this file, so that someone reading > this for the first time knows about it. > > ::: dom/camera/test/camera_common.js > @@ +228,5 @@ > > + self.camera = null; > > + } > > + }); > > + > > + //TODO: uncomment once CameraTest deprecated > > TODO? > > @@ +229,5 @@ > > + } > > + }); > > + > > + //TODO: uncomment once CameraTest deprecated > > + //ise(SpecialPowers.sanityCheck(), "foo", "SpecialPowers passed sanity check"); > > In or out? Done. > @@ +292,5 @@ > > + _run: function() { > > + var test = this._tests.shift(); > > + var self = this; > > + if (test) { > > + ok(true, test.name + ' started'); > > I think this can just be info(test.name + " started"); > > @@ +328,5 @@ > > + try { > > + testPromise = test.cb(); > > + if (!isDefinedObj(testPromise)) { > > + testPromise = Promise.resolve(); > > + //throw new Error(test.name + ' callback did not return promise'); > > In or out? Should succeed I think. > @@ +374,5 @@ > > + > > + var self = this; > > + return cameraManager.getCamera(name, config).then( > > + function(p) { > > + ok(true, 'got camera'); > > ok(p, ...) will confirm that we actually got a camera object -- if that > would be useful. Done.

(In reply to Mike Habicher [:mikeh] from comment #7) > Comment on attachment 8563478 [details] [diff] [review] > Part 3. Implement Gonk wrappers. > > Review of attachment 8563478 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks mostly good, but I have a few questions. > > ::: dom/camera/GonkCameraHwMgr.cpp > @@ +242,5 @@ > > sp<GonkCameraHardware> cameraHardware; > > if (test.EqualsASCII("hardware")) { > > NS_WARNING("Using test Gonk hardware layer"); > > cameraHardware = new TestGonkCameraHardware(aTarget, aCameraId, camera); > > + } else if (test.EqualsASCII("jshardware")) { > > This test is also in JsTestGonkCameraHardware.cpp. Does it need to be in > both places? > > ::: dom/camera/JsTestGonkCameraHardware.cpp > @@ +1,2 @@ > > +/* > > + * Copyright (C) 2013-2014 Mozilla Foundation > > -2015 > > @@ +28,5 @@ > > +using namespace android; > > +using namespace mozilla; > > +using namespace mozilla::dom; > > + > > +static nsMainThreadPtrHandle<MozCameraTestHardware> mPendingTestHw; > > Static variables start with 's'. > > @@ +36,5 @@ > > +public: > > + NS_DECL_ISUPPORTS > > + NS_DECL_NSIDOMEVENTLISTENER > > + > > + explicit JsTestGonkCameraHardwareListener(nsGonkCameraControl* aTarget, nsIThread* aCameraThread) > > Only single-arg ctors need 'explicit': > http://www.parashift.com/c++-faq/explicit-ctors.html > > @@ +330,5 @@ > > + > > + nsRefPtr<MozCameraTestHardware> hw = MozCameraTestHardware::Constructor(global, aCx, aRv); > > + if (hw) { > > + // Save the pointer so that we can capture it when the native > > + // wrapper gets created > > created. > > @@ +343,5 @@ > > + > > +nsresult > > +JsTestGonkCameraHardwareImpl::Dispatch(ControlMessage* aRunnable) > > +{ > > + MutexAutoLock lock(mMutex); > > Blank line after object declaration, please. > > @@ +377,5 @@ > > + : ControlMessage(aTestHw) > > + { } > > + > > + protected: > > + nsresult > > NS_METHODIMP ? > > @@ +387,5 @@ > > + return rv.ErrorCode(); > > + } > > + > > + EventListenerManager* manager = mTestHw->mTestHw->GetOrCreateListenerManager(); > > + if (!manager) { > > NS_WARN_IF(...) > > @@ +392,5 @@ > > + return NS_ERROR_FAILURE; > > + } > > + > > + mTestHw->mDomListener = new JsTestGonkCameraHardwareListener(mTestHw->mTarget, mTestHw->mCameraThread); > > + if (!mTestHw->mDomListener) { > > NS_WARN_IF(...) > > @@ +415,5 @@ > > + : ControlMessage(aTestHw) > > + { } > > + > > + protected: > > + nsresult > > NS_METHODIMP ? > Done. > @@ +419,5 @@ > > + nsresult > > + RunImpl() MOZ_OVERRIDE > > + { > > + ErrorResult rv; > > + mTestHw->mTestHw->AutoFocus(rv); > > Yeah, I see what you mean. > > Is the second mTestHw the JS object? Maybe mJSTestWrapper? or > mJSTestInstance? or mJSTestHandler? or...? > I chose mJSTestWrapper. > @@ +425,5 @@ > > + } > > + }; > > + > > + DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__); > > + if (NS_FAILED(Dispatch(new Delegate(this)))) { > > I appreciate that NS_FAILED is a good macro, but nonetheless I'm inclined to > say that you should do: > > nsresult rv = Dispatch(new Delegate(this)); > if (NS_FAILED(rv)) { ... > > Throw an NS_WARN_IF in there for good measure. > I went through all of the return codes and ensure we spit out a warning on all of them if they failed. > @@ +442,5 @@ > > + : ControlMessage(aTestHw) > > + { } > > + > > + protected: > > + nsresult > > NS_METHODIMP, etc. > > @@ +452,5 @@ > > + } > > + }; > > + > > + DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__); > > + if (NS_FAILED(Dispatch(new Delegate(this)))) { > > And ditto, etc. > > @@ +587,5 @@ > > + DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__); > > + Dispatch(new Delegate(this)); > > +} > > + > > +class JsTestGonkCameraHardwareImpl::PushParametersDelegate : public ControlMessage > > Is this class used outside of PushParameters()? or can it be stuffed in > there? > > @@ +599,5 @@ > > +protected: > > + nsresult > > + RunImpl() MOZ_OVERRIDE > > + { > > + if (!mParams) { > > NS_WARN_IF(...) > > @@ +604,5 @@ > > + return NS_ERROR_INVALID_ARG; > > + } > > + > > + ErrorResult rv; > > + DOM_CAMERA_LOGI("Push test parameters: %s\n", (const char *)*mParams); > > C++, please: static_cast<const char*>(mParams) > > @@ +605,5 @@ > > + } > > + > > + ErrorResult rv; > > + DOM_CAMERA_LOGI("Push test parameters: %s\n", (const char *)*mParams); > > + mTestHw->mTestHw->PushParameters(NS_ConvertASCIItoUTF16((const char *)*mParams), rv); > > Ditto. > Done. > @@ +617,5 @@ > > +JsTestGonkCameraHardwareImpl::PushParameters(const GonkCameraParameters& aParams) > > +{ > > + DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__); > > + String8 s = aParams.Flatten(); > > + if (NS_FAILED(Dispatch(new PushParametersDelegate(this, &s)))) { > > nsresult rv = ... > if (NS_FAILED(rv)) { ... > > @@ +623,5 @@ > > + } > > + return OK; > > +} > > + > > +class JsTestGonkCameraHardwareImpl::PullParametersDelegate : public ControlMessage > > Is this class used outside of PullParameters()? or can it be stuffed in > there? > > @@ +660,5 @@ > > + if (NS_FAILED(rv)) { > > + return rv; > > + } > > + > > + String8 s(NS_LossyConvertUTF16toASCII(as).get()); > > How does this work? The Dispatch() runs asynchronously, no? > > @@ +681,5 @@ > > +JsTestGonkCameraHardwareImpl::PullParameters(CameraParameters& aParams) > > +{ > > + DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__); > > + nsString as; > > + nsresult rv = Dispatch(new PullParametersDelegate(this, &as)); > > \o/ > > @@ +686,5 @@ > > + if (NS_FAILED(rv)) { > > + as.Truncate(); > > + } > > + > > + String8 s(NS_LossyConvertUTF16toASCII(as).get()); > > But also, how does this work? > ::Dispatch is a blocking call which waits on a condvar to be signaled after it posts the runnable to the main thread. Once the main thread has executed the runnable, it unblocks the camera thread and we can continue. > ::: dom/camera/JsTestGonkCameraHardware.h > @@ +1,2 @@ > > +/* > > + * Copyright (C) 2014 Mozilla Foundation > > -2015 > > @@ +34,5 @@ > > + uint32_t aCameraId, > > + const sp<Camera>& aCamera); > > + > > + static already_AddRefed<mozilla::dom::MozCameraTestHardware> > > + CreateTestHardware(nsIGlobalObject* aGlobal, JSContext *aCx, mozilla::ErrorResult& aRv); > > What is the difference between Create() and CreateTestHardware()? Add > comments to document them? > I removed ::Create and consolidated JsTestGonkCameraHardware and JsTestGonkCameraHardwareImpl into TestGonkCameraHardware. There were some compilation issues I was avoiding with the Impl if I recall correctly (conflict with some headers?) but it seems that is no longer a problem. I also renamed CreateTestHardware to CreateJSTestWrapper to mirror the internal name. > @@ +36,5 @@ > > + > > + static already_AddRefed<mozilla::dom::MozCameraTestHardware> > > + CreateTestHardware(nsIGlobalObject* aGlobal, JSContext *aCx, mozilla::ErrorResult& aRv); > > + > > + virtual ~JsTestGonkCameraHardware() {} > > Since this has a static factory that returns a ref-counted object, the dtor > should probably be protected too. > > Also, the other camera stack files use { }. > > @@ +43,5 @@ > > + JsTestGonkCameraHardware(mozilla::nsGonkCameraControl* aTarget, > > + uint32_t aCameraId, > > + const sp<Camera>& aCamera) > > + : GonkCameraHardware(aTarget, aCameraId, aCamera) > > + {} > > { } > > ::: dom/camera/JsTestGonkCameraHardwareImpl.h > @@ +1,2 @@ > > +/* > > + * Copyright (C) 2014 Mozilla Foundation > > -2015 > Done. > @@ +21,5 @@ > > +#include "nsAutoPtr.h" > > +#include "nsIDOMEventListener.h" > > +#include "mozilla/dom/CameraTestHardwareBinding.h" > > + > > +namespace android { > > Should these be in the mozilla namespace? Moved.

I addressed most of the problems and added documentation as requested. Can you thumb through test_camera_hardware_failures.html and see if there is anything that concerns you? (In reply to Mike Habicher [:mikeh] from comment #8) > Comment on attachment 8563479 [details] [diff] [review] > Part 4. Update test cases to use new test hardware. > > Review of attachment 8563479 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ with the questions/nits below addressed. > > ::: dom/camera/test/test_bug1022766.html > @@ +11,5 @@ > > <video id="viewfinder" width="200" height="200" autoplay></video> > > <img src="#" alt="This image is going to load" id="testimage"/> > > <script class="testbody" type="text/javascript;version=1.7"> > > > > +var suite = new CameraTestSuite(window, document); > > Does the ctor need 'window' and 'document'? Assuming it always needs them, > can it not grab them internally? > > @@ +16,3 @@ > > > > +suite.test('bug-1022766', function() { > > + function triggerAutoFocus(p) { > > What is 'p'? > p is the promise resolve parameter, if any. Not used in this case. > ::: dom/camera/test/test_camera_fake_parameters.html > @@ +118,5 @@ > > + [ "hdr" ].forEach(function(mode) { > > + ok(cap.sceneModes.indexOf(mode) == -1, "Scene mode '" + mode + "' is not present"); > > + try { > > + cam.sceneMode = mode; > > + } catch(e) { > > Okay to leave the catch case empty? > I think so yes. It isn't critical to the test which just needs to make sure the value wasn't set (a few lines down). > @@ +302,5 @@ > > + return suite.getCamera() > > + .then(resolve, suite.rejectGetCamera); > > +}); > > + > > +suite.test('bug-1054803', function() { > > Does doing it this way fix bug 1069725 ? > Yes. > ::: dom/camera/test/test_camera_hardware_auto_focus_moving_cb.html > @@ +43,5 @@ > > > > </script> > > </body> > > > > </html> > > Well that's much more concise. Well done. > > ::: dom/camera/test/test_camera_hardware_failures.html > @@ +50,5 @@ > > + .catch(suite.rejectGetCamera) > > + .then(startAutoFocusFail) > > + .then(resolveAutoFocusFail, suite.rejectAutoFocus) > > + .then(startAutoFocusError) > > + .then(suite.expRejectAutoFocus, rejectAutoFocusError) > > 'exp' --> 'expect' or 'expected', whichever makes more grammatical sense. > > Also: we discussed this in IRC, and it would be helpful to whomever comes > after us to have at least one very detailed explanation of how this all > works. > > ::: dom/camera/test/test_camera_hardware_init_failure.html > @@ +40,5 @@ > > + ok(true, 'onSuccess called correctly for second getCamera request'); > > + } > > + > > + return cameraManager.getCamera(whichCamera) > > + .then(suite.expRejectGetCamera, rejectGetCamera) > > ...expect[ed]...

Ooops, put a change in the wrong patch (end result if you applied them all is the same). bz: Can you do the DOM peer review and if you have the time, look through the JavaScript code to offer any suggestions? While it seems to work fine, neither Mike nor I are exactly JavaScript experts :). One thing I am concerned about is exposing createTestHardware on CameraManager, as it is kind of ugly. However I am somewhat perplexed as to a better way to share a JS handle between the gonk and application layers.

Move changes that should have been in part 2 from part 3.

CreateJSTestWrapper should be in part 2, right? I'll try to review this on Tuesday, but I have a few questions in the meantime: 1) Who gets access to the createTestHardware method? 2) Why both navigator.mozCameraTestHardware and the createTestHardware thing? when should one use one or the other? 3) MozCameraTestHardware needs a lot more documentation, both in terms of its purpose overall and in terms of what the methods do.

Comment on attachment 8564378 [details] [diff] [review] Part 4. Update test cases to use new test hardware. v2 Review of attachment 8564378 [details] [diff] [review]: ----------------------------------------------------------------- r+ with a few style nits addressed. ::: dom/camera/test/camera_common.js @@ +225,4 @@ > it so that the test case may manipulate it as it sees fit > in this.hw. Minimal setup is done for the test hardware > such that the camera is able to be brought up without > issue. nit: trailing whitespace. ::: dom/camera/test/test_camera_hardware_failures.html @@ +50,2 @@ > > + /* This is the promise chain which drives the test execution. This is great -- thank you! @@ +58,5 @@ > + automatically release it for you when the test is > + completed. > + > + .then(startAutoFocusFail, suite.rejectGetCamera) > + If the getCamera promise is resolved, startAutoFocusFail "If the getCamera promise..." --> "When the getCamera promise..." (Prefer optimism in the expected case. :) @@ +85,5 @@ > + If the suite.camera.autoFocus() promise is resolved, > + resolveAutoFocusFail will be called. That function > + simply verifies the result from the autoFocus() call. > + It should be false, given the modified behaviour we > + gave the driver. Since it doesn't return a new promise "gave" --> "injected into" ?

Comment on attachment 8564385 [details] [diff] [review] Part 3. Implement Gonk wrappers. v2.1 Review of attachment 8564385 [details] [diff] [review]: ----------------------------------------------------------------- r+ with a few nits fixed, and possibly coming up with a clearer/move-obvious-what's-going-on name for Dispatch(). ::: dom/camera/TestGonkCameraHardware.cpp @@ +31,5 @@ > +using namespace mozilla::dom; > + > +static nsMainThreadPtrHandle<MozCameraTestHardware> sPendingTestHw; > + > +static void CopyFaceFeature(int32_t* dst, bool exists, DOMPoint* src) nit: static void CopyFaceFeature(... @@ +35,5 @@ > +static void CopyFaceFeature(int32_t* dst, bool exists, DOMPoint* src) > +{ > + if (exists && src) { > + dst[0] = (int32_t)src->X(); > + dst[1] = (int32_t)src->Y(); C++-style casts, please. @@ +119,5 @@ > + return NS_OK; > + } > + > + uint8_t* data = new uint8_t[dataLength]; > + rv = NS_ReadInputStreamToBuffer(inputStream, (void**)&data, (uint32_t)dataLength); nit: C++-style casts, please. @@ +146,5 @@ > + class DeferredSystemFailure : public nsRunnable > + { > + public: > + DeferredSystemFailure(nsGonkCameraControl* aTarget) > + : mTarget(aTarget) nit: indent two spaces from ctor name, please. @@ +157,5 @@ > + return NS_OK; > + } > + > + protected: > + nsRefPtr<nsGonkCameraControl> mTarget; nit: de-indent (outdent?) the above two lines two spaces. @@ +185,5 @@ > + uint32_t i = facesData.Length(); > + > + metadata.number_of_faces = i; > + metadata.faces = new camera_face_t[i]; > + memset(metadata.faces, 0, sizeof(camera_face_t)*i); nit: sizeof(...) * i @@ +191,5 @@ > + while (i > 0) { > + --i; > + const nsRefPtr<DOMCameraDetectedFace>& face = facesData[i]; > + camera_face_t& f = metadata.faces[i]; > + const DOMRect* bounds = face->Bounds(); Can 'bounds' be a ref too? @@ +195,5 @@ > + const DOMRect* bounds = face->Bounds(); > + f.rect[0] = (int32_t)bounds->Left(); > + f.rect[1] = (int32_t)bounds->Top(); > + f.rect[2] = (int32_t)bounds->Right(); > + f.rect[3] = (int32_t)bounds->Bottom(); C++-style casts above, please. @@ +263,3 @@ > > TestGonkCameraHardware::TestGonkCameraHardware(nsGonkCameraControl* aTarget, > + uint32_t aCameraId, nit: line up with nsGonkCameraControl* @@ +307,4 @@ > DOM_CAMERA_LOGA("^===== Destroyed TestGonkCameraHardware =====^\n"); > } > > +/*static*/ already_AddRefed<MozCameraTestHardware> nit: /*<space>static<space>*/ @@ +307,5 @@ > DOM_CAMERA_LOGA("^===== Destroyed TestGonkCameraHardware =====^\n"); > } > > +/*static*/ already_AddRefed<MozCameraTestHardware> > +TestGonkCameraHardware::CreateJSTestWrapper(nsIGlobalObject* aGlobal, JSContext *aCx, ErrorResult& aRv) nit: ..., JSContext* aCx, ... @@ +314,5 @@ > + nsCString test; > + CameraPreferences::GetPref("camera.control.test.enabled", test); > + > + if (test.EqualsASCII("hardware")) { > + JS::Rooted<JSObject*> jsGlobal(aCx, aGlobal->GetGlobalJSObject()); JS::RootedObject jsGlobal(...); Ref. https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS::Rooted#Description @@ +420,2 @@ > { > + ErrorResult rv; Would this be simpler with just an nsresult rv? (Here and at line 381 above?) @@ +650,5 @@ > TestGonkCameraHardware::PushParameters(const GonkCameraParameters& aParams) > { > + DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__); > + String8 s = aParams.Flatten(); > + nsresult rv = Dispatch(new PushParametersDelegate(this, &s)); At first, this had me worried because Dispatch() is normally an async process, so I thought 's' [cw]ould be destroyed before PushParametersDelegate had a chance to run. Can we use a name that makes it more clear that Dispatch() blocks? "WaitWhileRunningOnMainThread()" or something involving the word "join"?

(In reply to Not doing reviews right now from comment #15) > CreateJSTestWrapper should be in part 2, right? > > I'll try to review this on Tuesday, but I have a few questions in the > meantime: > > 1) Who gets access to the createTestHardware method? > 2) Why both navigator.mozCameraTestHardware and the createTestHardware > thing? when should > one use one or the other? Only createTestHardware was needed and only test applications ideally would have access to it; I could not think of a way to share the object reference between JS and native Gonk without an additional method at the time. Now I have converted it from a WebIDL object into an XPCOM service merely implementing the XPIDL nsICameraTestHardware. This removed the createTestHardware API in favour of using the getService API (since we only need one instance anyways). > 3) MozCameraTestHardware needs a lot more documentation, both in terms of > its purpose > overall and in terms of what the methods do. Done although this is now in nsICameraTestHardware.idl.

Comment on attachment 8569654 [details] [diff] [review] Part 2. Implement DOM and JavaScript facing components. v3 Review of attachment 8569654 [details] [diff] [review]: ----------------------------------------------------------------- This looks much cleaner. r+ with the nits/minor questions below addressed. ::: dom/bindings/Bindings.conf @@ +191,5 @@ > }, > > 'CameraManager': { > 'nativeType': 'nsDOMCameraManager', > + 'headerFile': 'DOMCameraManager.h', nit: remove the extra trailing comma. ::: dom/camera/nsICameraTestHardware.idl @@ +12,5 @@ > +{ > + /* The following methods are intended to be used by the test cases > + written in JavaScript to define the behaviour of the hardware: */ > + > + nit: remove one blank line. @@ +36,5 @@ > + takePicture > + > + Implementation notes for handlers: > + > + - If the handler throws an error, we will be the return code "we will be..."? -- this isn't clear. @@ +83,5 @@ > + > + faces is an array of CameraDetectedFaceInit dictionaries although > + hasLeftEye, hasRightEye and hasMouth may be omitted and will be > + implied by the presence/absence of leftEye, rightEye and mouth. */ > + void fireFacesDetected(in jsval faces); No dictionary available for 'faces'? @@ +93,5 @@ > + E.g. params['preview-sizes'] = '320x240,640x480'; */ > + attribute jsval params; > + > + > + nit: remove two of the above blank lines. @@ +99,5 @@ > + in order to call back into JavaScript to get test case defined > + behaviour: */ > + > + /* Set a handler to capture asynchronous events triggered by the > + test case via the fireXXX methods. */ Please provide an example of how this is used. @@ +113,5 @@ > + /* Execute an intercepted CancelAutoFocus() driver call. */ > + void cancelAutoFocus(); > + > + /* Execute an intercepted StartFaceDetection() driver call. */ > + nit: remove this blank line. @@ +114,5 @@ > + void cancelAutoFocus(); > + > + /* Execute an intercepted StartFaceDetection() driver call. */ > + > + void startFaceDetection(); nit: add a blank line above this one. ::: dom/webidl/CameraControl.webidl @@ +443,5 @@ > readonly attribute boolean hasMouth; > readonly attribute DOMPoint? mouth; > }; > > +dictionary CameraDetectedFaceInit Can this be used in fireFacesDetected() ?

Comment on attachment 8569655 [details] [diff] [review] Part 3. Implement Gonk wrappers. v3 Review of attachment 8569655 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the nit below addressed. ::: dom/camera/TestGonkCameraHardware.cpp @@ +31,5 @@ > using namespace mozilla; > +using namespace mozilla::dom; > + > +static void > +CopyFaceFeature(int32_t* dst, bool exists, DOMPoint* src) nit: aDst, aExists, aSrc. Check out this: http://stackoverflow.com/questions/1328223/sizeof-array-passed-as-parameter I think this could be: static void CopyFaceFeature(int32_t (&aDst)[2], bool aExists, DOMPoint* aSrc) { ...

(In reply to Mike Habicher [:mikeh] from comment #22) > Comment on attachment 8569654 [details] [diff] [review] > Part 2. Implement DOM and JavaScript facing components. v3 > > Review of attachment 8569654 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks much cleaner. r+ with the nits/minor questions below addressed. > > ::: dom/bindings/Bindings.conf > @@ +191,5 @@ > > }, > > > > 'CameraManager': { > > 'nativeType': 'nsDOMCameraManager', > > + 'headerFile': 'DOMCameraManager.h', > > nit: remove the extra trailing comma. > > ::: dom/camera/nsICameraTestHardware.idl > @@ +12,5 @@ > > +{ > > + /* The following methods are intended to be used by the test cases > > + written in JavaScript to define the behaviour of the hardware: */ > > + > > + > > nit: remove one blank line. > > @@ +36,5 @@ > > + takePicture > > + > > + Implementation notes for handlers: > > + > > + - If the handler throws an error, we will be the return code > > "we will be..."? -- this isn't clear. > > @@ +93,5 @@ > > + E.g. params['preview-sizes'] = '320x240,640x480'; */ > > + attribute jsval params; > > + > > + > > + > > nit: remove two of the above blank lines. > > @@ +99,5 @@ > > + in order to call back into JavaScript to get test case defined > > + behaviour: */ > > + > > + /* Set a handler to capture asynchronous events triggered by the > > + test case via the fireXXX methods. */ > > Please provide an example of how this is used. > > @@ +113,5 @@ > > + /* Execute an intercepted CancelAutoFocus() driver call. */ > > + void cancelAutoFocus(); > > + > > + /* Execute an intercepted StartFaceDetection() driver call. */ > > + > > nit: remove this blank line. > > @@ +114,5 @@ > > + void cancelAutoFocus(); > > + > > + /* Execute an intercepted StartFaceDetection() driver call. */ > > + > > + void startFaceDetection(); > > nit: add a blank line above this one. Done. > > @@ +83,5 @@ > > + > > + faces is an array of CameraDetectedFaceInit dictionaries although > > + hasLeftEye, hasRightEye and hasMouth may be omitted and will be > > + implied by the presence/absence of leftEye, rightEye and mouth. */ > > + void fireFacesDetected(in jsval faces); > > No dictionary available for 'faces'? No because the type is defined in WebIDL. I don't think I can access them in XPIDL? > > ::: dom/webidl/CameraControl.webidl > @@ +443,5 @@ > > readonly attribute boolean hasMouth; > > readonly attribute DOMPoint? mouth; > > }; > > > > +dictionary CameraDetectedFaceInit > > Can this be used in fireFacesDetected() ? Yes, it is already used.

(In reply to Mike Habicher [:mikeh] from comment #23) > Comment on attachment 8569655 [details] [diff] [review] > Part 3. Implement Gonk wrappers. v3 > > Review of attachment 8569655 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ with the nit below addressed. > > ::: dom/camera/TestGonkCameraHardware.cpp > @@ +31,5 @@ > > using namespace mozilla; > > +using namespace mozilla::dom; > > + > > +static void > > +CopyFaceFeature(int32_t* dst, bool exists, DOMPoint* src) > > nit: aDst, aExists, aSrc. > > Check out this: > http://stackoverflow.com/questions/1328223/sizeof-array-passed-as-parameter > > I think this could be: > > static void > CopyFaceFeature(int32_t (&aDst)[2], bool aExists, DOMPoint* aSrc) > { > ... Done.

Fix use of nsCOMPtr vs nsRefPtr.

Comment on attachment 8571071 [details] [diff] [review] Part 2. Implement DOM and JavaScript facing components. v4 [carries r=mikeh] >+ pullParameters: function() { We never have to worry about ';' or '=' being present in param names/values? >+++ b/dom/camera/DOMCameraDetectedFace.cpp >+ nsCOMPtr<nsISupports> s = do_QueryInterface(aGlobal.GetAsSupports()); That QI shouldn't be needed, right? r=me on the IDL bits here

(In reply to Not doing reviews right now from comment #27) > Comment on attachment 8571071 [details] [diff] [review] > Part 2. Implement DOM and JavaScript facing components. v4 [carries r=mikeh] > > >+ pullParameters: function() { > > We never have to worry about ';' or '=' being present in param names/values? No, by definition / the Android API contract, ; and = are reserved. > > >+++ b/dom/camera/DOMCameraDetectedFace.cpp > >+ nsCOMPtr<nsISupports> s = do_QueryInterface(aGlobal.GetAsSupports()); > > That QI shouldn't be needed, right? Nice catch, I will remove this. > > r=me on the IDL bits here Thanks!

Remove unnecessary do_QueryInterface in DOMCameraDetectedFace.cpp.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65cb1080bdec

Refreshed against tree (no functional changes.)

https://hg.mozilla.org/integration/b2g-inbound/rev/f15a86a87f9e https://hg.mozilla.org/integration/b2g-inbound/rev/b7eca7da161d https://hg.mozilla.org/integration/b2g-inbound/rev/1072484a6e5f https://hg.mozilla.org/integration/b2g-inbound/rev/b44f57b1db32

https://hg.mozilla.org/mozilla-central/rev/f15a86a87f9e https://hg.mozilla.org/mozilla-central/rev/b7eca7da161d https://hg.mozilla.org/mozilla-central/rev/1072484a6e5f https://hg.mozilla.org/mozilla-central/rev/b44f57b1db32