Thinking: use SpecialPowers to switch the bottom end of the CameraControl stack to a fake camera into which we can inject various failure conditions.

First cut of a failable GonkCameraHardware layer than can be used to test the error-handling paths in the upper layers. This patch includes a number of fixes to the upper layers found just by failing to properly initialize the camera hardware. :) try-server push: https://tbpl.mozilla.org/?tree=Try&rev=6c6d04c3f0c4

Fix issues caught by the try-server. New push: https://tbpl.mozilla.org/?tree=Try&rev=6ad79ed30fc7

latest try-server results: https://tbpl.mozilla.org/?tree=Try&rev=26abbe97e4aa (Orange is due to another, unrelated test failing.)

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=21e8733fc216

Try not to hit [Enter] too quickly this time.

These are changes to the DOM interface layer, but they only involve the bottom end.

In short, separated camera creation from starting/stopping for testability, which affected how WebRTC talks to the camera.

Comment on attachment 8367630 [details] [diff] [review] Part 1 - Camera, v1 Review of attachment 8367630 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/camera/CameraCommon.h @@ +24,5 @@ > #ifdef PR_LOGGING > extern PRLogModuleInfo* GetCameraLog(); > #define DOM_CAMERA_LOG( type, ... ) PR_LOG(GetCameraLog(), (PRLogModuleLevel)type, ( __VA_ARGS__ )) > #else > +#define DOM_CAMERA_LOG( type, ... ) printf_stderr(__VA_ARGS__) /* MFH */ debug? ::: dom/camera/CameraControlImpl.cpp @@ +234,5 @@ > // This callback can run on threads other than the Main Thread and > // the Camera Thread. > RwLockAutoEnterRead lock(mListenerLock); > > +// #ifdef PR_LOGGING rather than commenting these out, introduce another LOGGING macro and use that instead (or use #if defined(PR_LOGGING) || defined(STDERR_LOGGING) @@ +259,5 @@ > + }; > + if (static_cast<unsigned int>(aError) < sizeof(error) / sizeof(error[0]) && > + static_cast<unsigned int>(aContext) < sizeof(context) / sizeof(context[0])) { > + DOM_CAMERA_LOGW("CameraControlImpl::OnError : aContext='%s', aError='%s'\n", > + context[aContext], error[aError]); I usually like to log the integer values as well, just in case there is ever a disagreement between the integer value and the associated string. ::: dom/camera/CameraControlListener.h @@ +20,5 @@ > + CameraControlListener() > + { > + MOZ_COUNT_CTOR(CameraControlListener); > + } > + nit: trailing space ::: dom/camera/GonkCameraControl.cpp @@ +101,5 @@ > + MOZ_ASSERT(NS_GetCurrentThread() == mCameraThread); > + > + nsresult rv = Initialize(); > + if (NS_FAILED(rv)) { > + return rv; Log failure? How likely is this to happen? @@ +107,5 @@ > + > + if (aInitialConfig) { > + rv = SetConfigurationInternal(*aInitialConfig); > + if (NS_FAILED(rv)) { > + // The initial configuration failed, close up the hardware Log failure? ::: dom/camera/ICameraControl.h @@ +117,5 @@ > Size mPreviewSize; > nsString mRecorderProfile; > }; > + static already_AddRefed<ICameraControl> Create(uint32_t aCameraId); > + nit: trailing space ::: dom/camera/TestGonkCameraHardware.cpp @@ +52,5 @@ > +const nsAdoptingCString& > +TestGonkCameraHardware::TestCase() > +{ > + const nsAdoptingCString& test = Preferences::GetCString("camera.control.test.hardware"); > + return test; This looks bad to me. The lifetime of the temporary returned by Preferences::GetCString will extend to the lifetime of test. test goes out of scope when this function returns, so the returned reference is invalid.

Comment on attachment 8367631 [details] [diff] [review] Part 2 - DOM, v1 Review of attachment 8367631 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/camera/DOMCameraControlListener.cpp @@ +17,5 @@ > + CameraPreviewMediaStream* aStream) > + : mDOMCameraControl(new nsMainThreadPtrHolder<nsDOMCameraControl>(aDOMCameraControl)) > + , mStream(aStream) > +{ > + DOM_CAMERA_LOGT("%s:%d : this=%p, camera=%p, stream=%p\n", You could hide the __func__ and __LINE__ stuff in the DOM_CAMERA_LOGT macro so you don't need to add them everytime you call them.

Incorporate review feedback.

Attach the right patch this time.

Attach the right patch this time.

try-server results: - emulator+tests: https://tbpl.mozilla.org/?tree=Try&rev=19e7a663f8a0 - win32+macosx: https://tbpl.mozilla.org/?tree=Try&rev=0eb436123ebc

Comment on attachment 8367632 [details] [diff] [review] Part 3 - Test, v1 One nit would be to document the usage of camera.control.test.enabled & camera.control.test.hardware. I see where it is happening in the source, but there seems to be a mix of previous functionality.

(In reply to dclarke@mozilla.com [:onecyrenus] from comment #19) > > One nit would be to document the usage of camera.control.test.enabled & > camera.control.test.hardware. > > I see where it is happening in the source, but there seems to be a mix of > previous functionality. Thanks for the feedback. Is there a good out-of-band place to document the test prefs, or do you just mean somewhere in the test and/or code?

It would be great if there was an out of band place for test prefs, but i just meant in the test to explain the purpose of the tests.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=31f14dfdf859

Carrying r+s forward; added notes on camera test prefs to camera_common.js, as requested.

Correcting hiccups found in previous try run. try-server push: https://tbpl.mozilla.org/?tree=Try&rev=b0ab42bfd493&showall=1

Applies on top of attachment 8378529 [details] [diff] [review]. Carrying r+ forward. try-server push: https://tbpl.mozilla.org/?tree=Try&rev=7b84eb4f5490&showall=1