Although bug 909542 removed most of the direct JS_*() calls from DOMCameraControl.cpp, some still remain in handling weighted regions (focusAreas, meteringAreas) and sizes (pictureSize, thumbnailSize). These are because dictionaries (and sequences of dictionaries) cannot be attributes, so CameraRegion and CameraSize need to be converted to (simple) interfaces.

Incremental patch converting CameraRegion to an interface. Unfortunately as far as I can tell this breaks the API compatibility. We used to be able to do this: cam.focusAreas = [ {top: -500, bottom: 500, left: -500, right: 500, weight: 100} ]; But using an interface and relying upon the generated code to pack/unpack the data structure requires us to do this in the client code: cam.focusAreas = [ new CameraRegion({top: -500, bottom: 500, left: -500, right: 500, weight: 100}) ]; Feedback would be appreciated :).

Andrew, I've discussed this with :bz, who is one of our DOM peers, and he recommends changing 'focusAreas' from an attribute to a getter/setter pair of methods, e.g. [Throws] sequence<CameraRegion> getFocusAreas(); [Throws] void setFocusAreas(optional sequence<CameraRegion> focusAreas); The reason for this is that JS is a reference-based language, and there's an expectation that, e.g. CameraControl.focusAreas == CameraControl.focusAreas ...is true, which is to say, that the two invocations always return the same object, not merely two sequences with the same values. However, in this case, I think it's better that the focus areas be a snapshot of the current state of the driver. This has the happy side-effect of making the code much simpler, since CameraRegion can remain a dictionary, but the framework will handle the unpacking for you.

Updated meteringAreas and focusAreas as discussed, and converted pictureSize / thumbnailSize as originally intended but that suffers from the same issue originally described (needs "new CameraSizeWrapper( ... )" around dictionaries) despite not being contained within a sequence.

Comment on attachment 8402958 [details] [diff] [review] js_cleanup.patch, v1 Adding version number--makes it easier to keep track of patches when generating interdiffs.

Comment on attachment 8403579 [details] [diff] [review] js_cleanup.patch, v2 Review of attachment 8403579 [details] [diff] [review]: ----------------------------------------------------------------- Andrew, this looks really good--thanks so much for working on this bug. I've provided some feedback below. Please see the .webidl files for my thoughts on how to approach 'pictureSize' and 'thumbnailSize'. ::: dom/bindings/Bindings.conf @@ +185,5 @@ > } > }, > > +'CameraSizeWrapper': { > + 'nativeType': 'nsDOMCameraSize', Take the 'ns' prefix out--just 'DOMCameraSize' is fine. ::: dom/camera/DOMCameraManager.cpp @@ +64,5 @@ > + : mWindow(aWindow) > + , mWidth(0) > + , mHeight(0) > +{ > + MOZ_COUNT_CTOR(nsDOMCameraSize); The MOZ_COUNT_ macros are not used in reference-counted objects. (Any such occurrences are my fault.) @@ +106,5 @@ > + > +JSObject* > +nsDOMCameraSize::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) > +{ > + return CameraSizeWrapperBinding::Wrap(aCx, aScope, this); Fyi. the 'aScope' argument has gone away in recent changes. ::: dom/camera/DOMCameraManager.h @@ +42,5 @@ > +public: > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(nsDOMCameraSize) > + > + static already_AddRefed<nsDOMCameraSize> Constructor(const mozilla::dom::GlobalObject& aOwner, const mozilla::dom::CameraSize& aOptions, mozilla::ErrorResult& aRv); We try to keep long lines to <=80 characters. Even though monitors these days are huge, the shorter line lengths help with multiple columns diffs and editors. In this case, you can move each of the arguments to its own line and line them up with the one above it. e.g. void func(int aFirstArg, int aSecondArg, int aThirdArg); (You might have to paste that into a monospaced editor to see things line up properly.) @@ +45,5 @@ > + > + static already_AddRefed<nsDOMCameraSize> Constructor(const mozilla::dom::GlobalObject& aOwner, const mozilla::dom::CameraSize& aOptions, mozilla::ErrorResult& aRv); > + > + nsDOMCameraSize(nsPIDOMWindow* aWindow); > + virtual ~nsDOMCameraSize(); Make the dtor protected, please. @@ +52,5 @@ > + long Height() const { return mHeight; } > + > + nsPIDOMWindow* GetParentObject() const { return mWindow; } > + virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE; > + nsresult Initialize(long width, long height); 'aWidth' and 'aHeight' @@ +57,5 @@ > + > +protected: > + nsresult Initialize(const mozilla::dom::CameraSize& aOptions); > + > + nsCOMPtr<nsPIDOMWindow> mWindow; nit: just one space here between the type and the var-name. ::: dom/webidl/CameraControl.webidl @@ +186,5 @@ > objects missing one or more of these properties will be ignored; > if the array contains more than capabilities.maxMeteringAreas, > extra areas will be ignored. > > this attribute can be set to null to allow the camera to determine "if the setter is called with no arguments, the camera will determine metering areas on its own" @@ +199,5 @@ > > if the array contains more than capabilities.maxFocusAreas, extra > areas will be ignored. > > this attribute can be set to null to allow the camera to determine "if the setter is called with no arguments, the camera will determine focus areas on its own" @@ +261,5 @@ > /* the size of the picture to be returned by a call to takePicture(); > an object with 'height' and 'width' properties that corresponds to > one of the options returned by capabilities.pictureSizes. */ > [Throws] > + attribute CameraSizeWrapper pictureSize; I've given this a bit more thought, and I think the proper approach for 'pictureSize' and 'thumbnailSize' is to document them as being deprecated, but leave them as-is, i.e. 'attribute any' types. Instead, let's add new get/set methods; then, when the Camera app migrates from the deprecated methods, we can remove them. ::: dom/webidl/CameraManager.webidl @@ +10,3 @@ > /* Used for the dimensions of a captured picture, > a preview stream, a video capture stream, etc. */ > dictionary CameraSize I think this should be CameraSizeDictionary, and the interface just CameraSize. @@ +16,5 @@ > }; > > +/* Wrapper around the CameraSize dictionary to > + allow it to be used directly as attributes */ > +[Constructor(optional CameraSize options)] The extended attributes for these new interfaces will also need a Func="..." clause; see bug 983180.

Updated based on review feedback. Noted that thumbnailSize and pictureSize attributes are deprecated in favour of setter/getter methods, switched test cases to using those methods.

Comment on attachment 8405633 [details] [diff] [review] js_cleanup.patch, v3 Review of attachment 8405633 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me, except for the nits I've identified below. Fix these up and I'll ask a DOM peer to review the changes (required for anything that faces the Web). ::: dom/camera/DOMCameraControl.cpp @@ +236,3 @@ > uint32_t length = 0; > + const Sequence<CameraRegion> &aRegions = aValue.Value(); > + if (aValue.WasPassed()) { Check .WasPassed() before copying into the local sequence. Also, 'aRegions' would be an argument--just call it 'regions'. Oh, and the reference type indicator does with the type, not the var. const Sequence<CameraRegion>& regions = aValue.Value(); @@ +251,2 @@ > ICameraControl::Region* r = regionArray.AppendElement(); > + const CameraRegion &aRegion = aRegions[i]; const CameraRegion& region = regions[i]; @@ +403,5 @@ > { > + aRv = Get(CAMERA_PARAM_METERINGAREAS, aAreas); > +} > +void > +nsDOMCameraControl::SetMeteringAreas(const Optional<Sequence<CameraRegion>>& aMeteringAreas, ErrorResult& aRv) Space inside >>. @@ +449,4 @@ > JS::Value > nsDOMCameraControl::GetPictureSize(JSContext* cx, ErrorResult& aRv) > { > JS::Rooted<JS::Value> value(cx); Please add a line at the beginning of this function, NS_WARNING("pictureSize attribute is deprecated and will be removed"); (Ditto for the setter, and for the thumbnailSize attributes as well.) @@ +496,4 @@ > JS::Value > nsDOMCameraControl::GetThumbnailSize(JSContext* aCx, ErrorResult& aRv) > { > JS::Rooted<JS::Value> value(aCx); NS_WARNING(...); ::: dom/camera/DOMCameraControl.h @@ +182,5 @@ > > // An agent used to join audio channel service. > nsCOMPtr<nsIAudioChannelAgent> mAudioChannelAgent; > > + nsresult Set(uint32_t aKey, const dom::Optional<dom::Sequence<dom::CameraRegion>>& aValue, uint32_t aLimit); Probably ought to put a space inside the >>.

Comment on attachment 8406249 [details] [diff] [review] js_cleanup.patch, v4 Review of attachment 8406249 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me! bz, do you have time to review these DOM changes?

I can do that; might be tomorrow, though.

review ping?

Comment on attachment 8406249 [details] [diff] [review] js_cleanup.patch, v4 >+nsDOMCameraControl::Set(uint32_t aKey, const Optional<Sequence<CameraRegion> >& aValue, uint32_t aLimit) >+ ICameraControl::Region* r = regionArray.AppendElement(); The length is under web page control, so regionArray needs to be a FallibleTArray and you need to handle OOM here. Unless the aLimit argument saves you, in which case that should be documented. >+ regionArray.SetCapacity(0); This seems pointless: regionArray already has 0 length and 0 capacity. >+nsDOMCameraControl::Get(uint32_t aKey, nsTArray<CameraRegion>& aValue) >+ CameraRegion* v = &aValue[i]; CameraRegion& v = aValue[i]; seems better to me, but either way. I'm not sure there's much of a point to the deprecation NS_WARNING, unless you think the people using these APIs are running debug builds. If you want to add actual deprecation warnings that actually go to the console, that seems like a great idea, as a separate patch... r=me

And my apologies that took so long. :(

carrying r=bz,mikeh forward Updated based on feedback from bz: 1) aLimit saves us from large memory allocations, comment added to that effect 2) Removed SetCapacity(0) as meaningless 3) Fixed style nit on reference vs pointer 4) Removed deprecation warnings

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

un-smash the emulator builds: https://tbpl.mozilla.org/?tree=Try&rev=d352875561b2

third time's the charm: https://tbpl.mozilla.org/?tree=Try&rev=227c2ea77c22

https://hg.mozilla.org/integration/b2g-inbound/rev/27b41f71ccd4 Congrats, Andrew, on landing your first Gecko patch!

Backed out this in http://hg.mozilla.org/integration/b2g-inbound/rev/33a615530ab1 and also bug 985496 for mochitest-5 orange: https://tbpl.mozilla.org/php/getParsedLog.php?id=38522732&tree=B2g-Inbound

Take 2: https://hg.mozilla.org/integration/b2g-inbound/rev/3be37517e259