With the advent of camera mochitests based on a JS camera implementation in bug 1062387, many of our tests are able to be run on B2G desktop in theory if the Gonk dependencies were isolated.

Currently suffers from a memory leak and breaks the normal build, but otherwise runs and passes the JS camera tests.

Fix build breaks for gonk builds.

Bug 1139721 fixes the memory leaks reported; as it turns out, not related to B2G desktop tests, just the first time it was noticed.

Comment on attachment 8574172 [details] [diff] [review] Permit running of camera tests on B2G desktop v3 Review of attachment 8574172 [details] [diff] [review]: ----------------------------------------------------------------- r+ with a bunch of nits fixed and an few issues addressed. ::: dom/camera/FallbackCameraPlatform.cpp @@ +1,3 @@ > +/* > + * Copyright (C) 2015 Mozilla Foundation > + * Is most of this copied from the AOSP file? If so, the license header should probably reflect that. I'm not expert, but Gerv Markham can confirm whether or not anything else needs to be here. ::: dom/camera/FallbackCameraPlatform.h @@ +73,5 @@ > + sp() > + : mPtr(nullptr) > + { } > + > + sp(T *aPtr) nit: T* @@ +77,5 @@ > + sp(T *aPtr) > + : mPtr(aPtr) > + { } > + > + virtual ~sp() nit: consider |virtual ~sp() { }| @@ +80,5 @@ > + > + virtual ~sp() > + { } > + > + T* get() const { nit: { on its own line, or make this a single-line function. Ditto for all of the trivial functions below. @@ +101,5 @@ > + > + enum error_t { > + OK = 0, > + UNKNOWN_ERROR, > + INVALID_OPERATION, meh-nit: trailing comma on last item. @@ +106,5 @@ > + }; > + > + enum camera_msg_t { > + CAMERA_MSG_SHUTTER, > + CAMERA_MSG_COMPRESSED_IMAGE, Ditto. @@ +114,5 @@ > + { > + public: > + String8() { } > + > + String8(const char *aData) nit: const char* aData @@ +121,5 @@ > + } > + > + virtual ~String8() { } > + > + const char* string() const { nit: { on its own line. @@ +138,5 @@ > + struct CameraInfo { > + camera_facing_t facing; > + }; > + > + class Camera : public nsISupports Should this get MOZ_FINAL? @@ +155,5 @@ > + int stopPreview() { return UNKNOWN_ERROR; } > + int startRecording() { return UNKNOWN_ERROR; } > + int stopRecording() { return UNKNOWN_ERROR; } > + int startFaceDetection() { return UNKNOWN_ERROR; } > + int stopFaceDetection() { return UNKNOWN_ERROR; } nit: consider lining up all of the function bodies. @@ +159,5 @@ > + int stopFaceDetection() { return UNKNOWN_ERROR; } > + > + static int32_t getNumberOfCameras() { return 2; } > + > + static int getCameraInfo(int32_t aDevice, CameraInfo* aInfo) { nit: { on its own line. @@ +176,5 @@ > + > + protected: > + Camera() { } > + virtual ~Camera() { } > + }; Nuke the copy and assignment constructors too, please. @@ +250,5 @@ > + static const char KEY_VIDEO_STABILIZATION_SUPPORTED[]; > + static const char KEY_LIGHTFX[]; > + }; > + > + class MediaProfiles { nit: { on its own line. Also, if this class is not subclassed, make it MOZ_FINAL. @@ +252,5 @@ > + }; > + > + class MediaProfiles { > + public: > + static MediaProfiles* getInstance() { Ditto, and for all the functions below. ::: dom/camera/GonkCameraHwMgr.h @@ +50,3 @@ > { > +#ifndef MOZ_WIDGET_GONK > + NS_DECL_ISUPPORTS; nit: no semi-colon at the end of this macro ::: dom/camera/GonkCameraParameters.cpp @@ +77,5 @@ > + nsCString* data = static_cast<nsCString*>(aUserArg); > + data->Append(aKey); > + data->Append('='); > + data->Append(*aValue); > + data->Append(';'); Is it okay if the set has a trailing semi-colon? (I assume it is, but... AOSP.) @@ +106,5 @@ > + > + nsCString key(data, pos - data); > + data = pos + 1; > + > + nsCString* value; Use nsAutoCString for 'key' -- it includes a 64-byte stack-based buffer, which should be enough for all of the keys, and should be a bit faster. ::: dom/camera/GonkCameraParameters.h @@ +21,3 @@ > #include "nsTArray.h" > #include "nsString.h" > +#include "mozilla/Mutex.h" Switching to Mutexes will also address bug 1008483. ::: dom/camera/GonkRecorderProfiles.cpp @@ +375,5 @@ > } > > aProfiles.Clear(); > profiles->EnumerateRead(Enumerate, static_cast<void*>(&aProfiles)); > nit: trailing whitespace. ::: dom/camera/TestGonkCameraHardware.cpp @@ +30,5 @@ > using namespace android; > using namespace mozilla; > using namespace mozilla::dom; > > +#ifndef MOZ_WIDGET_GONK ISUPPORTS seems orthogonal to the widget set. Is this intentional? ::: dom/camera/TestGonkCameraHardware.h @@ +27,5 @@ > class TestGonkCameraHardware : public android::GonkCameraHardware > { > +#ifndef MOZ_WIDGET_GONK > + NS_DECL_ISUPPORTS_INHERITED; > +#endif nit: no semicolon at the end of this macro. ISUPPORTS seems orthogonal to the widget set. Is this intentional? ::: dom/camera/test/test_bug1099390.html @@ +25,5 @@ > + ok(!gotCloseEvent, "gotCloseEvent was " + gotCloseEvent); > + ok(e.reason === "HardwareReleased", "'close' event reason is: " + e.reason); > + gotCloseEvent = true; > + if (gotReleasePromise) { > + resolve(); nit: trailing whitespace. ::: dom/camera/test/test_camera_bad_initial_config.html @@ +24,2 @@ > > + return navigator.mozCameras.getCamera(whichCamera, config); nit: trailing whitespace.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bc2bfe4c265 (In reply to Mike Habicher [:mikeh] from comment #6) > Comment on attachment 8574172 [details] [diff] [review] > Permit running of camera tests on B2G desktop v3 > > Review of attachment 8574172 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ with a bunch of nits fixed and an few issues addressed. > > ::: dom/camera/FallbackCameraPlatform.cpp > @@ +1,3 @@ > > +/* > > + * Copyright (C) 2015 Mozilla Foundation > > + * > > Is most of this copied from the AOSP file? If so, the license header should > probably reflect that. > > I'm not expert, but Gerv Markham can confirm whether or not anything else > needs to be here. > > ::: dom/camera/FallbackCameraPlatform.h > @@ +73,5 @@ > > + sp() > > + : mPtr(nullptr) > > + { } > > + > > + sp(T *aPtr) > > nit: T* > > @@ +77,5 @@ > > + sp(T *aPtr) > > + : mPtr(aPtr) > > + { } > > + > > + virtual ~sp() > > nit: consider |virtual ~sp() { }| > > @@ +80,5 @@ > > + > > + virtual ~sp() > > + { } > > + > > + T* get() const { > > nit: { on its own line, or make this a single-line function. > > Ditto for all of the trivial functions below. > > @@ +101,5 @@ > > + > > + enum error_t { > > + OK = 0, > > + UNKNOWN_ERROR, > > + INVALID_OPERATION, > > meh-nit: trailing comma on last item. > > @@ +106,5 @@ > > + }; > > + > > + enum camera_msg_t { > > + CAMERA_MSG_SHUTTER, > > + CAMERA_MSG_COMPRESSED_IMAGE, > > Ditto. > > @@ +114,5 @@ > > + { > > + public: > > + String8() { } > > + > > + String8(const char *aData) > > nit: const char* aData > > @@ +121,5 @@ > > + } > > + > > + virtual ~String8() { } > > + > > + const char* string() const { > > nit: { on its own line. > > @@ +138,5 @@ > > + struct CameraInfo { > > + camera_facing_t facing; > > + }; > > + > > + class Camera : public nsISupports > > Should this get MOZ_FINAL? > > @@ +155,5 @@ > > + int stopPreview() { return UNKNOWN_ERROR; } > > + int startRecording() { return UNKNOWN_ERROR; } > > + int stopRecording() { return UNKNOWN_ERROR; } > > + int startFaceDetection() { return UNKNOWN_ERROR; } > > + int stopFaceDetection() { return UNKNOWN_ERROR; } > > nit: consider lining up all of the function bodies. > > @@ +159,5 @@ > > + int stopFaceDetection() { return UNKNOWN_ERROR; } > > + > > + static int32_t getNumberOfCameras() { return 2; } > > + > > + static int getCameraInfo(int32_t aDevice, CameraInfo* aInfo) { > > nit: { on its own line. > > @@ +176,5 @@ > > + > > + protected: > > + Camera() { } > > + virtual ~Camera() { } > > + }; > > Nuke the copy and assignment constructors too, please. > > @@ +250,5 @@ > > + static const char KEY_VIDEO_STABILIZATION_SUPPORTED[]; > > + static const char KEY_LIGHTFX[]; > > + }; > > + > > + class MediaProfiles { > > nit: { on its own line. > > Also, if this class is not subclassed, make it MOZ_FINAL. > > @@ +252,5 @@ > > + }; > > + > > + class MediaProfiles { > > + public: > > + static MediaProfiles* getInstance() { > > Ditto, and for all the functions below. > > ::: dom/camera/GonkCameraHwMgr.h > @@ +50,3 @@ > > { > > +#ifndef MOZ_WIDGET_GONK > > + NS_DECL_ISUPPORTS; > > nit: no semi-colon at the end of this macro > > @@ +106,5 @@ > > + > > + nsCString key(data, pos - data); > > + data = pos + 1; > > + > > + nsCString* value; > > Use nsAutoCString for 'key' -- it includes a 64-byte stack-based buffer, > which should be enough for all of the keys, and should be a bit faster. > > ::: dom/camera/GonkRecorderProfiles.cpp > @@ +375,5 @@ > > } > > > > aProfiles.Clear(); > > profiles->EnumerateRead(Enumerate, static_cast<void*>(&aProfiles)); > > > > nit: trailing whitespace. > > ::: dom/camera/test/test_bug1099390.html > @@ +25,5 @@ > > + ok(!gotCloseEvent, "gotCloseEvent was " + gotCloseEvent); > > + ok(e.reason === "HardwareReleased", "'close' event reason is: " + e.reason); > > + gotCloseEvent = true; > > + if (gotReleasePromise) { > > + resolve(); > > nit: trailing whitespace. > > ::: dom/camera/test/test_camera_bad_initial_config.html > @@ +24,2 @@ > > > > + return navigator.mozCameras.getCamera(whichCamera, config); > > nit: trailing whitespace. Done. > ::: dom/camera/GonkCameraParameters.cpp > @@ +77,5 @@ > > + nsCString* data = static_cast<nsCString*>(aUserArg); > > + data->Append(aKey); > > + data->Append('='); > > + data->Append(*aValue); > > + data->Append(';'); > > Is it okay if the set has a trailing semi-colon? (I assume it is, but... > AOSP.) > Yes, the parser handles this just fine. But I fixed it anyways. > ::: dom/camera/GonkCameraParameters.h > @@ +21,3 @@ > > #include "nsTArray.h" > > #include "nsString.h" > > +#include "mozilla/Mutex.h" > > Switching to Mutexes will also address bug 1008483. > Only partially. There remains another RW lock in CameraControlImpl. > ::: dom/camera/TestGonkCameraHardware.cpp > @@ +30,5 @@ > > using namespace android; > > using namespace mozilla; > > using namespace mozilla::dom; > > > > +#ifndef MOZ_WIDGET_GONK > > ISUPPORTS seems orthogonal to the widget set. Is this intentional? > > ::: dom/camera/TestGonkCameraHardware.h > @@ +27,5 @@ > > class TestGonkCameraHardware : public android::GonkCameraHardware > > { > > +#ifndef MOZ_WIDGET_GONK > > + NS_DECL_ISUPPORTS_INHERITED; > > +#endif > > nit: no semicolon at the end of this macro. > > ISUPPORTS seems orthogonal to the widget set. Is this intentional? > Yes. On Gonk platforms, we put GonkCameraHardware and TestGonkCameraHardware in AOSP smart pointers, and on non-Gonk platforms (i.e. B2G desktop), we put them in Mozilla smart pointers. It is better to compile out support for the latter on the former platform to avoid any temptation.

Removing bug 1139721 as a dependency as the memory leak is already present in other builds and unrelated to B2G desktop testing changes.