Telephony: Make 'calls' array live

RESOLVED WONTFIX

Status

()

defect
RESOLVED WONTFIX
7 years ago
2 years ago

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

Tracking

12 Branch
Points:
---

Firefox Tracking Flags

(blocking-kilimanjaro:-, blocking-basecamp:-)

Details

Attachments

(1 attachment)

Posted patch Patch, v1Splinter Review
This patch uses the new dom bindings to make a live array of nsIDOMTelephonyCall objects (a "platform array object" according to WebIDL). Modifications are not allowed yet it behaves like an array. Win?
Attachment #587831 - Flags: review?(jonas)
Duplicate of this bug: 713959
Shouldn't item() return null when GetCallAt() returns null?  That's what nodelists do...
Comment on attachment 587831 [details] [diff] [review]
Patch, v1

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

r=me with those things fixed.

::: dom/telephony/Telephony.cpp
@@ +117,5 @@
>    }
> +  if (tmp->mCallsArray) {
> +    nsISupports* callsArray = tmp->mCallsArray->ToISupports();
> +    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_PTR(callsArray, TelephonyCallArray,
> +                                                 "mCallsArray")

Please have peterv, or someone that knows cycle collector better than me, look at this. Or simply use NoteXPCOMChild.

::: dom/telephony/TelephonyCallArray.cpp
@@ +59,5 @@
> +already_AddRefed<TelephonyCallArray>
> +TelephonyCallArray::Create(Telephony* aTelephony)
> +{
> +  nsRefPtr<TelephonyCallArray> array = new TelephonyCallArray();
> +  array->mTelephony = aTelephony;

Make the Telephony* a constructor argument?

@@ +67,5 @@
> +NS_IMPL_CYCLE_COLLECTION_CLASS(TelephonyCallArray)
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(TelephonyCallArray)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_PTR(tmp->mTelephony->ToISupports(),
> +                                               Telephony, "mTelephony")

Same thing here for the traverse goop.

@@ +93,5 @@
> +  NS_ASSERTION(aItem, "Null pointer!");
> +
> +  nsCOMPtr<nsIDOMTelephonyCall> item = GetCallAt(aIndex);
> +  if (!item) {
> +    return NS_ERROR_NOT_AVAILABLE;

Just return null for out-of-bounds checks. No exception.
Attachment #587831 - Flags: review?(jonas) → review+
(In reply to Jonas Sicking (:sicking) from comment #3)
> > +    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_PTR(callsArray, TelephonyCallArray,
> > +                                                 "mCallsArray")

This looks like the wrong traverse given that mCallsArray is an XPCOM object. Can't you just do:

    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mCallsArray)

?
I don't understand why you need ToISupports.
Comment on attachment 587831 [details] [diff] [review]
Patch, v1

The TRAVERSE_NATIVE_PTRs definitely look wrong.
TelephonyCallArray needs to traverse its script objects.
This should probably depend on a bug to have bindings that ignore the DOM bindings pref disabled. You don't really want to disable TelephonyCallArray completely just because we find a serious bug in the NodeList bindings.
Attachment #587831 - Flags: review-
(In reply to Peter Van der Beken [:peterv] from comment #5)
> This should probably depend on a bug to have bindings that ignore the DOM
> bindings pref disabled. You don't really want to disable TelephonyCallArray
> completely just because we find a serious bug in the NodeList bindings.

That was added in bug 732175.
blocking-basecamp: --- → +
blocking-kilimanjaro: --- → +
I don't think this is blocking basecamp or Kilimanjaro. This only affects apps which hold on to the calls array returned from telephony.calls. It's easy to work around in apps by simply using "telephony.calls" rather than holding on to the array.

Please feel free to renominate if you disagree. For example if it turns out that telephony.calls is really slow (though that would be surprising).
blocking-basecamp: + → -
blocking-kilimanjaro: + → -

Comment 9

2 years ago
Closing as WONTFIX since FxOS is no more.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.