Closed Bug 987954 Opened 6 years ago Closed 6 years ago

[Camera][Gecko] Remove last direct JS_*() calls from DOMCameraControl.cpp

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: mikeh, Assigned: aosmond)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [good first bug][mentor=mikeh])

Attachments

(1 file, 4 obsolete files)

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.
Component: Gaia::Camera → DOM: Device Interfaces
Product: Firefox OS → Core
Assignee: nobody → aosmond
Attached patch js_cleanup.patch, v1 (obsolete) — Splinter Review
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.
Status: NEW → ASSIGNED
Attached patch js_cleanup.patch, v2 (obsolete) — Splinter Review
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.
Attachment #8402958 - Attachment is obsolete: true
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.
Attachment #8402958 - Attachment description: js_cleanup.patch → js_cleanup.patch, v1
Attachment #8403579 - Attachment description: js_cleanup.patch → js_cleanup.patch, v2
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.
Attached patch js_cleanup.patch, v3 (obsolete) — Splinter Review
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.
Attachment #8403579 - Attachment is obsolete: true
Attachment #8405633 - Flags: review?(mhabicher)
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 >>.
Attached patch js_cleanup.patch, v4 (obsolete) — Splinter Review
Attachment #8405633 - Attachment is obsolete: true
Attachment #8405633 - Flags: review?(mhabicher)
Attachment #8406249 - Flags: review?(mhabicher)
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?
Attachment #8406249 - Flags: review?(mhabicher) → review?(bzbarsky)
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
Attachment #8406249 - Flags: review?(bzbarsky) → review+
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
Attachment #8406249 - Attachment is obsolete: true
Attachment #8412146 - Flags: review+
https://hg.mozilla.org/integration/b2g-inbound/rev/27b41f71ccd4

Congrats, Andrew, on landing your first Gecko patch!
https://hg.mozilla.org/mozilla-central/rev/3be37517e259
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
See Also: → 1004588
You need to log in before you can comment on or make changes to this bug.