Comment on attachment 782696 [details] [diff] [review] convert camera control to webidl >+++ b/content/media/webrtc/MediaEngineWebRTCVideo.cpp > + nsGlobalWindow::GetInnerWindowWithId(mWindowId)); What guarantees the window is still alive at that point? And if it's not, what should happen? >+MediaEngineWebRTCVideoSource::HandleEvent(nsISupports* camera) { How about "nsISupports* /* unused */" since it's unused? >+++ b/dom/bindings/Bindings.conf >+ 'nativeType': 'mozilla::nsDOMCameraControl', >+ 'headerFile': 'DOMCameraControl.h', Followup bug to rename stuff? >+ "release": "ReleaseHardware" >+} Please fix the indent on the curly brace... >+++ b/dom/camera/DOMCameraControl.cpp Please file a followup to change the ICameraControl API to better match the WebIDL one, ok? >+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(nsDOMCameraControl, mDOMCapabilities) You need to CC mWindow as well, no? >+nsDOMCameraControl::GetMeteringAreas(JSContext* cx, ErrorResult& aRv) >+ JS::Value areas; That needs to be a JS::Rooted<JS::Value>. >+nsDOMCameraControl::SetMeteringAreas(JSContext* cx, const JS::Handle<JS::Value> aMeteringAreas, ErrorResult& aRv) Please drop the const from the second arg. >+nsDOMCameraControl::GetFocusAreas(JSContext* cx, ErrorResult& aRv) >+ JS::Value value; JS::Rooted<JS::Value>. And maybe name this one "areas" too? >+nsDOMCameraControl::SetFocusAreas(JSContext* cx, const JS::Handle<JS::Value> aFocusAreas, ErrorResult& aRv) Again, no need for const on second arg. >+nsDOMCameraControl::SetExposureCompensation(const Optional<double>& aCompensation, ErrorResult& aRv) This is changing behavior when null or undefined are passed explicitly... Given that API comments on the old API, I guess that's OK, but worth checking our callers to make sure they don't do that. Another option here would be to make this an unrestricted double with a NaN default value, but that would allow passing through explicit NaN too, which I'm not sure is desirable... but again might be something we should watch out for existing code doing. A bunch of these callback interfaces should become WebIDL callbacks, or just event handlers. Please file a followup bug on that? >+nsDOMCameraControl::StartRecording(JSContext* aCx, >+ JS::Handle<JS::Value> aOptions, Followup bug on using a WebIDL dictionary for CameraStartRecordingOptions? >+ nsICameraStartRecordingCallback* onSuccess, Add a MOZ_ASSERT that this is non-null? >+ const Optional<nsICameraErrorCallback*>& onError, We used to allow passing explicit null or undefined, but no longer do. Again, I think that's OK, but worth checking the callers... >+nsDOMCameraControl::GetPreviewStream(JSContext* aCx, >+ JS::Handle<JS::Value> aOptions, Again, followup to use a WebIDL dictionary here. >+ aRv = mCameraControl->GetPreviewStream(size, onSuccess, >+ onError.WasPassed() >+ ? onError.Value() : nullptr); Please fix the indentation. And again watch out for explicit null-or-undefined onError from callers. >+nsDOMCameraControl::AutoFocus(nsICameraAutoFocusCallback* onSuccess, And here, watch out for explicit null-or-undefined onError too. :( +nsDOMCameraControl::TakePicture(JSContext* aCx, JS::Handle<JS::Value> aOptions, aOptions here is already a WebIDL dictionary: CameraPictureOptions. Please use that in the WebIDL, remove the manual dictionary creation in this code, and remove CameraPictureOptions from DummyBinding. And make sure there are followup bugs for the XPIDL dictionary use here (CameraSize/CameraPosition). >+ nsICameraErrorCallback* onError = aOnError.WasPassed() ? aOnError.Value() >+ : nullptr; I'd prefer: nsICameraErrorCallback* onError = aOnError.WasPassed() ? aOnError.Value() : nullptr; >+nsDOMCameraControl::GetPreviewStreamVideoMode(JSContext* aCx, Need a followup on making CameraRecorderOptions a WebIDL dictionary. And the usual thing about watching out for undefined-or-null onError. >+nsDOMCameraControl::ReleaseHardware(const > Optional<nsICameraReleaseCallback*>& onSuccess, >+ mCameraControl->ReleaseHardware(onSuccess.WasPassed() ? onSuccess.Value() >+ : nullptr, onError.WasPassed() ? onError.Value() : nullptr); The indentation there is very odd. Please fix it... perhaps like this: mCameraControl->ReleaseHardware( onSuccess.WasPassed() ? onSuccess.Value() : nullptr, onError.WasPassed() ? onError.Value() : nullptr); and of course the usual "watch out" caveat. >+++ b/dom/camera/FallbackCameraControl.cpp Does this not need to CC mWindow? Or do instances of this class never get allocated or something? >+++ b/dom/webidl/CameraControl.webidl >+ /* one of the vales chosen from capabilities.effects; I know you just copied this, but "values". >+ /* one of the valus chosen from capabilities.sceneModes; And here. >+ attribute any meteringAreas; This needs to be updated to use dictionaries and WebIDL sequences or something... except that those can't be an attribute. Please file a followup bug on this part. >+ attribute any focusAreas; Same here. >+ 'focusDistanceFar' may be infinity. */ >+ readonly attribute double focusDistanceFar; Infinity is not a valid value for WebIDL double. Should this be "unrestricted double"? >+ readonly attribute double exposureCompensation; Can that return NaN? If so, this needs to be unrestricted double. >+ [Throws] >+ void release(optional CameraReleaseCallback onSuccess, optional CameraErrorCallback onError); Please fix the indent. r=me with the above nits, but I'd like to see an updated patch.

And in particular, I need an answer for the window lifetime question....

(In reply to Boris Zbarsky (:bz) from comment #2) > Comment on attachment 782696 [details] [diff] [review] > convert camera control to webidl > > >+++ b/content/media/webrtc/MediaEngineWebRTCVideo.cpp > > + nsGlobalWindow::GetInnerWindowWithId(mWindowId)); > > What guarantees the window is still alive at that point? And if it's not, > what should happen? tbh I make no claim to understand this stuff. That said if the window is dead I'd assume js isn't going to run on a dead window so in this case GetParentObject() returning null is ok, so the rest of this stuff would need to null check mWindow. Onthe other hand its not really clear to me that making a CameraControl makes any sense in the case the window has gone away. > >+MediaEngineWebRTCVideoSource::HandleEvent(nsISupports* camera) { > > How about "nsISupports* /* unused */" since it's unused? I'd worry that'll get out of date, but sure > >+++ b/dom/bindings/Bindings.conf > >+ 'nativeType': 'mozilla::nsDOMCameraControl', > >+ 'headerFile': 'DOMCameraControl.h', > > Followup bug to rename stuff? sure > >+++ b/dom/camera/DOMCameraControl.cpp > > Please file a followup to change the ICameraControl API to better match the > WebIDL one, ok? yeah, I'm not even completely sure why an interface is required... > >+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(nsDOMCameraControl, mDOMCapabilities) > > You need to CC mWindow as well, no? I believe so > >+nsDOMCameraControl::GetMeteringAreas(JSContext* cx, ErrorResult& aRv) > >+ JS::Value areas; > > That needs to be a JS::Rooted<JS::Value>. ok (same for below) I wasn't sure if things like that need to be rooted or not. > >+nsDOMCameraControl::SetMeteringAreas(JSContext* cx, const JS::Handle<JS::Value> aMeteringAreas, ErrorResult& aRv) > > Please drop the const from the second arg. ok > >+nsDOMCameraControl::SetExposureCompensation(const Optional<double>& aCompensation, ErrorResult& aRv) > > This is changing behavior when null or undefined are passed explicitly... > Given that API comments on the old API, I guess that's OK, but worth > checking our callers to make sure they don't do that. I'll check this and others, but don't have time till tomorrow. > A bunch of these callback interfaces should become WebIDL callbacks, or just > event handlers. Please file a followup bug on that? sure, just trying to not write one rewrite dom/camera/ patch :) So I decided all the dictionaries and callbacks could stay as is for this patch. > +nsDOMCameraControl::TakePicture(JSContext* aCx, JS::Handle<JS::Value> > aOptions, > > aOptions here is already a WebIDL dictionary: CameraPictureOptions. Please > use that in the WebIDL, remove the manual dictionary creation in this code, > and remove CameraPictureOptions from DummyBinding. should have seen that :( > > >+++ b/dom/camera/FallbackCameraControl.cpp > > Does this not need to CC mWindow? Or do instances of this class never get > allocated or something? its not actually a class, the way this code handles multiple impls is by conditionally compiling <impl>Control.cpp which include methods of nsDOMCameraControl > r=me with the above nits, but I'd like to see an updated patch. sure

> On the other hand its not really clear to me that making a CameraControl makes any sense > in the case the window has gone away. Right. I'm not sure in what cases we hit this webrtc codepath. Randell, do you know? > I'd worry that'll get out of date Well, if it does then it won't compile and people will have to pick a name and whatnot. ;) > sure, just trying to not write one rewrite dom/camera/ patch I definitely appreciate that. ;)

Ok, so this use of DOMCameraControl is used (at least for now) to get access to the camera for video capture on B2G. Higher levels catch onnavigation and kill off any captures; see dom/media/MediaManager.cpp for details on how these get killed.

That includes iframes being removed, etc? So basically, can we assert that we get a non-null window from that id?

yes, I believe so. It comes from mWindowID in MediaManager.cpp. Though don't look too closely at MediaEngineWebRTC.cpp (the B2G camera part)....

> >+nsDOMCameraControl::SetExposureCompensation(const Optional<double>& aCompensation, ErrorResult& aRv) > > This is changing behavior when null or undefined are passed explicitly... > Given that API comments on the old API, I guess that's OK, but worth > checking our callers to make sure they don't do that. afaict thee are no callers in gaia or m-c > >+nsDOMCameraControl::StartRecording(JSContext* aCx, > >+ JS::Handle<JS::Value> aOptions, > > Followup bug on using a WebIDL dictionary for CameraStartRecordingOptions? > > >+ nsICameraStartRecordingCallback* onSuccess, > > Add a MOZ_ASSERT that this is non-null? > > >+ const Optional<nsICameraErrorCallback*>& onError, > > We used to allow passing explicit null or undefined, but no longer do. I believe the only caller is http://mxr.mozilla.org/gaia/source/apps/camera/js/camera.js#592 where I believe onerror is the function at line 553 or so, but I'm not exactly sure with the kind of odd scoping and possibly two things being called startRecording there. > >+ aRv = mCameraControl->GetPreviewStream(size, onSuccess, > >+ onError.WasPassed() > >+ ? onError.Value() : nullptr); > > Please fix the indentation. And again watch out for explicit > null-or-undefined onError from callers. the only caller seems to not even try and pass an onerror handler. > >+nsDOMCameraControl::AutoFocus(nsICameraAutoFocusCallback* onSuccess, > > And here, watch out for explicit null-or-undefined onError too. :( again the one gaia caller doesn't seem to pass an onError function > >+nsDOMCameraControl::GetPreviewStreamVideoMode(JSContext* aCx, > > Need a followup on making CameraRecorderOptions a WebIDL dictionary. > > And the usual thing about watching out for undefined-or-null onError. looks like gaia doesn't care about errors here either > >+nsDOMCameraControl::ReleaseHardware(const > > Optional<nsICameraReleaseCallback*>& onSuccess, > >+ mCameraControl->ReleaseHardware(onSuccess.WasPassed() ? onSuccess.Value() > >+ : nullptr, onError.WasPassed() ? onError.Value() : nullptr); > > The indentation there is very odd. Please fix it... perhaps like this: > > mCameraControl->ReleaseHardware( > onSuccess.WasPassed() ? onSuccess.Value() : nullptr, > onError.WasPassed() ? onError.Value() : nullptr); > > and of course the usual "watch out" caveat. the one caller I can find passes functions that are definitely non-null

Comment on attachment 786676 [details] [diff] [review] bug 899210 - review comments r=me Please do get those various followups filed, though.

ms2ger mind giving this a quick look? it seems sombody added another callback to cameraControl and some of the attributes should have been marked to possibly return null

Comment on attachment 790895 [details] [diff] [review] convert camera control to webidl Review of attachment 790895 [details] [diff] [review]: ----------------------------------------------------------------- Nothing obviously wrong from a quick look ::: dom/bindings/Bindings.conf @@ +157,5 @@ > > 'CallsList': { > 'nativeType': 'mozilla::dom::telephony::CallsList', > 'headerFile': 'CallsList.h', > + You're adding an empty line at the end of the CallsList block here; please drop it ::: dom/camera/DOMCameraControl.cpp @@ +263,1 @@ > mozilla::idl::CameraStartRecordingOptions options; Followup to move this to webidl, if we don't have one yet ::: dom/camera/DOMCameraControl.h @@ +4,5 @@ > > #ifndef DOM_CAMERA_DOMCAMERACONTROL_H > #define DOM_CAMERA_DOMCAMERACONTROL_H > > +#include "mozilla/dom/BindingDeclarations.h" You should be able to get away with this include if you forward declare Optional. @@ +23,5 @@ > namespace mozilla { > +namespace dom { > +class CameraPictureOptions; > +} > + class ErrorResult; This line shouldn't be indented

https://hg.mozilla.org/mozilla-central/rev/614052b6cbcc https://hg.mozilla.org/mozilla-central/rev/a9a8699e94c3