994912 - (camera-events) [Camera][Gecko] Make CameraControl use Events instead of callback attributes

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Camera, defect)

Description

[Mike Habicher [:mikeh] (high bugzilla latency)]

Ref. https://developer.mozilla.org/en-US/docs/Web/Guide/Events

This would allow JS to register for CameraControl events, e.g.

camera.addEventListener('pictureTaken', function () { ... });

See https://developer.mozilla.org/en-US/docs/Web/API/EventTarget

Comment 1

[Wilson Page [:wilsonpage]]

'wilsonpage likes this'

Comment 2

[Boris Zbarsky [:bzbarsky]]

Will this replace the callback arguments to takePicture, or coexist with those somehow?

Comment 3

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Boris Zbarsky [:bz] from comment #2)
>
> Will this replace the callback arguments to takePicture, or coexist with
> those somehow?

I think events and callbacks will coexist at first, long enough for the Camera app to switch over to using events, and then we will remove the callbacks.

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Attachment: WIP - proposed DOM changes for events, v1

Comment 5

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8416545 [details] [diff] [review]
WIP - proposed DOM changes for events, v1

Review of attachment 8416545 [details] [diff] [review]:
-----------------------------------------------------------------

Please see my feedback below.

Also, Camera app guys: can you take a look at the WebIDLs and see if they make sense to you? One thing I'm thinking about in CameraControl.webidl is to just have a single onfocus event, that would indicate (something like) { "focused", "unfocused", or "focusing" }, instead of having separate onfocus and onautofocusmoving events. Thoughts?

::: dom/webidl/CameraControl.webidl
@@ +257,5 @@
>    /* the function to call when the viewfinder stops or starts,
>       useful for synchronizing other UI elements. */
>    attribute CameraPreviewStateChange? onPreviewStateChange;
>  
> +  /* CameraStateChangeEvent */

These will need some more documentation.

@@ +297,5 @@
>  
>    /* tell the camera to attempt to focus the image */
>    [Throws]
> +  void autoFocus(optional CameraAutoFocusCallback onSuccess,
> +                 optional CameraErrorCallback onError);

Instead of making onSuccess optional for this method, would create a second 'void autoFocus()' that takes no arguments. IIRC, WebIDL supports overloading methods.

@@ +315,5 @@
>    [Pref="camera.control.autofocus_moving_callback.enabled"]
>    attribute CameraAutoFocusMovingCallback? onAutoFocusMoving;
>  
> +  /* CameraAutoFocusMovingEvent */
> +  attribute EventHandler onautofocusmoving;

I keep thinking about the autoFocus events, and I wonder if a single onautofocus would do the job. It could get passed "focused" or "not-focused" in the manual case, and in the continuous-autofocus case, it could get "moving" or "focusing" or something.

@@ +330,1 @@
>                     optional CameraErrorCallback onError);

As with autoFocus(), I would just keep takePicture() as-is and add a new takePicture() that takes no arguments. That will require making sure any members of CameraPictureOptions are properly exposed as attributes; some like get/setPictureSize() already are.

@@ +345,1 @@
>                        optional CameraErrorCallback onError);

Ditto. Though startRecording() needs at least one option: the name of the file to record into.

::: dom/webidl/CameraManager.webidl
@@ +40,5 @@
>                   CameraConfiguration initialConfiguration,
>                   GetCameraCallback callback,
>                   optional CameraErrorCallback errorCallback);
> +  void getCamera(DOMString camera,
> +                 CameraConfiguration initialConfiguration);

As we discussed in IRC, I think I would just leave this one with callbacks.

::: dom/webidl/CameraStateChangeEvent.webidl
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +[Constructor(DOMString type, optional CameraStateChangeEventInit eventInitDict), HeaderFile="GeneratedEventClasses.h"]

nit: we try to wrap long lines like this to ~80 characters. Some other .webidl files should have examples on the formatting style.

Also, where is the HeaderFile="..." entry coming from?

::: dom/webidl/moz.build
@@ +51,3 @@
>      'CameraManager.webidl',
>      'CanvasRenderingContext2D.webidl',
> +    'CameraStateChangeEvent.webidl',

I'm pretty sure these have to be in alphabetical order, or the build system will balk. Can we also collapse them into one file, like CameraEvent.webidl, or just include them in CameraControl.webidl?

Comment 6

[Diego Marcos [:dmarcos]]

It's fine to me. The only thing I ask for is consistency. All the APIs either should work via DOM-like addEventListeners or callbacks passed as arguments.

Comment 7

[Wilson Page [:wilsonpage]]

Comment on attachment 8416545 [details] [diff] [review]
WIP - proposed DOM changes for events, v1

mikeh: I'm not really sure how to read this code. I could give feedback on JS examples though. Some thoughts on the topic,

- Not everything makes sense to be an event. Things like fetching a camera object don't make sense to be events, callbacks work better here I think.
- Off the top of my head, things like 'recordingstart', 'recordingend', 'shutter', 'focus' seem to make sense to be event based.

Comment 8

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Wilson Page [:wilsonpage] from comment #7)
> 
> - Not everything makes sense to be an event. Things like fetching a camera
> object don't make sense to be events, callbacks work better here I think.
> - Off the top of my head, things like 'recordingstart', 'recordingend',
> 'shutter', 'focus' seem to make sense to be event based.

That's pretty much the division we're thinking of as well.

Comment 9

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8416545 [details] [diff] [review]
WIP - proposed DOM changes for events, v1

Review of attachment 8416545 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. My only concern was as Wilson had already stated; certain things such as retrieving a list of cameras might make more sense and might have cleaner syntax if they're handled via callbacks. But most other things, I think, would be better served as standard DOM events that can be bound using `addEventListener`.

One part of the code I was unsure about was with some of the event names such as `onpreviewstate` and `onrecordstate` -- I see in the code they're also referred to as `onPreviewStateChange` and `onRecorderStateChange` respectively. I'm not sure if this was something that was overlooked or if I'm just not understanding the code well enough. To me, I think its much clearer with the word 'change' appended to the end of the event name.

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

Attachment: WIP - proposed DOM changes for events, v2

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8435055 [details] [diff] [review]
WIP - proposed DOM changes for events, v2

Review of attachment 8435055 [details] [diff] [review]:
-----------------------------------------------------------------

Mike, can you take a look at what I've highlighted (and anything else that catches your eye)?

::: content/base/src/nsHostObjectProtocolHandler.cpp
@@ +575,3 @@
>      return NS_ERROR_DOM_BAD_URI;
>    }
> +  return rv;

Do we still need to explicitly do an ADDREF here?

::: content/media/DOMMediaStream.cpp
@@ +26,3 @@
>  
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(DOMMediaStream)
> +NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper)

If inheriting a list from DOMEventTargetHelper, do we need to explicitly list the interfaces it implements, such as nsISupports? I have seen it done with and without.

::: content/media/webspeech/recognition/SpeechRecognition.cpp
@@ +946,5 @@
> +  DOMLocalMediaStream *localStream = nullptr;
> +  nsresult rv = CallQueryInterface(aStream, &localStream);
> +  if (NS_SUCCEEDED(rv)) {
> +    mRecognition->StartRecording(localStream);
> +  }

I had to add an IID for DOMLocalMediaStream for this, but it is strange because it just casts it back to a DOMMediaStream inside the StartRecording function.

::: dom/camera/DOMCameraControl.cpp
@@ +171,2 @@
>    SetIsDOMBinding();
> +  BindToOwner(aWindow);

Perhaps I should not call BindToOwner in the above DOMMediaStream functions because it could potentially conflict with what we set here?

@@ +1190,5 @@
>          nsRefPtr<CameraStartRecordingCallback> cb = mStartRecordingOnSuccessCb.forget();
>          mStartRecordingOnErrorCb = nullptr;
>          cb->Call(ignored);
>        }
> +      DispatchTrustedEvent(NS_LITERAL_STRING("record"));

I feel like this should be folded into recorderstatechange but perhaps it is somehow easier for apps to manage if it is separate?

::: dom/webidl/CameraConfigurationEvent.webidl
@@ +14,5 @@
> +
> +dictionary CameraConfigurationEventInit : EventInit
> +{
> +  CameraMode mode = "picture";
> +  any previewSize = null;

Any suggestions on how to work around not being able to put dictionaries in the event? Create a wrapper interface perhaps?

::: dom/webidl/CameraFacesDetectedEvent.webidl
@@ +17,5 @@
> +dictionary CameraFacesDetectedEventInit : EventInit
> +{
> +  //sequence<CameraDetectedFace> faces;
> +  //CameraDetectedFace face;
> +  any faces;

Similarly, any suggestions on how to work around this one as well?

Comment 13

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Camera events, v3

Comment 14

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Media events, v1

Comment 15

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Camera events, v4

Forgot to track some files in hg, adding to patch.

Comment 16

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Camera events, v4.1

File update was incomplete, really do it this time :).

Comment 17

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Camera events, v4.2

Fix a nit and minor bug that I missed last night.

Comment 18

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Camera events, v5

Make changes discussed on IRC (elminate onrecord event, rely on onrecorderstatechange event instead), some other nits.

Comment 19

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8445297 [details] [diff] [review]
Camera events, v5

Review of attachment 8445297 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay in getting this done. I've got a few requested changes and a bunch of nits, so r-.

Once those are addressed, you'll need to get a DOM-peer to review the changes.

::: content/base/src/nsGkAtomList.h
@@ +691,5 @@
>  GK_ATOM(oncomplete, "oncomplete")
>  GK_ATOM(oncompositionend, "oncompositionend")
>  GK_ATOM(oncompositionstart, "oncompositionstart")
>  GK_ATOM(oncompositionupdate, "oncompositionupdate")
> +GK_ATOM(onconfig, "onconfig")

"onconfigurationchange"

@@ +692,5 @@
>  GK_ATOM(oncompositionend, "oncompositionend")
>  GK_ATOM(oncompositionstart, "oncompositionstart")
>  GK_ATOM(oncompositionupdate, "oncompositionupdate")
> +GK_ATOM(onconfig, "onconfig")
> +GK_ATOM(onconfigerror, "onconfigerror")

"onconfigurationerror"

@@ +813,5 @@
>  GK_ATOM(onreaderror, "onreaderror")
>  GK_ATOM(onreadsuccess, "onreadsuccess")
>  GK_ATOM(onreadystatechange, "onreadystatechange")
>  GK_ATOM(onreceived, "onreceived")
> +GK_ATOM(onrecorderror, "onrecorderror")

Make this "onrecordererror".

::: dom/camera/CameraDetectedFaceList.h
@@ +25,5 @@
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(CameraDetectedFaceList)
> +
> +public:
> +  CameraDetectedFaceList(nsISupports* aOwner, nsTArray<nsRefPtr<DOMCameraDetectedFace>>& aFaces)

nit: > >&

@@ +39,5 @@
> +    return mOwner;
> +  }
> +
> +  virtual JSObject*
> +  WrapObject(JSContext* aCx) MOZ_OVERRIDE;

nit: make this declaration one line, please.

@@ +54,5 @@
> +    return mFaces.SafeElementAt(aIndex);
> +  }
> +
> +  DOMCameraDetectedFace*
> +  IndexedGetter(uint32_t aIndex, bool &aFound) const

nit: "bool& aFound".

@@ +62,5 @@
> +  }
> +
> +public:
> +  nsCOMPtr<nsISupports> mOwner;
> +  nsTArray<nsRefPtr<DOMCameraDetectedFace>> mFaces;

nit: > >

::: dom/camera/CameraFacesDetectedEvent.cpp
@@ +20,5 @@
> +NS_IMPL_ADDREF_INHERITED(CameraFacesDetectedEvent, Event)
> +NS_IMPL_RELEASE_INHERITED(CameraFacesDetectedEvent, Event)
> +
> +NS_IMPL_CYCLE_COLLECTION_INHERITED(CameraFacesDetectedEvent, Event,
> +                                   mFaces)

nit: make this one line?

@@ +37,5 @@
> +
> +/* static */ already_AddRefed<CameraFacesDetectedEvent>
> +CameraFacesDetectedEvent::Constructor(const GlobalObject& aGlobal,
> +                                      const nsAString& aType,
> +                                      //const CameraFacesDetectedEventInit& aParam,

nit: remove this line.

@@ +52,5 @@
> +  bool trusted = event->Init(t);
> +  event->SetTrusted(trusted);
> +  return event.forget();
> +}
> +/* static */ already_AddRefed<CameraFacesDetectedEvent>

nit: add a blank line before this one.

@@ +55,5 @@
> +}
> +/* static */ already_AddRefed<CameraFacesDetectedEvent>
> +CameraFacesDetectedEvent::Constructor(EventTarget* aOwner,
> +                                      const nsAString& aType,
> +                                      //const CameraFacesDetectedEventInit& aParam,

nit: remove this line.

@@ +59,5 @@
> +                                      //const CameraFacesDetectedEventInit& aParam,
> +                                      ErrorResult& aRv)
> +{
> +  //nsCOMPtr<EventTarget> t = do_QueryInterface(aGlobal.GetAsSupports());
> +  //nsRefPtr<CameraFacesDetectedEvent> event = new CameraFacesDetectedEvent(t, nullptr, nullptr);

nit: remove these lines.

@@ +67,5 @@
> +  if (aRv.Failed()) {
> +    return nullptr;
> +  }
> +
> +  //bool trusted = event->Init(t);

nit: remove this line.

@@ +80,5 @@
> +    }
> +
> +    event->mFaces = new CameraDetectedFaceList(static_cast<EventBase*>(event), faces);
> +  }
> +*/

nit: remove this block?

::: dom/camera/CameraFacesDetectedEvent.h
@@ +12,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +//class CameraFacesDetectedEventInit;

nit: remove this line?

@@ +32,5 @@
> +  NS_FORWARD_TO_EVENT
> +
> +  virtual JSObject* WrapObject(JSContext* aCx) MOZ_OVERRIDE;
> +
> +  CameraDetectedFaceList* GetFaces() const

Just make this one line:
CameraDetectedFaceList* GetFaces() const { return mFaces; }

@@ +42,5 @@
> +
> +  static already_AddRefed<CameraFacesDetectedEvent>
> +  Constructor(const GlobalObject& aGlobal,
> +              const nsAString& aType,
> +//              CameraFacesDetectedEventInit& aParam,

nit: remove this line.

@@ +48,5 @@
> +
> +  static already_AddRefed<CameraFacesDetectedEvent>
> +  Constructor(EventTarget* aOwner,
> +              const nsAString& aType,
> +//              CameraFacesDetectedEventInit& aParam,

nit: remove this one too.

::: dom/camera/DOMCameraControl.cpp
@@ +824,5 @@
> +    if (ecb) {
> +      NS_DispatchToMainThread(new ImmediateErrorCallback(ecb,
> +                              NS_LITERAL_STRING("AutoFocusInterrupted")));
> +    }
> +    DispatchErrorEvent(NS_LITERAL_STRING("focuserror"), NS_LITERAL_STRING("AutoFocusAlreadyInProgress"));

I don't think it makes any sense to fire a 'focuserror' event when autoFocus() is interrupted. Just invoke the existing callback, if there is one.

@@ +829,3 @@
>    }
>  
> +  mAutoFocusPending = true;

Only set this if the call to mCameraControl->AutoFocus() below succeeds?

@@ +911,5 @@
>      mCameraControl->Set(CAMERA_PARAM_PICTURE_DATETIME, aOptions.mDateTime);
>      mCameraControl->SetLocation(p);
>    }
>  
> +  mTakePicturePending = true;

Dependent on the call below succeeding.

@@ +1012,5 @@
> +void
> +nsDOMCameraControl::DispatchStateEvent(const nsString& aType, const nsString& aState)
> +{
> +  CameraStateChangeEventInit eventInit;
> +  eventInit.mNewState = aState; 

nit: trailing whitespace.

@@ +1015,5 @@
> +  CameraStateChangeEventInit eventInit;
> +  eventInit.mNewState = aState; 
> +
> +  nsRefPtr<CameraStateChangeEvent> event =
> +  CameraStateChangeEvent::Constructor(this, aType, eventInit);

nit: indent this line (two spaces).

@@ +1210,5 @@
> +  eventInit.mWidth = mCurrentConfiguration->mPreviewSize.mWidth;
> +  eventInit.mHeight = mCurrentConfiguration->mPreviewSize.mHeight;
> +
> +  nsRefPtr<CameraConfigurationEvent> event =
> +  CameraConfigurationEvent::Constructor(this, NS_LITERAL_STRING("config"), eventInit);

nit: indent this line too.

@@ +1281,5 @@
> +
> +  nsRefPtr<CameraFacesDetectedEvent> event =
> +    CameraFacesDetectedEvent::Constructor(this,
> +    NS_LITERAL_STRING("facesdetected"),
> +    ignored);

nit: indent the above two lines to the same level as 'this'.

@@ +1302,2 @@
>    }
> + 

nit: trailing whitespace.

@@ +1306,3 @@
>  
> +  nsRefPtr<BlobEvent> event = BlobEvent::Constructor(this,
> +    NS_LITERAL_STRING("picture"), eventInit);

nit: indent subsequent lines to the same level as 'this'.

@@ +1319,2 @@
>    nsRefPtr<CameraErrorCallback> errorCb;
> + 

nit: trailing whitespace.

::: dom/camera/moz.build
@@ +16,5 @@
> +EXPORTS.mozilla.dom += [
> +    'CameraDetectedFaceList.h',
> +    'CameraFacesDetectedEvent.h',
> +    'DOMCameraDetectedFace.h',
> +]

Outstanding question from IRC: is this declaration required?

::: dom/camera/test/test_bug975472.html
@@ +134,5 @@
> +        camera.removeEventListener("record", successHandler);
> +        camera.removeEventListener("recorderror", errorHandler);
> +        ok(false, "event startRecording() process succeeded incorrectly");
> +      }
> +      errorHandler = function(evt) {

var errorHandler = ... ?

::: dom/camera/test/test_camera_event.html
@@ +105,5 @@
> +    var blob = evt.data;
> +    var img = new Image();
> +    var test = this._currentTest;
> +    img.onload = function Imgsize() {
> +      ok(this.width == test.pictureSize.width, "The image taken has the width " + 

nit: trailing whitespace.

@@ +107,5 @@
> +    var test = this._currentTest;
> +    img.onload = function Imgsize() {
> +      ok(this.width == test.pictureSize.width, "The image taken has the width " + 
> +                                              this.width + " pictureSize width = " + test.pictureSize.width);
> +      ok(this.height == test.pictureSize.height, "The image taken has the height " + 

nit: trailing whitespace.

@@ +123,5 @@
> +    img.src = window.URL.createObjectURL(blob);
> +  },
> +  shutter: function onShutter () {
> +    Camera._shutter++;
> +    

nit: trailing whitespace.

::: dom/webidl/CameraConfigurationEvent.webidl
@@ +17,5 @@
> +  CameraMode mode = "picture";
> +  DOMString recorderProfile = "cif";
> +  unsigned long width = 0;
> +  unsigned long height = 0;
> +};

Can we change 'width' and 'height' into a CameraSize object?

::: dom/webidl/CameraControl.webidl
@@ +536,5 @@
>    [Pref="camera.control.face_detection.enabled"]
>    attribute CameraFaceDetectionCallback? onFacesDetected;
> +
> +  /* CameraFacesDetectedEvent */
> +  attribute EventHandler onfacesdetected;

Guard with [Pref="camera.control.face_detection.enabled"].

::: dom/webidl/CameraFacesDetectedEvent.webidl
@@ +6,5 @@
> +[Constructor(DOMString type)]
> +interface CameraFacesDetectedEvent : Event
> +{
> +  readonly attribute CameraDetectedFaceList? faces;
> +};

Make sure it is dependent on [Pref="camera.control..."].

Comment 20

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Media events, v1.1

Refresh patch against latest master.

Comment 21

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Camera events, v6

Update patch based on review feedback.

Comment 22

[shinuk]

'CameraDetectedFaceList.webidl' is missing.

Please update 'CameraDetectedFaceList.webidl'.

Comment 23

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Camera events, v7

shinuk: My, that's embarrassing :). I've added it to the new patch thanks!

Comment 24

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8452981 [details] [diff] [review]
Camera events, v7

Review of attachment 8452981 [details] [diff] [review]:
-----------------------------------------------------------------

Getting there!

::: dom/camera/CameraFacesDetectedEvent.cpp
@@ +50,5 @@
> +  bool trusted = event->Init(t);
> +  event->SetTrusted(trusted);
> +  return event.forget();
> +}
> +/* static */ already_AddRefed<CameraFacesDetectedEvent>

nit: add a blank line before this one.

::: dom/camera/DOMCameraControl.cpp
@@ +835,5 @@
>    }
>  
>    aRv = mCameraControl->AutoFocus();
> +  if (!aRv.Failed()) {
> +    mAutoFocusPending = true;

Please clear mAutoFocusOnSuccess/ErrorCb on failure as well.

@@ +921,5 @@
>    }
>  
>    aRv = mCameraControl->TakePicture();
> +  if (!aRv.Failed()) {
> +    mTakePicturePending = true;

Please clear mTakePictureOnSuccess/ErrorCb on failure.

Comment 25

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Camera events, v8

Fix nits. Switch CameraConfigurationEvent and CameraFacesDetectedEvent to use the WebIDL implementation generator instead of manual versions; these evolved from attempts to solve the problem that CameraDetectedFaceList now resolves.

Comment 26

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8453234 [details] [diff] [review]
Camera events, v8

Review of attachment 8453234 [details] [diff] [review]:
-----------------------------------------------------------------

r+ from me with the nits below fixed; now you'll need to get r+ from a DOM peer before you can land.

::: dom/camera/CameraDetectedFaceList.h
@@ +57,5 @@
> +    aFound = aIndex < mFaces.Length();
> +    return aFound ? mFaces[aIndex] : nullptr;
> +  }
> +
> +  void AppendElement(DOMCameraDetectedFace* aFace)

nit:
void
AppendElement(...

::: dom/webidl/CameraFacesDetectedEvent.webidl
@@ +9,5 @@
> +{
> +  readonly attribute CameraDetectedFaceList? faces;
> +};
> +
> +dictionary CameraFacesDetectedEventInit : EventInit {

nit:
... : EventInit
{...

Comment 27

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Camera events, v9

Fixed nits, adding bz for DOM peer review.

Comment 28

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Media events, v1.2

Refreshing patch based on changes in hg tree and adding bz for DOM peer review (note the changes go together but were broken up for review purposes).

Comment 29

[Andrew Osmond [:aosmond] (he/him)]

try: https://tbpl.mozilla.org/?tree=Try&rev=67c0e98cb482

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8455541 [details] [diff] [review]
Camera events, v9

Sorry for the lag here; I'd missed that this just needed WebIDL/DOM API review, not overall review, because there were no other review flags on the patch and I didn't read the review request mail carefully enough.  :(

Why is takePicture using success/fail events instead of returning a Promise? Can those events fire multiple times after a single takePicture call?  Is the idea that someone can listen for pictures being taken when someone _else_ calls takePicture?

It would be good to at least document this, because these questions will _definitely_ come up when we go to standardize the API.  The answer might be that we want both the events _and_ to return a Promise here.  If so, followup bug is ok.

>+     If the success/error callbacks are not used, one may determine success by
>+     waiting for the onrecorderstatechange event. */

s/onrecorderstatechange/recorderstatechange/, right?

Why is this using an overload instead of just making onSuccess optional on the existing method?  Is that just because we don't want people passing in an onError without passing in an onSuccess?

And also, why is startRecording using events and not a Promise?

Similar for setConfiguration.  Why events and not a Promise?

>+interface CameraDetectedFaceList {

Why can we not just use a sequence<CameraDetectedFace>?  I'd much prefer that to adding more list interfaces, and I bet so would the people consuming this API.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8455544 [details] [diff] [review]
Media events, v1.2

r=me on the API bits here.

Comment 32

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Promise/event proposal, v1

mikeh: I've attached a delta between what you reviewed and what I believe bz is suggesting. You can see I've removed the events for the error cases (only triggered in response to calls from the application), switched the success/error CB functions to promises and left in the state change and data events. I also updated CameraManager to use a promise (and provided a conversion dictionary to avoid writing JS::* code ourselves).

Comment 33

[Boris Zbarsky [:bzbarsky]]

Note that promises can deliver success too, not just failure.

Also note that I'm not requiring we switch to promises; I'd just like to understand why they're not a good fit for the API we want, if they're not.

Comment 34

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #33)
> Note that promises can deliver success too, not just failure.
> 
> Also note that I'm not requiring we switch to promises; I'd just like to
> understand why they're not a good fit for the API we want, if they're not.

Understood. I was unaware of promises until you mentioned them and I did some reading today. I think they fit better and put up something to see how mikeh feels.

(In reply to Boris Zbarsky [:bz] from comment #30)
> Comment on attachment 8455541 [details] [diff] [review]
> Camera events, v9
> 
> Sorry for the lag here; I'd missed that this just needed WebIDL/DOM API
> review, not overall review, because there were no other review flags on the
> patch and I didn't read the review request mail carefully enough.  :(
> 

No worries. Would putting review+ first, and then doing review? for you make it clearer, for when I update the patch and thus clear the old review+?

> >+     If the success/error callbacks are not used, one may determine success by
> >+     waiting for the onrecorderstatechange event. */
> 
> s/onrecorderstatechange/recorderstatechange/, right?
>

Good catch, you are correct; I will fix :).
 
> Why is this using an overload instead of just making onSuccess optional on
> the existing method?  Is that just because we don't want people passing in
> an onError without passing in an onSuccess?
> 

I had originally done it that way, but I believe mikeh wanted them split to make it easier when removing the callbacks after the application as switched over (just deleting lines rather than modifying). Perhaps he can comment more on that.

> >+interface CameraDetectedFaceList {
> 
> Why can we not just use a sequence<CameraDetectedFace>?  I'd much prefer
> that to adding more list interfaces, and I bet so would the people consuming
> this API.

I've done so many iterations of this, I've forgotten what I've tried and didn't work. As I recall, the WebIDL event code generator wouldn't accept a sequence in dictionary initializer, but I will try this again and give an update on it.

Comment 35

[Boris Zbarsky [:bzbarsky]]

> Would putting review+ first, and then doing review? for you make it clearer

A bit.  But really, it was reading comprehension fail on my part.

> the WebIDL event code generator wouldn't accept a sequence in dictionary initializer

Might have been before the fixes for bug 1026080 and bug 1023762.

Comment 36

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8458339 [details] [diff] [review]
Promise/event proposal, v1

Review of attachment 8458339 [details] [diff] [review]:
-----------------------------------------------------------------

Just so I understand, with the Promises, we keep the event handlers? And we still have the methods that take callbacks?

If so, that seems good to me; what do you think, Camera App Team?

Comment 37

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Mike Habicher [:mikeh] from comment #36)
> Comment on attachment 8458339 [details] [diff] [review]
> Promise/event proposal, v1
> 
> Review of attachment 8458339 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just so I understand, with the Promises, we keep the event handlers? And we
> still have the methods that take callbacks?
> 
> If so, that seems good to me; what do you think, Camera App Team?

Yes to both keeping event handlers and still retaining callbacks (although the callback versions should be considered deprecated I assume).

Comment 38

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8458339 [details] [diff] [review]
Promise/event proposal, v1

Review of attachment 8458339 [details] [diff] [review]:
-----------------------------------------------------------------

Seems like a good plan to me. As long as we can continue using regular callbacks for the time being, the Camera app shouldn't break and we won't have to worry about timing the landing of a dual-patch.

Comment 39

[Diego Marcos [:dmarcos]]

I would actually remove the callbacks and keep just one way of doing things. We can coordinate the changes with camera.

Comment 40

[Diego Marcos [:dmarcos]]

Comment on attachment 8458339 [details] [diff] [review]
Promise/event proposal, v1

It looks good but I would get rid of the callbacks

Comment 41

[Justin D'Arcangelo [:justindarc]]

(In reply to Diego Marcos [:dmarcos] from comment #39)
> I would actually remove the callbacks and keep just one way of doing things.
> We can coordinate the changes with camera.

Couldn't we just file a follow-up to remove the callbacks? This way we can land a Camera app patch that uses the new API in the meantime.

Comment 42

[Diego Marcos [:dmarcos]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #41)
> (In reply to Diego Marcos [:dmarcos] from comment #39)
> > I would actually remove the callbacks and keep just one way of doing things.
> > We can coordinate the changes with camera.
> 
> Couldn't we just file a follow-up to remove the callbacks? This way we can
> land a Camera app patch that uses the new API in the meantime.

It's fine to me

Comment 43

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Media events, v1.3

Fix build errors for non-B2G targets

Comment 44

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Camera events, v10

Implement promises, update all test cases, update based on comments from review. Merged overloaded CameraControl functions which return a promise into one function because WebIDL complained about different return types.

Comment 45

[Andrew Osmond [:aosmond] (he/him)]

try: https://tbpl.mozilla.org/?tree=Try&rev=ad54caa99b8c

Comment 46

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Camera events, v10.1

Some .orig files snuck in from when I accidently managed to revert all of my changes and trash my hg mq :). Fix the diff the remove those.

Comment 47

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8464401 [details] [diff] [review]
Camera events, v10.1

Review of attachment 8464401 [details] [diff] [review]:
-----------------------------------------------------------------

This looks okay to me, though I'm not a Promise-expert by any means. r+ with the nits below addressed and a few outstanding logic questions answered.

Over to you, bz, with a question about rejecting Promises with values other than NS_ERROR_* in dom/camera/test/test_bug1022766.html.

::: dom/camera/DOMCameraControl.cpp
@@ +751,5 @@
>    ImmediateErrorCallback(CameraErrorCallback* aCallback, const nsAString& aMessage)
>      : mCallback(aCallback)
>      , mMessage(aMessage)
>    { }
>    

nit: extra whitespace.

@@ +796,3 @@
>      }
> +    NS_DispatchToMainThread(new ImmediateErrorCallback(promise,
> +                            NS_ERROR_IN_PROGRESS));

nit: does this all fit on one line?

@@ +1096,5 @@
> +      {
> +        nsRefPtr<Promise> promise = mReleasePromise.forget();
> +        if (promise || mReleaseOnSuccessCb) {
> +          // If we have this event handler, this was a solicited hardware close.
> +          promise->MaybeResolve(JS::UndefinedHandleValue);

'promise' could be null here.

@@ +1172,5 @@
>  
>    switch (aState) {
>      case CameraControlListener::kRecorderStarted:
> +      {
> +        nsRefPtr<Promise> sp = mStartRecordingPromise.forget();

nit: instead of 'sp', use 'promise' like you do (almost) everywhere else.

@@ +1486,3 @@
>      DOM_CAMERA_LOGW("DOM No error handler for aError=0x%x in aContext=%u\n",
>        aError, aContext);
>      return;

I think this will prevent any error callbacks from getting invoked, if there are no promises. Is that right?

::: dom/camera/test/mochitest.ini
@@ +9,5 @@
> +[callback/test_bug975472.html]
> +[callback/test_camera_fake_parameters.html]
> +[callback/test_camera_hardware_face_detection.html]
> +[callback/test_camera_hardware_auto_focus_moving_cb.html]
> +[callback/test_bug1022766.html]

Ugh, two sets of tests. Please file a future-bug to remove the callback versions of the API (and the callback tests) and make a note of it here.

::: dom/camera/test/test_bug1022766.html
@@ +46,5 @@
>    successOne: function test_successOne(focused) {
>      ok(false, "First call to autoFocus() succeeded unexpectedly");
>    },
>    failureOne: function test_failureOne(error) {
> +    ok(error.name == "NS_ERROR_IN_PROGRESS", "First call to autoFocus() failed with: "

bz: can Promises only throw NS_ERROR_ values, or is there a way to pass a more descriptive string here?

::: dom/camera/test/test_camera.html
@@ +102,5 @@
>      this._zoomRatios = this.cameraObj.capabilities.zoomRatios;
>    },
>    takePictureSuccess: function taken_foto(blob) {
> +    ok(blob.size > 100 , "Blob Size Gathered = " + blob.size);
> +    ok("image/" + test.fileFormat ==  blob.type, "Blob Type = " + blob.type);

Niiice. :)

@@ +110,4 @@
>      var img = new Image();
>      var test = this._currentTest;
>      img.onload = function Imgsize() {
>        ok(this.width == test.pictureSize.width, "The image taken has the width " + 

nit: extra whitespace after the +.

@@ +111,5 @@
>      var test = this._currentTest;
>      img.onload = function Imgsize() {
>        ok(this.width == test.pictureSize.width, "The image taken has the width " + 
>                                                this.width + " pictureSize width = " + test.pictureSize.width);
>        ok(this.height == test.pictureSize.height, "The image taken has the height " + 

nit: extra whitespace after the +.

@@ +197,5 @@
>        "preview size (w x h) = " + config.previewSize.width + " x " + config.previewSize.height);
>    },
> +  onPreviewStateChange: function onPreviewStateChange(e) {
> +    if (e.newState === 'started') {
> +      ok(true, "viewfinder is ready and playing");

What's the failure case for this event handler?

::: dom/camera/test/test_camera_2.html
@@ +113,4 @@
>    },
>    shutter: function onShutter () {
>      Camera._shutter++;
>      

nit: extra whitespace on blank line.

@@ +132,5 @@
>      for (var prop in capabilities) {
>        prop = capabilities[prop];
>        ok(camcap[prop] || isFinite(camcap[prop]) || camcap[prop] == null, "Camera Capability: " +
>                      prop + " is exposed, value = " + JSON.stringify(camcap[prop]));
>      } 

nit: extra whitespace after the }.

Comment 48

[Boris Zbarsky [:bzbarsky]]

> bz: can Promises only throw NS_ERROR_ values, or is there a way to pass a more
> descriptive string here?

Promises should generally be rejected with an Error instance.

If you reject with an nsresult, the promise will automatically wrap it up in the corresponding DOMException for you.  The .name will be whatever it is based on the nsresult, but exceptions also have a .message.  Pretty sure dom/base/domerr.msg defines the mappings of nsresults to .name and .message values.

For the rest, if I need to re-review the patch in its entirety I'm not that likely to get to it before I go on vacation on Tuesday.  If the interdiff from the last thing I reviewed is small enough, I might be able to do that...

Comment 49

[Changbin Park]

Attachment: Enable QueryInterface of DOMMediaStream

Comment 50

[Changbin Park]

Comment on attachment 8474355 [details] [diff] [review]
Enable QueryInterface of DOMMediaStream

>diff --git a/content/media/DOMMediaStream.cpp b/content/media/DOMMediaStream.cpp
>index fb111a7..84bb192 100644
>--- a/content/media/DOMMediaStream.cpp
>+++ b/content/media/DOMMediaStream.cpp
>@@ -25,6 +25,7 @@ NS_IMPL_ADDREF_INHERITED(DOMMediaStream, DOMEventTargetHelper)
> NS_IMPL_RELEASE_INHERITED(DOMMediaStream, DOMEventTargetHelper)
> 
> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(DOMMediaStream)
>+  NS_INTERFACE_MAP_ENTRY(DOMMediaStream)
> NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper)
> 
> NS_IMPL_ISUPPORTS_INHERITED0(DOMLocalMediaStream, DOMMediaStream)

Comment 51

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8474355 [details] [diff] [review]
Enable QueryInterface of DOMMediaStream

Review of attachment 8474355 [details] [diff] [review]:
-----------------------------------------------------------------

I will fold this into the Media events patch. (Aside: I was surprised this entry was not created already but sure enough, looking at the macro chain, it needs to be explicitly added!)

Comment 52

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8464401 [details] [diff] [review]
Camera events, v10.1

Sorry for the lag here.  Needed to find a long enough chunk of time to read the whole thing.  :(

> nsDOMCameraControl::SetConfiguration(const CameraConfiguration& aConfiguration,
>+    NS_DispatchToMainThread(new ImmediateErrorCallback(promise,
>+                            NS_ERROR_IN_PROGRESS));

Why do you need to do the MaybeReject() call async?  Seems to me like you can just do:

  promise->MaybeReject(NS_ERROR_IN_PROGRESS);
  return promise.forget();

Unlike the callback case this does not involve any sync invocations of user code, so should be fine.

>+
>+  aRv = mCameraControl->SetConfiguration(config);

Looks like an extra blank line being added there.

> nsDOMCameraControl::ReleaseHardware(const Optional<OwningNonNull<CameraReleaseCallback> >& aOnSuccess,

I'm not sure I follow the logic in this method.  If I call releaseHardware() twice in a row, what happens?  Seems to me like the callbacks passed to the first call, and the promise returned from it, are never notified?

> nsDOMCameraControl::Shutdown()

Should this perhaps reject the promises as needed?

>+nsDOMCameraControl::CreatePromise(ErrorResult& aRv)
>+  nsRefPtr<Promise> promise = Promise::Create(global, aRv);
>+  if (aRv.Failed()) {
>+    return nullptr;
>+  }
>+  return promise.forget();

How about replacing all that with:

  return Promise::Create(global, aRv);

?

> nsDOMCameraControl::OnHardwareStateChange(CameraControlListener::HardwareState aState)

In the kHardwareOpen case, we're not setting mGetCameraOnErrorCb to null unless mGetCameraOnSuccessCb is set.  But nothing forces us to have a non-null mGetCameraOnSuccessCb just because we have a non-null mGetCameraOnErrorCb.  Seems to me we should null out mGetCameraOnErrorCb no matter what.

>     case CameraControlListener::kHardwareClosed:
>+        if (promise || mReleaseOnSuccessCb) {
>+          // If we have this event handler, this was a solicited hardware close.
>+          promise->MaybeResolve(JS::UndefinedHandleValue);

Won't this crash if !promise && mReleaseOnSuccessCb?

And again, shouldn't we null out mReleaseOnErrorCb here no matter what?

> nsDOMCameraControl::OnRecorderStateChange(CameraControlListener::RecorderState aState,

Do we not need to null out the mStartRecordingOnErrorCb somewhere in here?  We used to!

> nsDOMCameraControl::OnConfigurationChange(DOMCameraConfiguration* aConfiguration)
>+    promise->MaybeResolve(aConfiguration);

I'm 99% sure this is wrong, unless you're trying to resolve with a boolean.  Otherwise, pass *aConfiguration, assuming it's known to not be null?  And add a test?

Also, why aConfiguration and not mCurrentConfiguration, which is what all the rest of this method uses?

> nsDOMCameraControl::OnTakePictureComplete(nsIDOMBlob* aPicture)
>+    promise->MaybeResolve(aPicture);

Again, pass *aPicture.  And add a test?

>+++ b/dom/camera/DOMCameraManager.cpp
> nsDOMCameraManager::GetCamera(const nsAString& aCamera,
>+  nsRefPtr<GetCameraCallback> successCallback = nullptr;

No need for the nullptr assignment.

I skipped over reading the tests.

Like I said on IRC, the Promise return values should say, in angle brackets, what type the Promise will resolve to (or "any" if they can resolve to various different types).

>+++ b/dom/webidl/CameraConfigurationEvent.webidl
>+interface CameraConfigurationEvent : Event

Should this really be exposed on all windows?

>+++ b/dom/webidl/CameraStateChangeEvent.webidl
>+interface CameraStateChangeEvent : Event

Likewise.

If they are, doesn't test_interfaces.html need fixing?

r=me with those issues addressed.

Comment 53

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Media events, v1.4

Refreshed against latest gecko tree.

Comment 54

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Camera events, v11

Fix nits and merge in QueryInterface patch.

Comment 55

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Media events, v2

roc: I made a change to DOMMediaStream::TrackChange::Run to workaround a crash occurring when an event was generated for a non-existent track (so deref-ing a null pointer) when shutting down the stream. Oddly this only happened on XP, albeit consistently. There shouldn't be any other changes of note needing a second look :).

broken try: https://tbpl.mozilla.org/?tree=Try&rev=cad35f7fa9e2
fixed try: https://tbpl.mozilla.org/?tree=Try&rev=2a6857e5f30a

Comment 56

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Camera events, v12

mikeh: I made some additions to DOMCameraControl which should be reviewed, specifically fixing the onfocus event (trigger due to ::AutoFocus and the callbacks associated with it, stop triggering on ::OnAutoFocusMoving if moving is false), and fixing the race condition on the preview event and CameraManager::GetCamera promise.

try: https://tbpl.mozilla.org/?tree=Try&rev=304ca5008a81

Comment 57

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8494737 [details] [diff] [review]
Camera events, v12

Review of attachment 8494737 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 58

[Andrew Osmond [:aosmond] (he/him)]

try: https://tbpl.mozilla.org/?tree=Try&rev=304ca5008a81

I don't believe the b2g emulator xpcshell failure is related to this change; it has been passing in previous try attempts today as well. Both patches (media and camera) should be committed together, they were only broken up to ease the review burden.

Comment 59

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/23b18c4d6f6a
https://hg.mozilla.org/integration/b2g-inbound/rev/8e754efd35cd

Comment 60

[Carsten Book [:Tomcat]]

sorry had to backout this changes since this changes might have caused https://tbpl.mozilla.org/php/getParsedLog.php?id=48854505&tree=B2g-Inbound

Comment 61

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Media events, v2.1

Update patch based on latest b2g-inbound

Comment 62

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Camera events, v12.1

Update patch based on latest b2g-inbound

Comment 63

[Andrew Osmond [:aosmond] (he/him)]

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=137dac66671f

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/6a0a585a25ef
https://hg.mozilla.org/integration/b2g-inbound/rev/05df8323b820

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a0a585a25ef
https://hg.mozilla.org/mozilla-central/rev/05df8323b820

Comment 66

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8499503 [details] [diff] [review]
Media events, v2.1

Review of attachment 8499503 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webspeech/recognition/SpeechRecognition.cpp
@@ +942,1 @@
>    return NS_OK;

Nit: shouldn't this return rv?

Comment 67

[Andre Natal]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #66)
> Comment on attachment 8499503 [details] [diff] [review]
> Media events, v2.1
> 
> Review of attachment 8499503 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webspeech/recognition/SpeechRecognition.cpp
> @@ +942,1 @@
> >    return NS_OK;
> 
> Nit: shouldn't this return rv?

Apparently, seems this broken SpeechRecognition at https://github.com/mozilla/gecko-dev/blob/master/dom/media/webspeech/recognition/SpeechRecognition.cpp#L945

NS_IMETHODIMP
SpeechRecognition::GetUserMediaSuccessCallback::OnSuccess(nsISupports* aStream)
{
DOMLocalMediaStream *localStream = nullptr;
nsresult rv = CallQueryInterface(aStream, &localStream);
if (NS_SUCCEEDED(rv)) {
mRecognition->StartRecording(localStream);
}
return NS_OK;
}

After this patch, the test NS_SUCCEEDED(rv) does not result as success anymore.

Comment 68

[Boris Zbarsky [:bzbarsky]]

> nsresult rv = CallQueryInterface(aStream, &localStream);

I filed bug 1094436 on this leak.  Please nominate it for the right releases...

Comment 69

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Fix speech recognition, v1

(In reply to Andre Natal from comment #67)
> Apparently, seems this broken SpeechRecognition at
> https://github.com/mozilla/gecko-dev/blob/master/dom/media/webspeech/
> recognition/SpeechRecognition.cpp#L945
> 
> NS_IMETHODIMP
> SpeechRecognition::GetUserMediaSuccessCallback::OnSuccess(nsISupports*
> aStream)
> {
> DOMLocalMediaStream *localStream = nullptr;
> nsresult rv = CallQueryInterface(aStream, &localStream);
> if (NS_SUCCEEDED(rv)) {
> mRecognition->StartRecording(localStream);
> }
> return NS_OK;
> }
> 
> After this patch, the test NS_SUCCEEDED(rv) does not result as success
> anymore.

I don't see how it can fail in a new exciting way since the cast should be equivalent. Could you give me some STR? Or try out this patch? It is a bit of a long shot, but since the speech recognition function doesn't actually need a DOMLocalMediaStream, just a DOMMediaStream, maybe it is something specific to the former.

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Andre Natal from comment #67)
> Apparently, seems this broken SpeechRecognition at
> https://github.com/mozilla/gecko-dev/blob/master/dom/media/webspeech/
> recognition/SpeechRecognition.cpp#L945
> 
> NS_IMETHODIMP
> SpeechRecognition::GetUserMediaSuccessCallback::OnSuccess(nsISupports*
> aStream)
> {
> DOMLocalMediaStream *localStream = nullptr;
> nsresult rv = CallQueryInterface(aStream, &localStream);
> if (NS_SUCCEEDED(rv)) {
> mRecognition->StartRecording(localStream);
> }
> return NS_OK;
> }
> 
> After this patch, the test NS_SUCCEEDED(rv) does not result as success
> anymore.

If that fails, then localStream is null. So presumably StartRecording wouldn't work even if we called it. I don't see how this patch would have changed the value of localStream. Can you tell us what aStream is and why the CallQueryInterface fails?

Comment 71

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Boris Zbarsky [:bz] from comment #68)
> > nsresult rv = CallQueryInterface(aStream, &localStream);
> 
> I filed bug 1094436 on this leak.  Please nominate it for the right
> releases...

Note: the problem Andre is reporting is that the QI fails....  and thus no leak in that case, though it's not very useful (per Roc).  The question is why/when did it stop working?  Perhaps testing Nightly builds could verify if it worked in the day before this change, and if it failed with this change.  (Andre?)  You can download them from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/<date-time>-mozilla-central

bz: What's hitting the leak, a mochitest?

And I agree: it should never have been a DOMLocalMediaStream anyways (that will break if fed WebAudio streams, PeerConnection streams, etc).  Though I suspect this won't solve Andre's problem.

Andre: if it fails only after you merge newer code, please post a patch for that here.  If it fails without any changes, please post steps to reproduce and we should be able to nail it down.  Thanks

Comment 72

[Boris Zbarsky [:bzbarsky]]

> bz: What's hitting the leak, a mochitest?

I just looked at the code and saw that if the QI succeeds it leaks.  Presumably we have no coverage for this, since mochitests go orange on leaks.

Comment 73

[Andre Natal]

(In reply to Andrew Osmond [:aosmond] from comment #69)
> Created attachment 8517721 [details] [diff] [review]
> Fix speech recognition, v1
> 
> (In reply to Andre Natal from comment #67)
> > Apparently, seems this broken SpeechRecognition at
> > https://github.com/mozilla/gecko-dev/blob/master/dom/media/webspeech/
> > recognition/SpeechRecognition.cpp#L945
> > 
> > NS_IMETHODIMP
> > SpeechRecognition::GetUserMediaSuccessCallback::OnSuccess(nsISupports*
> > aStream)
> > {
> > DOMLocalMediaStream *localStream = nullptr;
> > nsresult rv = CallQueryInterface(aStream, &localStream);
> > if (NS_SUCCEEDED(rv)) {
> > mRecognition->StartRecording(localStream);
> > }
> > return NS_OK;
> > }
> > 
> > After this patch, the test NS_SUCCEEDED(rv) does not result as success
> > anymore.
> 
> I don't see how it can fail in a new exciting way since the cast should be
> equivalent. Could you give me some STR? Or try out this patch? It is a bit
> of a long shot, but since the speech recognition function doesn't actually
> need a DOMLocalMediaStream, just a DOMMediaStream, maybe it is something
> specific to the former.

Andrew, I confirm this patch fixed the issue.

Comment 74

[Andre Natal]

Comment on attachment 8517721 [details] [diff] [review]
Fix speech recognition, v1

Review of attachment 8517721 [details] [diff] [review]:
-----------------------------------------------------------------

Andew, this patch fixed the issue.

Comment 75

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Andre Natal from comment #74)
> Comment on attachment 8517721 [details] [diff] [review]
> Fix speech recognition, v1
> 
> Review of attachment 8517721 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Andew, this patch fixed the issue.

Weird :|. Must be DOMLocalMediaStream vs DOMMediaStream then...? Or some unforeseen consequence of not freeing older media streams. See bug 1094436 for tracking this.

Comment 76

[Randell Jesup [:jesup] (needinfo me)]

Andrew:
Why did the original patch for this remove this from DOMMediaStream.cpp's CC macros:
 tmp->Destroy();

That would have avoided the issues with bug 1078017... and it may well be a better fix than we just landed, which added an "if (tmp->mListener) { tmp->mListener->Forget(); }".  Destroy() nukes mListener and mStream.   (roc?)

Comment 77

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Randell Jesup [:jesup] from comment #76)
> Andrew:
> Why did the original patch for this remove this from DOMMediaStream.cpp's CC
> macros:
>  tmp->Destroy();

I don't know. Probably shouldn't have.

Comment 78

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Randell Jesup [:jesup] from comment #76)
> Andrew:
> Why did the original patch for this remove this from DOMMediaStream.cpp's CC
> macros:
>  tmp->Destroy();
> 
> That would have avoided the issues with bug 1078017... and it may well be a
> better fix than we just landed, which added an "if (tmp->mListener) {
> tmp->mListener->Forget(); }".  Destroy() nukes mListener and mStream.  
> (roc?)

It is a good question. I had to make some changes since DOMMediaStream inherited nsWrapperCache via DOMEventTargetHelper instead of implementing it itself. I don't remember the details to be honest; while this change only got checked in a month or so ago, I must have done those DOMMediaStream changes back in May (this dragged on for a long time). Clearly it was an oversight on my part. Mea culpa.