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

RESOLVED FIXED in 2.1 S8 (7Nov)

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mikeh, Assigned: mikeh)

Tracking

unspecified
2.1 S8 (7Nov)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 10 obsolete attachments)

104.75 KB, patch
mikeh
: review+
Details | Diff | Splinter Review
1.35 KB, patch
mikeh
: review+
Details | Diff | Splinter Review
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
Attachment #8458758 - Attachment is obsolete: true
Attachment #8508950 - Flags: review?(aosmond)
Fix a bunch of try failures.

try re-push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=26daa31d6ea2
Assignee: nobody → mhabicher
Attachment #8508950 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8508950 - Flags: review?(aosmond)
Attachment #8509488 - Flags: review?(aosmond)
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
Attachment #8509488 - Attachment is obsolete: true
Attachment #8509488 - Flags: review?(aosmond)
Attachment #8509709 - Flags: review?(aosmond)
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?
Attachment #8509488 - Flags: review+
Attachment #8509709 - Flags: review?(aosmond) → review+
Carrying r+ from aosmond forward.
Attachment #8512168 - Flags: review+
Attachment #8512170 - Flags: review?(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?
Attachment #8512170 - Flags: review?(bzbarsky) → review-
> 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?.
Attachment #8512170 - Attachment is obsolete: true
Attachment #8513761 - Flags: review?(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?
Attachment #8513761 - Flags: review?(bzbarsky) → review-
Posted patch Part 3. WebIDL changes, v3.3 (obsolete) — Splinter Review
For some reason my patch-splitting script is dropping the .webidl file. Here it is.
Attachment #8513811 - Flags: review?(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?
Attachment #8513811 - Flags: review?(bzbarsky) → review+
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.
Attachment #8512168 - Attachment is obsolete: true
Attachment #8513759 - Attachment is obsolete: true
Attachment #8513761 - Attachment is obsolete: true
Attachment #8513811 - Attachment is obsolete: true
Attachment #8514500 - Flags: review+
> 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.
Duplicate of this bug: 1011606
https://hg.mozilla.org/mozilla-central/rev/3fde6fc99b0a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S8 (7Nov)
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 2.1 S8 (7Nov) → ---
https://hg.mozilla.org/integration/b2g-inbound/rev/95be75403d70

Adds missing #includes and pre-definitions required to get non-unified builds to work.
Depends on: 1093663
https://hg.mozilla.org/mozilla-central/rev/95be75403d70
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S8 (7Nov)
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.
Attachment #8562344 - Flags: review?(mhabicher)
Attachment #8562344 - Flags: review?(bzbarsky)
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?
Attachment #8562344 - Flags: review?(mhabicher) → review+
> 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
Attachment #8562344 - Flags: review?(bzbarsky) → approval-mozilla-beta?
Attachment #8562344 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.