1020368 - [Camera][Gecko] Remove direct JS_*() calls from CameraRecorderProfiles.cpp/.h

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

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

Attachment: WIP - initial work on removing remaining JS_*() calls from CameraControl, v1

Comment 2

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

Attachment: Remove remaining JS_*() calls from CameraControl, v2

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

Comment 3

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

Attachment: Remove remaining JS_*() calls from CameraControl, v3

Fix a bunch of try failures.

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

Comment 4

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

Attachment: Remove remaining JS_*() calls from CameraControl, v3.1

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 5

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

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?

Comment 6

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

Attachment: Remove remaining JS_*() calls from CameraControl, v3.2

Comment 7

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

Attachment: Part 1. Camera-specific changes, v3.2, r=aosmond

Carrying r+ from aosmond forward.

Comment 8

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

Attachment: Part 2. DOM-specific changes, v3.2

Comment 9

[Boris Zbarsky [:bzbarsky]]

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?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> And likewise here, for the thing in parens.

Several places, in fact.

Comment 11

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

Attachment: Remove remaining JS_*() calls from CameraControl, v3.3

Comment 12

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

Attachment: Part 2. DOM-specific changes, v3.3

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 13

[Boris Zbarsky [:bzbarsky]]

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?

Comment 14

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

Attachment: Part 3. WebIDL changes, v3.3

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

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?

Comment 16

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

Attachment: Remove remaining JS_*() calls from CameraControl, v3.4 r=aosmond,bz

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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> since the presence of "toJSON" breaks the Camera app.

Is there a bug tracking that?

Comment 18

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

(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.

Comment 19

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

https://hg.mozilla.org/integration/b2g-inbound/rev/3fde6fc99b0a

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3fde6fc99b0a

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

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

https://hg.mozilla.org/integration/b2g-inbound/rev/95be75403d70

Adds missing #includes and pre-definitions required to get non-unified builds to work.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/95be75403d70

Comment 25

[Justin Wood (:Callek)]

Attachment: beta-only Fix for MSVC2010 this constructor

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 26

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

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?

Comment 27

[Boris Zbarsky [:bzbarsky]]

> 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 28

[Justin Wood (:Callek)]

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

Comment 29

[Justin Wood (:Callek)]

https://hg.mozilla.org/releases/mozilla-beta/rev/af24cff80f2d