These are the last files in CameraControl that call directly into the JS API. We should/need to remove these calls and replace them with proper (safer, much simpler to maintain) WebIDL interfaces.

No more JS-engine calls in the CameraControl code. Everything goes through the WebIDL framework. This patch also removes CameraCapabilities::Populate() -- capabilities are now demand-loaded. try-server push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f55e9d48ad29

Fix a bunch of try failures. try re-push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=26daa31d6ea2

This should fix the win32/64 test failures. Apparently calling printf_stderr during layout shutdown crashes on those platforms. re-try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f96881456d0c

Comment on attachment 8509488 [details] [diff] [review] Remove remaining JS_*() calls from CameraControl, v3 Review of attachment 8509488 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/camera/DOMCameraCapabilities.cpp @@ +188,5 @@ > + if (p) { > + profile = new CameraRecorderProfile(this, *p); > + } > + aFound = !!profile; > + return profile; I would have thought a named getter would have the expectation of returning the same object each time, i.e. foo["bar"] === foo["bar"]. ::: dom/camera/DOMCameraCapabilities.h @@ +33,5 @@ > + , public nsWrapperCache > +{ > +public: > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(CameraRecorderVideoProfile) This is just an aside, but I am not sure I understand what SCRIPT_HOLDER_CLASS does. ::: dom/camera/GonkCameraControl.cpp @@ +1373,5 @@ > + mCurrentConfiguration.mRecorderProfile = aConfig.mRecorderProfile; > + const RecorderProfile::Video& video(profile->GetVideo()); > + const Size& size = video.GetSize(); > + int fps = video.GetFramesPerSecond(); > + if (fps <= 0 || size.width <= 0 || size.height <= 0) { The matching algorithm works with width == 0 || height == 0. Is there a reason to start excluding it here?

Carrying r+ from aosmond forward.

Comment on attachment 8512170 [details] [diff] [review] Part 2. DOM-specific changes, v3.2 >+++ b/dom/camera/DOMCameraCapabilities.cpp >+CameraCapabilities::CameraCapabilities(nsPIDOMWindow* aWindow, >+ ICameraControl* mCameraControl) aCameraControl? >+ , mCameraControl(mCameraControl) And likewise here, for the thing in parens. This seems to be missing the actual webidl changes, no?

> And likewise here, for the thing in parens. Several places, in fact.

Fix the bad arguments, and add the missing .webidl file. Not sure how it got dropped--it's very much in my mqueue. Re-r?.

Comment on attachment 8513761 [details] [diff] [review] Part 2. DOM-specific changes, v3.3 >+ CameraRecorderProfile* profile = nullptr; >+ if (!mProfiles.Get(aName, &profile) || !profile) { Wait. Doesn't this leak profile? You want GetWeak here. Or make "profile" an nsRefPtr, which is probably better anyway, so you don't pass around 0-refcount things to Put(). This is still missing the .webidl file, no?

For some reason my patch-splitting script is dropping the .webidl file. Here it is.

Comment on attachment 8513811 [details] [diff] [review] Part 3. WebIDL changes, v3.3 r=me, though maybe we should make some of this stuff serializable?

Incorporating review feedback and carrying r+en forward, try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6e9008033529 This patch does not include jsonifiers since the presence of "toJSON" breaks the Camera app. Once that's fixed, we can add serialization in bug 1011606.

> since the presence of "toJSON" breaks the Camera app. Is there a bug tracking that?

(In reply to Boris Zbarsky [:bz] from comment #17) > Is there a bug tracking that? Bug 1091820, opened (and fixed) yesterday; so I'll be adding the jsonifiers back into this patch.

Bad news, this had non-unified bustage that was hidden behind the other non-unified bustage you had. You weren't around to look at it, so I went ahead and backed out. https://hg.mozilla.org/mozilla-central/rev/23d70439fb8b https://treeherder.mozilla.org/ui/logviewer.html#?job_id=567627&repo=mozilla-central

https://hg.mozilla.org/integration/b2g-inbound/rev/95be75403d70 Adds missing #includes and pre-definitions required to get non-unified builds to work.

Will take whoever is first to review... This bug broke MSVC2010 on beta (Firefox 36) which is still supported, C.F. Bug 1129388 for when I found it. try run (with MSVC2010) before this patch at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4931b2f2b9b And after this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=739946fdf587 Which shows this did indeed fix this issue.

Comment on attachment 8562344 [details] [diff] [review] beta-only Fix for MSVC2010 this constructor Review of attachment 8562344 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine to me, but I can't find any other references to MOZ_THIS_IN_INITIALIZER_LIST in dxr (aside from an #include annotation, to a file that doesn't contain the macro), even for patches that seem to have landed. What am I missing?

> What am I missing? On beta, MOZ_THIS_IN_INITIALIZER_LIST exists just fine right now. See http://mxr.mozilla.org/mozilla-beta/search?string=MOZ_THIS_IN_INITIALIZER_LIST&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-beta On m-c, it got removed in bug 1118076.

Comment on attachment 8562344 [details] [diff] [review] beta-only Fix for MSVC2010 this constructor Approval Request Comment [Feature/regressing bug #]: (this bug) [User impact if declined]: Unable to build on MSVC2010 (official builds use MSVC2013) [Describe test coverage new/current, TreeHerder]: No coverage for 2010, but this code is still compiled/used in MSVC2013 [Risks and why]: No risk, we disable the warning that errors on MSVC2010 and leave it as is on MSVC2013 [String/UUID change made/needed]: None