Bug 994912 (camera-events)

[Camera][Gecko] Make CameraControl use Events instead of callback attributes

RESOLVED FIXED in 2.1 S6 (10oct)

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mikeh, Assigned: aosmond)

Tracking

({dev-doc-needed})

unspecified
2.1 S6 (10oct)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [priority])

Attachments

(3 attachments, 23 obsolete attachments)

37.81 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
163.51 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
1.23 KB, patch
anatal
: feedback+
Details | Diff | Splinter Review
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
'wilsonpage likes this'

Updated

5 years ago
Whiteboard: [priority]
Will this replace the callback arguments to takePicture, or coexist with those somehow?
(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.
Assignee

Updated

5 years ago
Assignee: nobody → aosmond
Assignee

Updated

5 years ago
Status: NEW → ASSIGNED
Assignee

Comment 4

5 years ago
Attachment #8416545 - Flags: feedback?(mhabicher)
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?
Attachment #8416545 - Flags: feedback?(wilsonpage)
Attachment #8416545 - Flags: feedback?(mhabicher)
Attachment #8416545 - Flags: feedback?(jdarcangelo)
Attachment #8416545 - Flags: feedback?(dmarcos)
Attachment #8416545 - Flags: feedback?(dflanagan)
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.
Attachment #8416545 - Flags: feedback?(dmarcos)
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.
Attachment #8416545 - Flags: feedback?(wilsonpage)
(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 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.
Attachment #8416545 - Flags: feedback?(jdarcangelo) → feedback+
Assignee

Comment 10

5 years ago
Attachment #8416545 - Attachment is obsolete: true
Attachment #8416545 - Flags: feedback?(dflanagan)
Assignee

Updated

5 years ago
Attachment #8435055 - Attachment description: 8416545: WIP - proposed DOM changes for events, v2 → WIP - proposed DOM changes for events, v2
Assignee

Comment 11

5 years ago
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?
Attachment #8435055 - Flags: feedback?(mhabicher)
Duplicate of this bug: 1022541
No longer depends on: 967802
Assignee

Updated

5 years ago
Depends on: 1027635
See Also: → 1028245
Assignee

Comment 13

5 years ago
Posted patch Camera events, v3 (obsolete) — Splinter Review
Attachment #8435055 - Attachment is obsolete: true
Attachment #8435055 - Flags: feedback?(mhabicher)
Attachment #8444939 - Flags: review?(mhabicher)
Assignee

Comment 14

5 years ago
Posted patch Media events, v1 (obsolete) — Splinter Review
Attachment #8444940 - Flags: review?(roc)
Assignee

Comment 15

5 years ago
Posted patch Camera events, v4 (obsolete) — Splinter Review
Forgot to track some files in hg, adding to patch.
Attachment #8444939 - Attachment is obsolete: true
Attachment #8444939 - Flags: review?(mhabicher)
Attachment #8444942 - Flags: review?(mhabicher)
Assignee

Comment 16

5 years ago
Posted patch Camera events, v4.1 (obsolete) — Splinter Review
File update was incomplete, really do it this time :).
Attachment #8444942 - Attachment is obsolete: true
Attachment #8444942 - Flags: review?(mhabicher)
Attachment #8444945 - Flags: review?(mhabicher)
Assignee

Comment 17

5 years ago
Posted patch Camera events, v4.2 (obsolete) — Splinter Review
Fix a nit and minor bug that I missed last night.
Attachment #8444945 - Attachment is obsolete: true
Attachment #8444945 - Flags: review?(mhabicher)
Attachment #8445174 - Flags: review?(mhabicher)
Assignee

Comment 18

5 years ago
Posted patch Camera events, v5 (obsolete) — Splinter Review
Make changes discussed on IRC (elminate onrecord event, rely on onrecorderstatechange event instead), some other nits.
Attachment #8445174 - Attachment is obsolete: true
Attachment #8445174 - Flags: review?(mhabicher)
Attachment #8445297 - Flags: review?(mhabicher)
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..."].
Attachment #8445297 - Flags: review?(mhabicher) → review-
Assignee

Comment 20

5 years ago
Posted patch Media events, v1.1 (obsolete) — Splinter Review
Refresh patch against latest master.
Attachment #8444940 - Attachment is obsolete: true
Attachment #8452087 - Flags: review+
Assignee

Comment 21

5 years ago
Posted patch Camera events, v6 (obsolete) — Splinter Review
Update patch based on review feedback.
Attachment #8445297 - Attachment is obsolete: true
Attachment #8452088 - Flags: review?(mhabicher)
Assignee

Updated

5 years ago
Blocks: 1035594

Comment 22

5 years ago
'CameraDetectedFaceList.webidl' is missing.

Please update 'CameraDetectedFaceList.webidl'.
Assignee

Comment 23

5 years ago
Posted patch Camera events, v7 (obsolete) — Splinter Review
shinuk: My, that's embarrassing :). I've added it to the new patch thanks!
Attachment #8452088 - Attachment is obsolete: true
Attachment #8452088 - Flags: review?(mhabicher)
Attachment #8452981 - Flags: review?(mhabicher)
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.
Attachment #8452981 - Flags: review?(mhabicher) → review-
Assignee

Comment 25

5 years ago
Posted patch Camera events, v8 (obsolete) — Splinter Review
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.
Attachment #8452981 - Attachment is obsolete: true
Attachment #8453234 - Flags: review?(mhabicher)
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
{...
Attachment #8453234 - Flags: review?(mhabicher) → review+
Assignee

Comment 27

5 years ago
Posted patch Camera events, v9 (obsolete) — Splinter Review
Fixed nits, adding bz for DOM peer review.
Attachment #8453234 - Attachment is obsolete: true
Attachment #8455541 - Flags: review?(bzbarsky)
Assignee

Comment 28

5 years ago
Posted patch Media events, v1.2 (obsolete) — Splinter Review
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).
Attachment #8452087 - Attachment is obsolete: true
Attachment #8455544 - Flags: review?(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.
Attachment #8455541 - Flags: review?(bzbarsky) → feedback+
Flags: needinfo?(aosmond)
Comment on attachment 8455544 [details] [diff] [review]
Media events, v1.2

r=me on the API bits here.
Attachment #8455544 - Flags: review?(bzbarsky) → review+
Assignee

Comment 32

5 years ago
Posted patch Promise/event proposal, v1 (obsolete) — Splinter Review
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).
Attachment #8458339 - Flags: feedback?(mhabicher)
Flags: needinfo?(aosmond)
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.
Assignee

Comment 34

5 years ago
(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.
> 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 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?
Attachment #8458339 - Flags: feedback?(mhabicher)
Attachment #8458339 - Flags: feedback?(jdarcangelo)
Attachment #8458339 - Flags: feedback?(dmarcos)
Attachment #8458339 - Flags: feedback+
Assignee

Comment 37

5 years ago
(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 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.
Attachment #8458339 - Flags: feedback?(jdarcangelo) → feedback+
I would actually remove the callbacks and keep just one way of doing things. We can coordinate the changes with camera.
Comment on attachment 8458339 [details] [diff] [review]
Promise/event proposal, v1

It looks good but I would get rid of the callbacks
Attachment #8458339 - Flags: feedback?(dmarcos) → feedback+
(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.
(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
Assignee

Comment 43

5 years ago
Posted patch Media events, v1.3 (obsolete) — Splinter Review
Fix build errors for non-B2G targets
Attachment #8455544 - Attachment is obsolete: true
Attachment #8464393 - Flags: review+
Assignee

Comment 44

5 years ago
Posted patch Camera events, v10 (obsolete) — Splinter Review
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.
Attachment #8455541 - Attachment is obsolete: true
Attachment #8458339 - Attachment is obsolete: true
Attachment #8464394 - Flags: review?(mhabicher)
Assignee

Comment 46

5 years ago
Posted patch Camera events, v10.1 (obsolete) — Splinter Review
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.
Attachment #8464394 - Attachment is obsolete: true
Attachment #8464394 - Flags: review?(mhabicher)
Attachment #8464401 - Flags: review?(mhabicher)
Assignee

Updated

5 years ago
Depends on: 1046341
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 }.
Attachment #8464401 - Flags: review?(mhabicher)
Attachment #8464401 - Flags: review?(bzbarsky)
Attachment #8464401 - Flags: review+
> 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 50

5 years ago
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)
Attachment #8474355 - Attachment description: While I tried to test WebRTC functionality after applying the Attachment #8464393 and Attachment #8464401, the video is not working. So, I made patch for this. → Enable QueryInterface of DOMMediaStream

Updated

5 years ago
Attachment #8474355 - Flags: review?(aosmond)

Updated

5 years ago
Attachment #8474355 - Attachment is patch: true
Assignee

Comment 51

5 years ago
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!)
Attachment #8474355 - Flags: review?(aosmond) → review+
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.
Attachment #8464401 - Flags: review?(bzbarsky) → review+
Blocks: 1068559
Assignee

Updated

5 years ago
Depends on: 1071090
Assignee

Comment 53

5 years ago
Posted patch Media events, v1.4 (obsolete) — Splinter Review
Refreshed against latest gecko tree.
Attachment #8464393 - Attachment is obsolete: true
Attachment #8493954 - Flags: review+
Assignee

Comment 54

5 years ago
Posted patch Camera events, v11 (obsolete) — Splinter Review
Fix nits and merge in QueryInterface patch.
Attachment #8464401 - Attachment is obsolete: true
Attachment #8474355 - Attachment is obsolete: true
Attachment #8493955 - Flags: review+
Assignee

Comment 55

5 years ago
Posted patch Media events, v2 (obsolete) — Splinter Review
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
Attachment #8493954 - Attachment is obsolete: true
Attachment #8494735 - Flags: review?(roc)
Assignee

Comment 56

5 years ago
Posted patch Camera events, v12 (obsolete) — Splinter Review
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
Attachment #8493955 - Attachment is obsolete: true
Attachment #8494737 - Flags: review?(mhabicher)
Comment on attachment 8494737 [details] [diff] [review]
Camera events, v12

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

LGTM.
Attachment #8494737 - Flags: review?(mhabicher) → review+
Assignee

Comment 58

5 years ago
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.
No longer blocks: 1068559
No longer depends on: 1071090
Keywords: checkin-needed
sorry had to backout this changes since this changes might have caused https://tbpl.mozilla.org/php/getParsedLog.php?id=48854505&tree=B2g-Inbound
Assignee

Updated

5 years ago
Depends on: 1059101
Assignee

Comment 61

5 years ago
Update patch based on latest b2g-inbound
Attachment #8494735 - Attachment is obsolete: true
Attachment #8499503 - Flags: review+
Assignee

Comment 62

5 years ago
Update patch based on latest b2g-inbound
Attachment #8494737 - Attachment is obsolete: true
Attachment #8499505 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6a0a585a25ef
https://hg.mozilla.org/mozilla-central/rev/05df8323b820
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S6 (10oct)
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

5 years ago
(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.
Flags: needinfo?(roc)
Flags: needinfo?(rjesup)
Flags: needinfo?(aosmond)
> nsresult rv = CallQueryInterface(aStream, &localStream);

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

Comment 69

5 years ago
(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.
Flags: needinfo?(aosmond)
Attachment #8517721 - Flags: feedback?(anatal)
(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?
(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
Flags: needinfo?(rjesup)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(anatal)
> 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.
Flags: needinfo?(bzbarsky)

Comment 73

5 years ago
(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.
Flags: needinfo?(roc)
Flags: needinfo?(anatal)

Comment 74

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

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

Andew, this patch fixed the issue.
Attachment #8517721 - Flags: feedback?(anatal) → feedback+
Assignee

Comment 75

5 years ago
(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.
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?)
Flags: needinfo?(roc)
Flags: needinfo?(aosmond)
(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.
Flags: needinfo?(roc)
Assignee

Comment 78

5 years ago
(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.
Flags: needinfo?(aosmond)
You need to log in before you can comment on or make changes to this bug.