Goals - allow web content to dial out - allow content to mediate incoming calls (accept/reject/merge) - allow content to query transceiver state We should look over the existing device APIs for this and find a nice generalization. The security model for this and WebSMS (bug 674725) ought to be quite similar so each should keep the other in mind.

Here's a crack at this - I'll try to formalize more details. It's useful to note that the industry itself (mobile and fixed line) has been moving to all-IP transport for a long time on the backhaul, but is also making strides (and is on record as stating) to move all services to an IP based network to the consumer. In addition, it's likely that SMS, Video and Audio will all traverse networks in similar ways, though we have to be careful to support alternate providers for each (it's a possibility). With that in mind, to remaining bleeding edge, I would propose merging the common telephony and VoIP APIs usually found in other API sets (like Android) into one. It is redundant to have them separate - the clarity of which network you prefer can be set as a persistent preference globally, or an option on each API call, or by a third-party tool altogether. For example, you might have one thread or app managing the best quality connection at any given moment (a completely separate app) and another thread or app managing how to communicate between your phone and a provider. There are a number of implementations popping up to try and provide web/javascript APIs for telephony that are probably useful references. For pure VoIP almost all of them require a server side component. For regular landline calls almost always assume the phone hardware will secure a circuit and provide some limited set of interaction. This can be hidden from the user and normalized in the APIs themselves unless it is explicitly requested. The reality is that most users want to be able to request communication from their device to an identifier (SIP URI, phone number, SMS shortcode, Facebook ID, etc.) and not worry about the transport method or service. Application developers must be aware of the available transport methods to ensure quality and available features, but it does not take a split API set to do so. I propose two core API classes: telephony - Manage telephony bindings / base APIs to request sessions telephonySession - An object representing an in progress session There will be some additional data types as objects: mediaType - The type of media for a session (G729, G722, G711, H263, H264, etc.) peerData - The party you wish to reach (SIP URI, XMPP String, etc.) protocolType - The type of protocol for a session's signaling (SIP, XMPP, etc.) telephonyProviders - List of telephonyProvider objects telephonyProvider - A single provider, including information on how to reach said provider, what mediaTypes they offer and their ranked weight/quality Note that a session can represent an SMS, too. These are still sessions between two parties (even if they're short-lived). Here are some proposed APIs to start with: telephony: providers listProviders() providers selectProviders(protocolType allowed_protocol, mediaType allowed_media) - Takes an ordered list of allowed media and protocol types. Protocol is checked first (as it's required for call setup), then media. (This strategy needs improvement / more flexibility) - Returns a list of possible providers to try, in order boolean linkProvider(provider providerType, event eventHandler) - This links a provider to an event handler. The idea is that one or more providers could be "listened to" for events like message waiting indicator / etc. to manage messages which happen outside of sessions. There shouldn't be many but there are some. session createSession(providers providerList, mediaType media, peer peerData, int timeout) - Attempt to setup a new channel with the requested mediaType using the provider boolean destroySession(session sessionData) telephonySession: boolean active() void answer(int timeout) void attachEventHandler(eventHandler handler) - Passes any activity regarding this session to the event handler void close() void conference(session sessionData) void hangup() int getState() - Can be connecting / ringing / answered void hold(int timeout) boolean isMuted() boolean isOnHold() void mute() void sendDtmf(int code) void setCallerId(string callerName, string callerNumber) void setScreen(string ScreenUpdate) void setMessage(string Message) void setSpeakerMode(boolean speakerMode) void setVideoMode(boolean videoMode) Stats statistics() - Get statistics about the call (# packets each way, codec selected, length/time of call, etc.) void toggleMute() void transfer(peer peerData) void resume(int timeout) void unmute() This was thrown together within an hour and needs plenty of work, but hopefully it's a good start. If it's off base my apologies! First crack at working on a Mozilla project... Please add suggestions. I referenced: http://wiki.2600hz.org/display/docs/Home, http://developer.android.com/reference/android/net/sip/SipSession.html , http://developer.android.com/reference/android/net/sip/SipAudioCall.html , http://phono.com/docs and others. Please add thoughts.

*dives in* Re call mediation: there's a fine line here, and on one side is CCXML (and SCXML)... and we don't want to fall into that trap :) On the other hand, one can probably provide a more useful API than "delegate this to a system telephony UA". Scoping this will be a real challenge. More broadly: there's a real tension between being overly general (SIP!) and being expedient but inadequate. When one starts to get into some of the functionality that a browser would need to offer to content in order to implement a web conference system, you really get into event-driven SIP interface territory, and it'd be foolish to ignore those lessons.

Those patches are *far* from what the API is going to look like at the end: a lot of use cases can't be fulfilled with these patches. Take them as proof of concept. We do not handle yet the part about receiving phone calls and likely a lot of other things like events telling us what happened with the call (rejected, correctly placed, etc.).

I'm new to this sort of thing, what javascript methods am I calling from your patch?

It would be nice to make these APIs more connected by being able to supply a contact from the Contacts API and have the telephony system select the preferred/best method to contact that… contact. session createSession(providers providerList, contact WebContact, int timeout)

(In reply to John Drinkwater (:beta) from comment #7) > It would be nice to make these APIs more connected by being able to supply a > contact from the Contacts API and have the telephony system select the > preferred/best method to contact that… contact. I don't think they should. IMO, it's up to the Contacts API to specify what is the preferred way to contact the contact then it's up to the application to make sure the correct method is used to contact it (using Web Activities/Intents for example). Asking the phone API to be able to call a contact move too much UI/UX behavior to the phone API and that is not what we want, I think.

(In reply to Mounir Lamouri (:volkmar) from comment #8) > > I don't think they should. IMO, it's up to the Contacts API to specify what > is the preferred way to contact the contact then it's up to the application > to make sure the correct method is used to contact it (using Web > Activities/Intents for example). Just imo, it’s for consistency & lack of duplication. You cant rely on a new application choosing the peerData you’re expecting, and would put a reasonable burden of boilerplate code into a telephony app when it shouldnt need to care for it.

(In reply to John Drinkwater (:beta) from comment #9) > (In reply to Mounir Lamouri (:volkmar) from comment #8) > > > > I don't think they should. IMO, it's up to the Contacts API to specify what > > is the preferred way to contact the contact then it's up to the application > > to make sure the correct method is used to contact it (using Web > > Activities/Intents for example). > > Just imo, it’s for consistency & lack of duplication. You cant rely on a new > application choosing the peerData you’re expecting, and would put a > reasonable burden of boilerplate code into a telephony app when it shouldnt > need to care for it. I believe someone could write a JS library that would prevent the boilerplate code.

(In reply to Mounir Lamouri (:volkmar) from comment #10) > (In reply to John Drinkwater (:beta) from comment #9) > > (In reply to Mounir Lamouri (:volkmar) from comment #8) > > > > > > I don't think they should. IMO, it's up to the Contacts API to specify what > > > is the preferred way to contact the contact then it's up to the application > > > to make sure the correct method is used to contact it (using Web > > > Activities/Intents for example). > > > > Just imo, it’s for consistency & lack of duplication. You cant rely on a new > > application choosing the peerData you’re expecting, and would put a > > reasonable burden of boilerplate code into a telephony app when it shouldnt > > need to care for it. > > I believe someone could write a JS library that would prevent the > boilerplate code. Would implementing the boilerplate code in jQuery Mobile be a good idea? Considering it's widespread use, I think it'd help distribution of WebAPI.

Ok, this is the beginning of the API at https://wiki.mozilla.org/WebAPI/WebTelephony#Proposed_API . Everything lives in dom/telephony. We have a testing component that will replace the android radio for desktop mochitests. I've written a single mochitest that simulates an outgoing call. Next step is to implement the radio piece, waiting on some more investigation from Kyle before I go any further there.

need a full review

Whats the status here? RIL is coming along, we will blocked on this very soon. Please review asap.

If you all are at a stage where this is nearing completion or a design has been chosen to fix the problem we really should schedule a security review. Please take a look at the calendar (https://mail.mozilla.com/home/ckoenig@mozilla.com/Security%20Review.html) and let me know the date/time slot you would like.

We have partially implemented the WebTelephony API in JS in bug 699256 and will be using that as a basis for future work. Note that it's ifdef'ed out as it's highly experimental still. It's also unclear at this point whether it can stay in JS at all.

Here's a refresh. Philipp, what do you think of the js changes?

Comment on attachment 583204 [details] [diff] [review] Untested patch I only skimmed the C++ part since you wanted me to focus on the JS stuff. Overall I agree with the change since it makes thing simpler. Although I'm sad to see nsTelephonyWorker losing control over the whole phone state. It was kind of neat how it was the puppet master that controlled the RIL and the audio system. But that's not really important. See detailed comments below. >+interface nsITelephoneCallback : nsISupports >+{ >+ // 'callState' uses the CALL_STATE values from nsITelephone. >+ void oncallstatechange(in unsigned long index, Nit: I would suggest calling this parameter 'callIndex'. This isn't just any index, it's the call index assigned by the RIL (which also happens to be 1-based). So it's a special thing and special names for special things generally help documenting that fact. > [scriptable, uuid(f6baa721-665e-403e-8a98-acaa0d8bf267)] >-interface nsITelephone : nsISupports { >+interface nsITelephone : nsISupports >+{ >+ const unsigned short CALL_STATE_UNKNOWN = 0; >+ const unsigned short CALL_STATE_DIALING = 1; >+ const unsigned short CALL_STATE_RINGING = 2; >+ const unsigned short CALL_STATE_BUSY = 3; >+ const unsigned short CALL_STATE_CONNECTING = 4; >+ const unsigned short CALL_STATE_CONNECTED = 5; >+ const unsigned short CALL_STATE_HOLDING = 6; >+ const unsigned short CALL_STATE_HELD = 7; >+ const unsigned short CALL_STATE_RESUMING = 8; >+ const unsigned short CALL_STATE_DISCONNECTING = 9; >+ const unsigned short CALL_STATE_DISCONNECTED = 10; >+ const unsigned short CALL_STATE_INCOMING = 11; So I'm not 100% sure I understand why we need to introduce another set of states. What's wrong with setting the state to the strings here already? Or even in the RIL, which is how it was. I'm not saying I'm opposed, just trying to understand the reasons. (And perhaps putting them in a comment outlining the design decision.) >diff --git a/dom/telephony/nsTelephonyWorker.js b/dom/telephony/nsTelephonyWorker.js >--- a/dom/telephony/nsTelephonyWorker.js >+++ b/dom/telephony/nsTelephonyWorker.js ... >+ set activeCall(val) { >+ if (val && !this._activeCall) { >+ // Enable audio. >+ switch (val.state) { >+ case nsITelephone.CALL_STATE_INCOMING: >+ gAudioManager.phoneState = nsIAudioManager.PHONE_STATE_RINGTONE; >+ break; >+ case nsITelephone.CALL_STATE_DIALING: // Fall through... >+ case nsITelephone.CALL_STATE_CONNECTED: >+ gAudioManager.phoneState = nsIAudioManager.PHONE_STATE_IN_CALL; >+ gAudioManager.setForceForUse(nsIAudioManager.USE_COMMUNICATION, >+ nsIAudioManager.FORCE_NONE); >+ break; >+ default: >+ throw new Error() This could use a useful error message, e.g. Error("Invalid call state for active call: " + val.state) >+ } >+ } >+ else if (!val && this._activeCall) { Nit: in JS we cuddle the else. > _callbacks: null, >+ _enumeratingCallbacks: false, > > registerCallback: function registerCallback(callback) { ... > > unregisterCallback: function unregisterCallback(callback) { ... > >+ _deliverCallback(name, args, callbacks) { ... Ugh, this is pretty inelegant, incorrect in some places, and probably also slow. I had all this logic already in Telephony.js. Here's an adaption of that logic: _callbacks: null, registerCallback: function registerCallback(callback) { if (!this._callbacks) { this._callbacks = []; } if (this._callbacks.indexOf(callback) != -1) { throw new Error("Already registered this callback!"); } this._callbacks.push(callback); }, unregisterCallback: function unregisterCallback(callback) { let index; if (this._callbacks && (index = this._callbacks.indexOf(callback) != -1)) { this._callbacks.splice(index, 1); } }, _deliverCallback(name, args) { if (!this._callbacks) { return; } // We need to worry about callback registration state mutations during the // callback firing. The behaviour we want is to *not* call any callbacks that // are added during the firing and to *not* call any callbacks that are removed // during the firing. To address this, we make a copy of the callback list // before dispatching and then double-check that each callback is still // registered before calling it. let callbacks = this._callbacks.slice(); for (let i = 0; i < callbacks.length; i++) { let cb = callbacks[i]; if (this._callbacks.indexOf(cb) == -1) { return; } cb.apply(null, args); } } >diff --git a/dom/telephony/ril_consts.js b/dom/telephony/ril_consts.js >--- a/dom/telephony/ril_consts.js >+++ b/dom/telephony/ril_consts.js ... >+var EXPORTED_SYMBOLS = [ >+ CALL_STATE_ACTIVE, >+ CALL_STATE_HOLDING, >+ CALL_STATE_DIALING, >+ CALL_STATE_ALERTING, >+ CALL_STATE_INCOMING, >+ CALL_STATE_WAITING, >+ CALL_PRESENTATION_ALLOWED, >+ CALL_PRESENTATION_RESTRICTED, >+ CALL_PRESENTATION_UNKNOWN, >+ CALL_PRESENTATION_PAYPHONE, >+ >+ TOA_INTERNATIONAL, >+ TOA_UNKNOWN > ]; EXPORTED_SYMBOLS contains a list of strings naming the variables you want to export. This list could get pretty tedious pretty quickly, so why not just do const EXPORTED_SYMBOLS = Object.keys(this); to simply export all the constants from this file? You're already importing them into a 'RIL' namespace object in nsTelephonyWorker.js, so I see no harm in simply exporting everything. >diff --git a/dom/telephony/ril_worker.js b/dom/telephony/ril_worker.js >--- a/dom/telephony/ril_worker.js >+++ b/dom/telephony/ril_worker.js ... >+ _handleChangedCallState: function _handleChangedCallState(changedCall) { >+ let message = {type: "callStateChange", >+ call: {index: changedCall.index, >+ state: changedCall.state, >+ number: changedCall.number, >+ name: changedCall.name >+ } Nits: (a) closing brace on the previous line, (b) all other "foobar changed" notifications are "foobarchange" (all lowercase). This naming convention goes all the way from the RIL to the nsTelephonyWorker and the DOM. I see no reason to break it now. >+ }; >+ this.sendDOMMessage(message); >+ }, >+ >+ _handleDisconnectedCall: function _handleDisconnectedCall(disconnectedCall) { >+ let message = {type: "callDisconnected", >+ call: {index: disconnectedCall.index} >+ }; Ditto (a) and (b). >- onSetMute: function onSetMute() { >- }, >- Please don't remove this. It's still being called by the RIL object. (At some point we might want to figure out what to do with responses from the radio, but I don't think it should be in this bug ;))

Comment on attachment 583204 [details] [diff] [review] Untested patch Oh, I forgot to mention this earlier: please make sure you rebase on trunk. Kyle has already implemented DTMF tones. If you're not going to include that work in your patch for this bug, let's make sure there's a follow-up to bring it back.

This gets us up to current functionality in Telephony.js, plus it gets all the event stuff right. Needs to be tested on a phone but in my hacked up test without a real RIL looks like it works ok. Philipp, can you look over the JS changes? Mounir or Sicking for the C++ :)

Comment on attachment 583851 [details] [diff] [review] Patch, v1 Review of attachment 583851 [details] [diff] [review]: ----------------------------------------------------------------- The stuff that I looked at was all solid. Great job, this patch is quite a beast. I pretty much only have nits, most of them regarding naming/commenting. I'd like them to be addressed, but if you don't want to do them in this patch/bug I'm totally fine with doing them as follow-ups. I would even help you out on some of them. ::: dom/telephony/Telephony.cpp @@ +90,5 @@ > + // XXX This is not what Jonas wants. He wants it to be live. > + if (!JS_FreezeObject(aCx, arrayObj)) { > + return NS_ERROR_FAILURE; > + } > + It took me a while to come around, but I think Jonas is making sense here. navigator.telephony.activeCalls is an attribute and I'd expect to be able to access it and obtain the same Array object over and over (navigator.telephony.activeCalls === navigator.telephony.activeCalls). If that's impractical, perhaps we should make this a method (getActiveCalls()). It's a read-only attribute anyway. File follow-up bug for this? ::: dom/telephony/nsIDOMTelephony.idl @@ +74,5 @@ > + attribute nsIDOMEventListener onsignalstrengthchange; > + readonly attribute jsval signalStrength; > + readonly attribute jsval operator; > + readonly attribute jsval radioState; > + readonly attribute jsval cardState; Those radio & network-related properties and events should really go somewhere else. We can leave them in for now, but let's find a new home for them. I filed bug 713849 for this. ::: dom/telephony/nsITelephone.idl @@ +72,5 @@ > + const unsigned short CALL_STATE_HELD = 7; > + const unsigned short CALL_STATE_RESUMING = 8; > + const unsigned short CALL_STATE_DISCONNECTING = 9; > + const unsigned short CALL_STATE_DISCONNECTED = 10; > + const unsigned short CALL_STATE_INCOMING = 11; I understand why but it makes me sad. @@ +93,5 @@ > void unregisterCallback(in nsITelephoneCallback callback); > + > + // Will continue calling callback.enumerateCallState until the callback > + // returns false. > + void enumerateAllCalls(in nsITelephoneCallback callback); Is this going to enumerate data calls too? The name suggests it a bit (ENUMERATE ALL THE CALLS!!1!). I think we might want to call it "enumerateCalls" and then later we can also have a "enumerateDataCalls" without it being confusing. I also have some doc nits here and elsewhere in this file, but I'll gladly address that (myself even) in a follow-up. No need to drag this big patch out any further for that. ::: dom/telephony/nsTelephonyWorker.js @@ +35,5 @@ > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > +"use strict"; <3 @@ -40,3 +42,5 @@ > > > > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > > > > +var RIL = {}; > > +Cu.import("resource://gre/modules/ril_consts.js", RIL); Nit: s/var/const @@ -122,5 @@ > - // for component contexts and can result in crashes. This onerror > - // handler has to make sure that it calls preventDefault on the > - // incoming event. > - event.preventDefault(); > - Any reason you removed this comment? Is it no longer true? @@ +151,5 @@ > + case "callDisconnected": > + // This one will handle its own notifications. > + this.handleCallDisconnected(message.call); > + return; > + case "enumerateallcalls": Not a big fan of the varying naming convention here. Would prefer "enumerateAllCalls". Not feeling strongly, so feel free to punt to follow-up. @@ +211,5 @@ > + * system. > + */ > + handleCallStateChange: function handleCallStateChange(call) { > + debug("handleCallStateChange: " + JSON.stringify(call)); > + call.state = convertRILCallState(call.state); I would prefer to use different attribute names for the two different state values. In fact, this doesn't even have to be an attribute of "call" at all since we're going to throw that object away anyway and only use its attribute values. @@ +221,3 @@ > } > + this._deliverCallback("callStateChanged", > + [call.index, call.state, call.number]); (See my comments on ril_worker.js regarding 'index' v 'callIndex'.) @@ +245,5 @@ > + let callback = this._enumerationCallbacks.shift(); > + let activeCallIndex = this.activeCall ? this.activeCall.index : -1; > + for (let i in calls) { > + let call = calls[i]; > + call.state = convertRILCallState(call.state); Ditto. @@ +367,5 @@ > + // is still registered before calling it. > + if (this._callbacks) { > + let callbacks = this._callbacks.slice(); > + for each (let callback in callbacks) { > + if (this._callbacks.indexOf(callback) != -1) { Nit: Could bail out early here to save some indentation in the rest of the loop. if (this._callbacks.indexOf(callback) == -1) { continue; } ::: dom/telephony/ril_consts.js @@ +221,5 @@ > const DOM_CARDSTATE_NETWORK_LOCKED = "network_locked"; > const DOM_CARDSTATE_NOT_READY = "not_ready"; > const DOM_CARDSTATE_READY = "ready"; > > +var EXPORTED_SYMBOLS = Object.keys(this); nit: s/var/const ::: dom/telephony/ril_worker.js @@ +1081,5 @@ > + }, > + > + _handleChangedCallState: function _handleChangedCallState(changedCall) { > + let message = {type: "callStateChange", > + call: {index: changedCall.index, This property of the message used to be called 'callIndex' for a reason. This index is not something we control, e.g. by sorting the calls into an array or something. It's an oddball 1-based number, assigned by the RIL and typically known as the 'callIndex' or 'gsmIndex' in its interfaces. I would prefer to keep this nomenclature for those reasons. The only place where I used the simple 'index' name was in the REQUEST_GET_CURRENT_CALLS handler and I think that was a mistake (which I shall fix). @@ +1363,5 @@ > > /** > + * Get a list of all current calls. > + */ > + enumerateAllCalls: function enumerateAllCalls() { (See discussion of this name in my review of nsITelephone.idl.) @@ +1368,5 @@ > + if (DEBUG) debug("Sending all current calls"); > + let calls = []; > + for each (let call in this.currentCalls) { > + calls.push(call); > + } If only there was an Object.values() to match Object.keys()... ::: ipc/ril/Ril.h @@ +40,5 @@ > > #ifndef mozilla_ipc_Ril_h > #define mozilla_ipc_Ril_h 1 > > +#include "jsstdint.h" Why is this needed here now all of a sudden? I see no other changes to this file and nothing in Ril.cpp either. ::: modules/libpref/src/init/all.js @@ +3414,5 @@ > // enable JS dump() function. > pref("browser.dom.window.dump.enabled", false); > > +// URL for the dialer application. > +pref("dom.telephony.app.phone.url", "file:///data/local/dialer.html"); I believe this is now a localhost:8888 URL. vingtetun can tell you details. Also, I don't think this should be in global prefs. b2g/app/b2g.js seems a better location.

(In reply to Philipp von Weitershausen [:philikon] from comment #22) > It took me a while to come around, but I think Jonas is making sense here. Long term Jonas wants it to be a live-updating object like NodeList. That's tough at the moment, mostly because I'm not sure how the new NodeList bindings work (it's on my list of things to dive into soon). In the meantime we've decided to just do this, where we return an array that will be === until the call list changes (at which point a new array will be returned). > File follow-up bug for this? Bug 713959. > I filed bug 713849 for this. Thanks. > I think we might want to call it "enumerateCalls" Ok. > Nit: s/var/const Er... Can you do a Cu.import on a const? > Any reason you removed this comment? Is it no longer true? The big "you will crash" part is no longer (was never?) true. > Not a big fan of the varying naming convention here. Would prefer > "enumerateAllCalls". Not feeling strongly, so feel free to punt to follow-up. I thought I had fixed that, oops. Will fix now. > I would prefer to use different attribute names for the two different state > values. In fact, this doesn't even have to be an attribute of "call" at all > since we're going to throw that object away anyway and only use its > attribute values. We're not, though. It gets saved in the activeCall slot and that checks state value as well to set audio options. I can make it a different name, or make the original 'rilState' or something, but it doesn't seem worth it imo. Up to you. > (See my comments on ril_worker.js regarding 'index' v 'callIndex'.) Ok. > > + let callback = this._enumerationCallbacks.shift(); > ... > > + call.state = convertRILCallState(call.state); Fixed this one to just be a local var since we're not saving it anywhere. > Nit: Could bail out early here to save some indentation in the rest of the > loop. Ok. > > +var EXPORTED_SYMBOLS = Object.keys(this); > nit: s/var/const Ok. > The only place where I used the simple 'index' name was in the > REQUEST_GET_CURRENT_CALLS handler and I think that was a mistake (which I > shall fix). I got it. > Why is this needed here now all of a sudden? I see no other changes to this > file and nothing in Ril.cpp either. RIL.h uses uint8_t which doesn't exist on windows without some additional help. > I believe this is now a localhost:8888 URL. vingtetun can tell you details. I can't find him, but won't check in without this. > Also, I don't think this should be in global prefs. b2g/app/b2g.js seems a > better location. Ok. That didn't exist when I first wrote this patch!

With comments addressed.

Comment on attachment 584648 [details] [diff] [review] Patch, v2 Needs review on C++ parts still, whichever one of you can get to it first :)

Comment on attachment 584648 [details] [diff] [review] Patch, v2 This removes the default pref value for "dom.telephony.app.phone.url" entirely. Was this intended? Also, when I skimmed the C++ part before I overlooked that navigator.mozTelephony is actually not available at all unless the page is whitelisted by the "dom.telephony.app.phone.url" preference. I understand the intent, but this seems rather limiting. I'm sure other parts in the system, not just the dialer, will want to listen for some call-related events. Take the "incoming" event, for instance. We actually need something to listen to that and then launch the dialer app in the first place.

We'll use intents to launch the dialer.

(In reply to Philipp von Weitershausen [:philikon] from comment #26) > Take the "incoming" event, for instance. We actually need something > to listen to that and then launch the dialer app in the first place. Unless we route this the other way and have dialer app always on and then have it request foreground on "incoming". See also https://github.com/andreasgal/gaia/issues/220.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #27) > We'll use intents to launch the dialer. Sure. Still need to be able to know when to launch it, right?

(In reply to Philipp von Weitershausen [:philikon] from comment #26) > This removes the default pref value for "dom.telephony.app.phone.url" > entirely. Was this intended? No, I forgot to qref. It's in b2g.js now like you suggested.

(In reply to Philipp von Weitershausen [:philikon] from comment #29) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #27) > > We'll use intents to launch the dialer. > > Sure. Still need to be able to know when to launch it, right? Yes. The dialer app would be registered to listen for an INCOMING_CALL intent or something to that effect, and an incoming call on the radio stack in gecko would send that intent and launch its handler, the dialer (with cooperation from the app launcher), if it's not already running.

There were gaia bugs resulting in the incoming notification being dropped. Now I'm chasing down a problem where |var call window.navigator.mozTelephony.liveCalls[0];| is resulting in undefined |call|.

Doesn't look like liveCalls is really set up for incoming calls, only for dialed calls. Should be fixed with the patch here.

(In reply to ben turner [:bent] from comment #33) > Doesn't look like liveCalls is really set up for incoming calls Um, it should be! I had that working before.

If I open the dialer app and then call the phone, the call receiver works fine. If I let the homescreen launch the dialer app, then even though the same code runs as when the dialer is already open, the incoming call doesn't work. If I wait 500ms after the dialer launches, still no luck. I'm suspecting it's something to do with let currentState = this.telephone.currentState; let states = currentState.currentCalls; for (let i = 0; i < states.length; i++) { let state = states[i]; let call = new TelephonyCall(this.telephone, state.callIndex); because when the dialer is launched by the homescreen, the call should already exist but the *mozTelephony* instance doesn't, so the call has to be restored here. But I'm getting lost trying to track down nsITelephone.

(In the case of mozTelephony already created, Telephony._processCallState() will create a call when it comes in.)

(In reply to Chris Jones [:cjones] [:warhammer] from comment #35) > because when the dialer is launched by the homescreen, the call should > already exist but the *mozTelephony* instance doesn't, so the call has to be > restored here. Oh I see, yes. > But I'm getting lost trying to track down nsITelephone. It's in dom/telephony/nsTelephonyWorker.js. There might indeed be a bug here that prevents newly launched windows from seeing existing call state in navigator.mozTelephony.liveCalls. Because of the rewriting happening in this bug, we haven't spent much time refining these things.

Alright, no worries. Eagerly awaiting these patches.

My C++ is always perfect, land away! ;)

Unless there is something unexpected happening, I will review this today.

Comment on attachment 584648 [details] [diff] [review] Patch, v2 Review of attachment 584648 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. It took more time than expected on Friday and finishing the review during the week-end wouldn't have been a good idea ;) BTW, splitting this behemoth in multiple patches would have make things easier. A few general comments in addition of the specific one below: - You have a lot of private ctor and you have a ::Create() method that allows the construction of the object. I don't really get why you are using such pattern; - Same thing for dtor; - You might want to use MOZ_ASSERT() instead of NS_ASSERTION sometimes; - I think you should get ride of TelephonyCommon.h; - It seems that you are requesting a context in some calls when mScriptContext->GetNativeContext() would be enough. Canceling the review given that I would like to discuss some points and see the new patch. Also, I think you should ask as sr to Jonas. ::: dom/base/Navigator.cpp @@ +1032,5 @@ > +Navigator::GetMozTelephony(nsIDOMTelephony** aTelephony) > +{ > + nsCOMPtr<nsIDOMTelephony> telephony = mTelephony; > + > + if (!telephony) { nit: That pattern seems more consistent with what we have in this file: *aTelephony = nsnull; if (!mTelephony) { ... } NS_ADDREF(*aTelephony = mTelephony); ::: dom/base/nsDOMClassInfo.cpp @@ +4209,5 @@ > + DOM_CLASSINFO_MAP_ENTRY(nsIDOMTelephony) > + DOM_CLASSINFO_MAP_ENTRY(nsIDOMEventTarget) > + // Wish we didn't have to expose this, but nsITelephone is implemented in JS > + // so we have no choice... > + DOM_CLASSINFO_MAP_ENTRY(nsITelephoneCallback) Could you file a bug? and put the bug number in the comment. ::: dom/telephony/CallEvent.cpp @@ +50,5 @@ > + NS_ASSERTION(aCall, "Null pointer!"); > + > + nsRefPtr<CallEvent> event = new CallEvent(); > + > + event->mCall = aCall; Why isn't that part of the ctor? ::: dom/telephony/CallEvent.h @@ +69,5 @@ > + ToISupports() const > + { > + return static_cast<nsIDOMEvent*>( > + static_cast<nsDOMEvent*>( > + const_cast<CallEvent*>(this))); Why do you need to cast_cast here except to have the method being const? Why do you need the method to be const? @@ +75,5 @@ > + > + nsresult > + Dispatch(nsIDOMEventTarget* aTarget, const nsAString& aEventType, > + bool aCanBubble = false, bool aCanCancel = false, > + bool* aPreventDefaultCalled = nsnull) I would prefer to remove the default values for those parameters. If you really want to, you can keep the last one but AFAIK, we try to not use optional argument when we can. Also, we should open a follow-up to have a helper method in nsContentUtils that does that. There is an equivalent of this method in SmsManager and BatteryManager already. @@ +76,5 @@ > + nsresult > + Dispatch(nsIDOMEventTarget* aTarget, const nsAString& aEventType, > + bool aCanBubble = false, bool aCanCancel = false, > + bool* aPreventDefaultCalled = nsnull) > + { Could you move that definition to the cpp file? @@ +92,5 @@ > + > + bool dummy; > + bool* preventDefaultCalled = aPreventDefaultCalled ? > + aPreventDefaultCalled : > + &dummy; nit: aPreventDefaultCalled ? aPreventDefaultCalled : &dummy seems a bit nicer. @@ +96,5 @@ > + &dummy; > + rv = aTarget->DispatchEvent(thisEvent, preventDefaultCalled); > + NS_ENSURE_SUCCESS(rv, rv); > + > + return NS_OK; |return rv;| maybe? unless you really want the warning to show up. @@ +105,5 @@ > + : nsDOMEvent(nsnull, nsnull) > + { } > + > + ~CallEvent() > + { } Why a private dtor? ::: dom/telephony/Makefile.in @@ +51,5 @@ > include $(topsrcdir)/dom/dom-config.mk > > +EXPORTS_NAMESPACES = mozilla/dom/telephony > + > +EXPORTS_mozilla/dom/telephony = TelephonyPublic.h This name is unfortunate. I wonder if you could fix that by having a public/ subdir which is exporting that file. ::: dom/telephony/Telephony.cpp @@ +66,5 @@ > + > + if (aSourceArray.IsEmpty()) { > + arrayObj = JS_NewArrayObject(aCx, 0, nsnull); > + } > + else { nit: } else { @@ +70,5 @@ > + else { > + nsTArray<jsval> valArray; > + valArray.SetLength(aSourceArray.Length()); > + > + JSObject* global = JS_GetGlobalForScopeChain(aCx); You can use mScriptContext->GetNativeGlobal() to get the global. Maybe you should rely on mScriptContext for the context and the global in this method and do not ask them to the caller (which means make this part of the class)? Otherwise, I think it would be better to ask for both of them. @@ +99,5 @@ > +} // anonymous namespace > + > +Telephony::Telephony(nsPIDOMWindow* aOwner) > +: mActiveCall(nsnull), mCallsArray(nsnull), mSpeakerVolume(0), > + mDoNotDisturb(false), mEnabled(true), mRooted(false) I think doing this is more readable and creates better diff when changing the list: : mFoo , mBar @@ +101,5 @@ > +Telephony::Telephony(nsPIDOMWindow* aOwner) > +: mActiveCall(nsnull), mCallsArray(nsnull), mSpeakerVolume(0), > + mDoNotDisturb(false), mEnabled(true), mRooted(false) > +{ > + mOwner = aOwner; AFAIK, you have to set mScriptContext too. And you should put this in the initialization list. @@ +112,5 @@ > + } > + > + if (mListenerManager) { > + mListenerManager->Disconnect(); > + } You don't need that. @@ +146,5 @@ > + > + if (mEnabled) { > + // XXX What should we do here? > + mEnabled = false; > + } Please open a follow-up for that and put the bug number here. @@ +154,5 @@ > +Telephony::SwitchActiveCall(TelephonyCall* aCall) > +{ > + if (mActiveCall) { > + // Put the call on hold? > + NS_NOTYETIMPLEMENTED("Implement me!"); ditto @@ +207,5 @@ > + > +NS_IMETHODIMP > +Telephony::Dial(const nsAString& aNumber, nsIDOMTelephonyCall** aResult) > +{ > + NS_ENSURE_ARG(!aNumber.IsEmpty()); I would prefer: NS_ENSURE_TRUE(!aNumber.IsEmpty(), NS_ERROR_INVALID_ARG); or even: if (!aNumber.IsEmpty()) { return NS_ERROR_INVALID_ARG; } @@ +227,5 @@ > +NS_IMETHODIMP > +Telephony::GetMuted(bool* aMuted) > +{ > + nsresult rv = mTelephone->GetMicrophoneMuted(aMuted); > + NS_ENSURE_SUCCESS(rv, rv); |return mTelephone->GetMicrophoneMuted(aMuted);| unless you care about the warning. @@ +236,5 @@ > +NS_IMETHODIMP > +Telephony::SetMuted(bool aMuted) > +{ > + nsresult rv = mTelephone->SetMicrophoneMuted(aMuted); > + NS_ENSURE_SUCCESS(rv, rv); ditto @@ +245,5 @@ > +NS_IMETHODIMP > +Telephony::GetSpeakerEnabled(bool* aSpeakerEnabled) > +{ > + nsresult rv = mTelephone->GetSpeakerEnabled(aSpeakerEnabled); > + NS_ENSURE_SUCCESS(rv, rv); ditto @@ +254,5 @@ > +NS_IMETHODIMP > +Telephony::SetSpeakerEnabled(bool aSpeakerEnabled) > +{ > + nsresult rv = mTelephone->SetSpeakerEnabled(aSpeakerEnabled); > + NS_ENSURE_SUCCESS(rv, rv); ditto @@ +302,5 @@ > + > + nsresult rv = > + nsContentUtils::WrapNative(aCx, JS_GetGlobalForScopeChain(aCx), > + mActiveCall->ToISupports(), aActive); > + NS_ENSURE_SUCCESS(rv, rv); ditto @@ +310,5 @@ > + > +NS_IMETHODIMP > +Telephony::SetActive(JSContext* aCx, const jsval& aActive) > +{ > + if (!JSVAL_IS_PRIMITIVE(aActive)) { You want: if (!aActive.isObject()) { return NS_ERROR_INVALID_ARG; } @@ +319,5 @@ > + GetWrappedNativeOfJSObject(aCx, obj, getter_AddRefs(wrappedNative)); > + > + nsCOMPtr<nsIDOMTelephonyCall> native = > + do_QueryInterface(wrappedNative->Native()); > + if (native) { I think you can do that instead: nsCOMPtr<nsIDOMTelephonyCall> native = do_QueryInterface(nsContentUtils::XPConnect()->GetNativeOfWrapper( mScriptContext->GetNativeContext(), &aActive.toObject())); NS_ENSURE_TRUE(native, NS_ERROR_INVALID_ARG); And you can remove all the lines before. @@ +321,5 @@ > + nsCOMPtr<nsIDOMTelephonyCall> native = > + do_QueryInterface(wrappedNative->Native()); > + if (native) { > + TelephonyCall* call = static_cast<TelephonyCall*>(native.get()); > + if (call->mTelephony == this) { if (call->mTelephony != this) { return THE_ERROR_YOU_WANT; } @@ +336,5 @@ > +Telephony::GetLiveCalls(JSContext* aCx, jsval* aLiveCalls) > +{ > + JSObject* calls = mCallsArray; > + if (!calls) { > + nsresult rv = nsTArrayToJSArray(aCx, mCalls, &calls); You can use mScriptContext->GetNativeContext() to get the context. @@ +364,5 @@ > + return NS_ERROR_INVALID_ARG; > + } > + > + nsresult rv = mTelephone->StartTone(aDTMFChar); > + NS_ENSURE_SUCCESS(rv, rv); |return mTelephone->StartTone(aDTMFChar);| unless you really want the warning to show up. @@ +373,5 @@ > +NS_IMETHODIMP > +Telephony::StopTone() > +{ > + nsresult rv = mTelephone->StopTone(); > + NS_ENSURE_SUCCESS(rv, rv); ditto @@ +392,5 @@ > +NS_IMPL_EVENT_HANDLER(Telephony, radiostatechange); > +NS_IMPL_EVENT_HANDLER(Telephony, cardstatechange); > +NS_IMPL_EVENT_HANDLER(Telephony, signalstrengthchange); > + > +// This is probably going to move to a different API. Why do you have all these methods in the IDL if they are not implemented and might move to a different API? @@ +436,5 @@ > + if (!strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID)) { > + NS_ConvertUTF16toUTF8 newPhoneAppURL(aData); > + if (!newPhoneAppURL.Equals(mPhoneAppURL, > + nsCaseInsensitiveCStringComparator())) { > + Disable(); Do we really want to support this for the moment? I mean we have no use of that for the moment and Disable() isn't even working... I don't really like adding some code to support a feature we don't have yet and isn't implemented yet given that it might end-up staying here for no reasons. @@ +440,5 @@ > + Disable(); > + } > + } > + else { > + NS_WARNING("Unknown observer topic!"); That might look nicer: if (strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID)) { NS_WARNING(...); return NS_OK; } // do what you want @@ +453,5 @@ > +{ > + // If we already know about this call then just update its readyState. > + for (PRUint32 index = 0; index < mCalls.Length(); index++) { > + nsRefPtr<TelephonyCall>& tempCall = mCalls[index]; > + if (tempCall->CallIndex() == aCallIndex) { nit: if (!tempCall->CallIndex() != aCallIndex) { continue; } @@ +484,5 @@ > + NS_ASSERTION(event, "This should never fail!"); > + > + nsresult rv = > + event->Dispatch(ToIDOMEventTarget(), NS_LITERAL_STRING("incoming")); > + NS_ENSURE_SUCCESS(rv, rv); |return event->Dispatch(...);| unless you really want the warning. @@ +519,5 @@ > + > +NS_IMETHODIMP > +Telephony::Onoperatorchange(const jsval& aOperator) > +{ > + NS_WARNING("Onoperatorchange not implemented"); Why do you use NS_NOTYETIMPLEMENTED() above and NS_WARNING here? (this apply for other methods below) ::: dom/telephony/Telephony.h @@ +96,5 @@ > + nsIDOMEventTarget* > + ToIDOMEventTarget() const > + { > + return static_cast<nsDOMEventTargetWrapperCache*>( > + const_cast<Telephony*>(this)); Again for the const_cast. @@ +122,5 @@ > + mCallsArray = nsnull; > + } > + > + nsITelephone* > + Telephone() const Why not GetTelephone? ::: dom/telephony/TelephonyCall.cpp @@ +46,5 @@ > + > +USING_TELEPHONY_NAMESPACE > + > +TelephonyCall::TelephonyCall(nsPIDOMWindow* aOwner) > +: mCallIndex(-1), mCallState(nsITelephone::CALL_STATE_UNKNOWN), mLive(false) nit: : mCallIndex(-1) , mCallState(...) , mLive(false) It is more readable and adding a new member will produce a cleaner diff. @@ +48,5 @@ > + > +TelephonyCall::TelephonyCall(nsPIDOMWindow* aOwner) > +: mCallIndex(-1), mCallState(nsITelephone::CALL_STATE_UNKNOWN), mLive(false) > +{ > + mOwner = aOwner; Why mOwner isn't in the initialization list? Also, I believe you have to initialize mScriptContext. @@ +57,5 @@ > + NS_ASSERTION(!mLive, "Should never die while we're live!"); > + > + if (mListenerManager) { > + mListenerManager->Disconnect(); > + } You do not need that anymore. @@ +75,5 @@ > + nsRefPtr<TelephonyCall> call = new TelephonyCall(aOwner); > + > + call->mTelephony = aTelephony; > + call->mNumber = aNumber; > + call->mCallIndex = aCallIndex; Why don't you add those to the ctor? or at least add a ctor with those? Given that it is the only usage of that private ctor, it would make sense I believe. @@ +77,5 @@ > + call->mTelephony = aTelephony; > + call->mNumber = aNumber; > + call->mCallIndex = aCallIndex; > + > + call->ChangeReadyStateInternal(aCallState, false); You could even add this to the ctor. @@ +123,5 @@ > + stateString.AssignLiteral("incoming"); > + } > + else { > + NS_NOTREACHED("Unknown state!"); > + } Please, use a switch block. @@ +133,5 @@ > + NS_ASSERTION(mLive, "Should be live!"); > + mTelephony->RemoveCall(this); > + mLive = false; > + } > + else if (!mLive) { nit: } else if (!mLive) { @@ +226,5 @@ > + ChangeReadyStateInternal(nsITelephone::CALL_STATE_CONNECTING, true); > + } > + else { > + NS_WARNING("Answer on non-incoming call ignored"); > + } This would be more readable: if (mCallState != nsITelephone::CALL_STATE_INCOMING) { NS_WARNING("Answer on non-incoming call ignored"); return NS_OK; } // everything else @@ +235,5 @@ > +NS_IMETHODIMP > +TelephonyCall::HangUp() > +{ > + if (mCallState != nsITelephone::CALL_STATE_DISCONNECTING && > + mCallState != nsITelephone::CALL_STATE_DISCONNECTED) { Like ::Answer, you could warn and do an early exit if the call is disconnecting or disconnected. ::: dom/telephony/TelephonyCall.h @@ +78,5 @@ > + nsDOMEventTargetWrapperCache) > + > + static already_AddRefed<TelephonyCall> > + Create(nsPIDOMWindow* aOwner, Telephony* aTelephony, const nsAString& aNumber, > + PRUint16 aCallState, PRUint32 aCallIndex = PRUint32(-1)); Can't use use PR_UINT32_MAX? @@ +84,5 @@ > + nsIDOMEventTarget* > + ToIDOMEventTarget() const > + { > + return static_cast<nsDOMEventTargetWrapperCache*>( > + const_cast<TelephonyCall*>(this)); I guess you are const_cast'ing to have a const method but why do you need that? @@ +100,5 @@ > + ChangeReadyStateInternal(aCallState, true); > + } > + > + PRUint32 > + CallIndex() const Why not GetCallIndex? Same for CallState below. @@ +105,5 @@ > + { > + return mCallIndex; > + } > + > + PRUint16 CallState() const nit: PRUint16 CallState() const @@ +112,5 @@ > + } > + > +private: > + TelephonyCall(nsPIDOMWindow* aOwner); > + ~TelephonyCall(); Why do you want those to be private? ::: dom/telephony/TelephonyCommon.h @@ +49,5 @@ > +#include "nsDOMEventTargetWrapperCache.h" > +#include "nsServiceManagerUtils.h" > +#include "nsStringGlue.h" > +#include "nsTArray.h" > +#include "nsTObserverArray.h" I don't really like the idea of this file because you are going to include headers you do not need. Also, you shouldn't include jsval.h in headers but jspubtd.h. ::: dom/telephony/TelephonyPublic.h @@ +40,5 @@ > +#ifndef mozilla_dom_telephony_telephonypublic_h__ > +#define mozilla_dom_telephony_telephonypublic_h__ > + > +#include "nsIDOMTelephony.h" > +#include "nsPIDOMWindow.h" Those lines could be converted to: class nsIDOMTelephony; class nsPIDOMWindow; ::: dom/telephony/nsIDOMTelephony.idl @@ +50,5 @@ > + > + attribute boolean muted; > + attribute boolean speakerEnabled; > + attribute double speakerVolume; > + attribute boolean doNotDisturb; What is that exactly? And why is that in the IDL if it's not implemented? @@ +53,5 @@ > + attribute double speakerVolume; > + attribute boolean doNotDisturb; > + > + [implicit_jscontext] > + attribute jsval active; nit: say what this is returning. @@ +56,5 @@ > + [implicit_jscontext] > + attribute jsval active; > + > + [implicit_jscontext] > + readonly attribute jsval liveCalls; nit: say what this is returning. bikeshedding: I would prefer currentCalls, it's clearer for me. @@ +66,5 @@ > + [optional] in unsigned long intervalDuration); > + > + attribute nsIDOMEventListener onincoming; > + > + //XXX philikon's additions nit: remove that comment. @@ +74,5 @@ > + attribute nsIDOMEventListener onsignalstrengthchange; > + readonly attribute jsval signalStrength; > + readonly attribute jsval operator; > + readonly attribute jsval radioState; > + readonly attribute jsval cardState; Why do you add all those attributes to this interface? why jsval? Given that it's not implemented, I would prefer to have them removed for the moment. ::: dom/telephony/nsIDOMTelephonyCall.idl @@ +45,5 @@ > +interface nsIDOMTelephonyCall : nsIDOMEventTarget > +{ > + readonly attribute DOMString number; > + > + readonly attribute DOMString readyState; Shouldn't this be named |state| instead? @@ +50,5 @@ > + > + void answer(); > + void hangUp(); > + > + attribute nsIDOMEventListener onreadystatechange; If I understood it correctly, this is called every time one of the eventlistener is called. Do we really want that? IMO, we could have a statechange event and have scripts checking what the state is. ::: dom/telephony/nsITelephone.idl @@ +54,2 @@ > > //XXX philikon's additions nit: remove this. ::: dom/telephony/nsTelephonyWorker.js @@ +73,5 @@ > + return nsITelephone.CALL_STATE_RINGING; > + case RIL.CALL_STATE_INCOMING: > + return nsITelephone.CALL_STATE_INCOMING; > + case RIL.CALL_STATE_WAITING: > + return nsITelephone.CALL_STATE_HELD; // XXX This may not be right... Please open a follow-up and put the bug number in a TODO comment instead of adding an XXX comment. ::: js/xpconnect/src/dom_quickstubs.qsconf @@ +473,5 @@ > + 'nsIDOMTelephony.*', > + 'nsIDOMTelephonyCall.*', > + 'nsIDOMCallEvent.*', > + 'nsITelephone.*', > + 'nsITelephoneCallback.*', Have you been contacted by a callcenter to make sure those were as fast as possible? :)

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #41) > ::: dom/base/nsDOMClassInfo.cpp > @@ +4209,5 @@ > > + DOM_CLASSINFO_MAP_ENTRY(nsIDOMTelephony) > > + DOM_CLASSINFO_MAP_ENTRY(nsIDOMEventTarget) > > + // Wish we didn't have to expose this, but nsITelephone is implemented in JS > > + // so we have no choice... > > + DOM_CLASSINFO_MAP_ENTRY(nsITelephoneCallback) > > Could you file a bug? and put the bug number in the comment. Maybe the DOM telephony class could use a delegate friend class to implement nsITelephoneCallback? That way it wouldn't have to expose nsITelephoneCallback to content. > @@ +310,5 @@ > > + > > +NS_IMETHODIMP > > +Telephony::SetActive(JSContext* aCx, const jsval& aActive) > > +{ > > + if (!JSVAL_IS_PRIMITIVE(aActive)) { > > You want: > if (!aActive.isObject()) { Modulo the ! sign, I think. > @@ +392,5 @@ > > +NS_IMPL_EVENT_HANDLER(Telephony, radiostatechange); > > +NS_IMPL_EVENT_HANDLER(Telephony, cardstatechange); > > +NS_IMPL_EVENT_HANDLER(Telephony, signalstrengthchange); > > + > > +// This is probably going to move to a different API. > > Why do you have all these methods in the IDL if they are not implemented and > might move to a different API? Because we haven't figured it out yet. So yeah we might as well wait until we have. See bug 713849 for this. > @@ +74,5 @@ > > + attribute nsIDOMEventListener onsignalstrengthchange; > > + readonly attribute jsval signalStrength; > > + readonly attribute jsval operator; > > + readonly attribute jsval radioState; > > + readonly attribute jsval cardState; > > Why do you add all those attributes to this interface? why jsval? Because most of them are objects with attributes. > Given that it's not implemented, I would prefer to have them removed for the > moment. Probably not a bad idea. Once again bug 713849. > ::: dom/telephony/nsTelephonyWorker.js > @@ +73,5 @@ > > + return nsITelephone.CALL_STATE_RINGING; > > + case RIL.CALL_STATE_INCOMING: > > + return nsITelephone.CALL_STATE_INCOMING; > > + case RIL.CALL_STATE_WAITING: > > + return nsITelephone.CALL_STATE_HELD; // XXX This may not be right... > > Please open a follow-up and put the bug number in a TODO comment instead of > adding an XXX comment. This originated in my code, so I filed the bug for it: bug 714968.

(In reply to Philipp von Weitershausen [:philikon] from comment #42) > > @@ +310,5 @@ > > > + > > > +NS_IMETHODIMP > > > +Telephony::SetActive(JSContext* aCx, const jsval& aActive) > > > +{ > > > + if (!JSVAL_IS_PRIMITIVE(aActive)) { > > > > You want: > > if (!aActive.isObject()) { > > Modulo the ! sign, I think. No. |aActive| has to be an object so we want to leave the method early if it is not. > > @@ +74,5 @@ > > > + attribute nsIDOMEventListener onsignalstrengthchange; > > > + readonly attribute jsval signalStrength; > > > + readonly attribute jsval operator; > > > + readonly attribute jsval radioState; > > > + readonly attribute jsval cardState; > > > > Why do you add all those attributes to this interface? why jsval? > > Because most of them are objects with attributes. Interesting. At a first glance, I would do: readonly attribute double signalStrength; readonly attribute DOMString operator; readonly attribute DOMString radioState; readonly attribute DOMString cardState; But I wanted to underline that you were specifying something in an IDL with no implementation and no comment explaining what those attributes are. > > ::: dom/telephony/nsTelephonyWorker.js > > @@ +73,5 @@ > > > + return nsITelephone.CALL_STATE_RINGING; > > > + case RIL.CALL_STATE_INCOMING: > > > + return nsITelephone.CALL_STATE_INCOMING; > > > + case RIL.CALL_STATE_WAITING: > > > + return nsITelephone.CALL_STATE_HELD; // XXX This may not be right... > > > > Please open a follow-up and put the bug number in a TODO comment instead of > > adding an XXX comment. > > This originated in my code, so I filed the bug for it: bug 714968. Thanks :)

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #43) > > > @@ +74,5 @@ > > > > + attribute nsIDOMEventListener onsignalstrengthchange; > > > > + readonly attribute jsval signalStrength; > > > > + readonly attribute jsval operator; > > > > + readonly attribute jsval radioState; > > > > + readonly attribute jsval cardState; > > > > > > Why do you add all those attributes to this interface? why jsval? > > > > Because most of them are objects with attributes. > > Interesting. At a first glance, I would do: > readonly attribute double signalStrength; > readonly attribute DOMString operator; > readonly attribute DOMString radioState; > readonly attribute DOMString cardState; > > But I wanted to underline that you were specifying something in an IDL with > no implementation and no comment explaining what those attributes are. Yeah, hence the XXX :). It's not as simple as declaring those DOMStrings/doubles, so let's defer that discussion to bug 713849.

Changes to attachment 584648 [details] [diff] [review]. (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #41) > - You have a lot of private ctor and you have a ::Create() method that > allows the construction of the object. I don't really get why you are using > such pattern; It's nice to ensure that no one ever gets a half-constructed object. Some of the objects are not valid if they haven't had some sort of an Init method called on them so rather than make people remember to call 'new Foo()' followed by 'foo->Init()' I bundle both into Create(). It's very useful. > - Same thing for dtor; These are all refcounted objects so calling 'delete foo' should be disallowed in all but the Release method. > - You might want to use MOZ_ASSERT() instead of NS_ASSERTION sometimes; That didn't exist when I first wrote this patch. Going forward we can change if you feel strongly. > - I think you should get ride of TelephonyCommon.h; It includes the things that I almost always need. I'd rather have that then duplicate it everywhere. > - It seems that you are requesting a context in some calls when > mScriptContext->GetNativeContext() would be enough. Sort of. See below. > > +Navigator::GetMozTelephony(nsIDOMTelephony** aTelephony) > ... > nit: > That pattern seems more consistent with what we have in this file: > *aTelephony = nsnull; That code is as old as the hills, no need to keep that pattern in new code. > ::: dom/base/nsDOMClassInfo.cpp > ... > > + // Wish we didn't have to expose this, but nsITelephone is implemented in JS > > + // so we have no choice... > > + DOM_CLASSINFO_MAP_ENTRY(nsITelephoneCallback) > > Could you file a bug? and put the bug number in the comment. I don't see the point of putting the bug number in the comment. Will file though. > ::: dom/telephony/CallEvent.cpp > ... > Why isn't that part of the ctor? See above. Create is replacing Constructor and Init. > ::: dom/telephony/CallEvent.h > ... > > + ToISupports() const > > + { > > + return static_cast<nsIDOMEvent*>( > > + static_cast<nsDOMEvent*>( > > + const_cast<CallEvent*>(this))); > > Why do you need to cast_cast here except to have the method being const? Why > do you need the method to be const? The const method simply means that the object won't be changed by running this member function. It's correct, but means that I have to un-const 'this'. I don't see any compelling reason to change this. > > + Dispatch(nsIDOMEventTarget* aTarget, const nsAString& aEventType, > > + bool aCanBubble = false, bool aCanCancel = false, > > + bool* aPreventDefaultCalled = nsnull) > > I would prefer to remove the default values for those parameters. If you > really want to, you can keep the last one but AFAIK, we try to not use > optional argument when we can. They'll never be true anyway, so I'll remove the args altogether. > Could you move that definition to the cpp file? No, it doesn't depend on anything in the cpp and it would lose its inline-ability. > |return rv;| maybe? unless you really want the warning to show up. I do. Any time something fails (with the exception of content code) I want to know about it. > > +EXPORTS_mozilla/dom/telephony = TelephonyPublic.h > > This name is unfortunate. I wonder if you could fix that by having a public/ > subdir which is exporting that file. I don't think this should be exported any more (only needed by Navigator.cpp) so I'm just going to remove the export altogether. Also renamed to TelephonyFactory.h. > You can use mScriptContext->GetNativeGlobal() to get the global. Yeah, looks like using the calling context isn't what we want (it would be fine if we weren't hanging on to these objects, but since we decided to do so we have to create them in the owning context). > I think doing this is more readable and creates better diff when changing > the list: > : mFoo > , mBar I prefer prettier code as opposed to prettier diffs, decided to leave as is. > > +Telephony::Telephony(nsPIDOMWindow* aOwner) > > +: mActiveCall(nsnull), mCallsArray(nsnull), mSpeakerVolume(0), > > + mDoNotDisturb(false), mEnabled(true), mRooted(false) > > +{ > > + mOwner = aOwner; > > AFAIK, you have to set mScriptContext too. I didn't need it before, but now I guess I do :( > And you should put this in the initialization list. Can't. C++ doesn't allow you to do anything but call base constructors or set immediate members in initailizer lists. > > + if (mListenerManager) { > > + mListenerManager->Disconnect(); > > + } > > You don't need that. Sigh. I did when I wrote this patch! > > + if (mEnabled) { Since writing this we've more or less decided not to support changing the dialer. I'll drop this code for now. > > +Telephony::SwitchActiveCall(TelephonyCall* aCall) > ... > ditto I'm going to keep this since we're going to want this to work very soon. This implementation isn't 100% complete yet, we just need to get it in so people stop modifying the JS version. Holding calls will have to be supported before we ship anything. > > + NS_ENSURE_ARG(!aNumber.IsEmpty()); > > I would prefer: > NS_ENSURE_TRUE(!aNumber.IsEmpty(), NS_ERROR_INVALID_ARG); Until we remove these macros I don't see the problem using them (they're used all over the place). > You want: > if (!aActive.isObject()) { > return NS_ERROR_INVALID_ARG; > } I kinda don't like this new api, but i switched it anyway. Yuck. > I think you can do that instead: > nsCOMPtr<nsIDOMTelephonyCall> native = > do_QueryInterface(nsContentUtils::XPConnect()->GetNativeOfWrapper( > mScriptContext->GetNativeContext(), &aActive.toObject())); Ok. > if (call->mTelephony != this) { > return THE_ERROR_YOU_WANT; I opted not to do this since I would have to repeat a bunch of 'return NS_ERROR_INVALID_ARG'. > Why do you have all these methods in the IDL if they are not implemented and > might move to a different API? Since we're going to move it I just removed it. > nit: > if (!tempCall->CallIndex() != aCallIndex) { > continue; I prefer letting the loop continue on its own without making it explicit. > > + nsITelephone* > > + Telephone() const > > Why not GetTelephone? Because it can never fail and return null. > Why don't you add those to the ctor? or at least add a ctor with those? > Given that it is the only usage of that private ctor, it would make sense I > believe. For the moment TelephonyCall::Create is infallible (it may not be in the future), so I could just make it a constructor. I prefer to keep it consistent with Telephony however and keep the create method. > > + call->ChangeReadyStateInternal(aCallState, false); > > You could even add this to the ctor. Yes, but only because it is currently infallible. If that changes I'll want to catch it in the create method. > Please, use a switch block. Ok. > This would be more readable: Ok. > Like ::Answer, you could warn and do an early exit if the call is > disconnecting or disconnected. Ok. > Can't use use PR_UINT32_MAX? I could, but I don't think that is any nicer. > Why not GetCallIndex? Same for CallState below. Because they can't fail. > nit: > PRUint16 > CallState() const Oops. > Also, you shouldn't include jsval.h in headers but jspubtd.h. Now that I removed a bunch of the jsval IDL methods I don't think i need this anymore. > What is that exactly? And why is that in the IDL if it's not implemented? It's part of the telephony API. We'll have to implement it all before we can ship. > nit: say what this is returning. Ok. > nit: say what this is returning. Ok. > bikeshedding: I would prefer currentCalls, it's clearer for me. Take it up with jonas :) > > + //XXX philikon's additions > > nit: remove that comment. It's all gone now. > > + readonly attribute DOMString readyState; > > Shouldn't this be named |state| instead? No, readyState is "common" now (xhr, filereader, etc.) > > + attribute nsIDOMEventListener onreadystatechange; > > If I understood it correctly, this is called every time one of the > eventlistener is called. Do we really want that? > IMO, we could have a statechange event and have scripts checking what the > state is. Similar to XHR, we decided we want both. That way we support a single function usage as well as usage based on individual events. > > + case RIL.CALL_STATE_WAITING: > > + return nsITelephone.CALL_STATE_HELD; // XXX This may not be right... > > Please open a follow-up and put the bug number in a TODO comment instead of > adding an XXX comment. I believe philikon responded here already.

Complete patch.

(In reply to ben turner [:bent] from comment #45) > Created attachment 585939 [details] [diff] [review] > Interdiff > > Changes to attachment 584648 [details] [diff] [review]. > > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #41) > > - You have a lot of private ctor and you have a ::Create() method that > > allows the construction of the object. I don't really get why you are using > > such pattern; > > It's nice to ensure that no one ever gets a half-constructed object. Some of > the objects are not valid if they haven't had some sort of an Init method > called on them so rather than make people remember to call 'new Foo()' > followed by 'foo->Init()' I bundle both into Create(). It's very useful. There is only one |Create| method that is faillible in your patch. For the others, a ctor should be as good except that |Create| returns an |already_addRefed| object but I forgetting to addref would lead to crashes anyway. > > - Same thing for dtor; > > These are all refcounted objects so calling 'delete foo' should be > disallowed in all but the Release method. Calling |delete| will very likely lead to crashes so I wouldn't bother making the dtor private. > > - You might want to use MOZ_ASSERT() instead of NS_ASSERTION sometimes; > > That didn't exist when I first wrote this patch. Going forward we can change > if you feel strongly. I don't feel strongly. It was just for information. > > - I think you should get ride of TelephonyCommon.h; > > It includes the things that I almost always need. I'd rather have that then > duplicate it everywhere. I really dislike that. I'm not going to be stubborn and r- the patch for that though but it seems a bad practice to me. > > ::: dom/base/nsDOMClassInfo.cpp > > ... > > > + // Wish we didn't have to expose this, but nsITelephone is implemented in JS > > > + // so we have no choice... > > > + DOM_CLASSINFO_MAP_ENTRY(nsITelephoneCallback) > > > > Could you file a bug? and put the bug number in the comment. > > I don't see the point of putting the bug number in the comment. Will file > though. If someone see the comment, she/he can go to the bug to see how things are going or event fix it. > > |return rv;| maybe? unless you really want the warning to show up. > > I do. Any time something fails (with the exception of content code) I want > to know about it. I think this might still be nicer: if (NS_FAILED(rv)) { NS_WARNING("something"); } return rv; > > > + NS_ENSURE_ARG(!aNumber.IsEmpty()); > > > > I would prefer: > > NS_ENSURE_TRUE(!aNumber.IsEmpty(), NS_ERROR_INVALID_ARG); > > Until we remove these macros I don't see the problem using them (they're > used all over the place). I believe those methods are new and should be preferred. > > > + nsITelephone* > > > + Telephone() const > > > > Why not GetTelephone? > > Because it can never fail and return null. Does a method prefixed by |Get| means it can fail? If there is no |Get| does that mean the method can never returns null? AFAIK, a method can only fail if it is returning |nsresult| or has a out arg being |nsresult| also the only safe way to assume it will not return null is when it is returning an object or a reference to an object. > > Can't use use PR_UINT32_MAX? > > I could, but I don't think that is any nicer. It would be nicer. By using -1 you are just trying to use PR_UINT32_MAX without naming it. Also, using -1 might warn or fail in some configurations because you are setting a signed value to an unsigned int. > > What is that exactly? And why is that in the IDL if it's not implemented? > > It's part of the telephony API. We'll have to implement it all before we can > ship. I wonder what "DoNotDisturb" means for a phone. > > > + readonly attribute DOMString readyState; > > > > Shouldn't this be named |state| instead? > > No, readyState is "common" now (xhr, filereader, etc.) As far as I understand it, |readyState| is used when there is a notion of success or error or at least something being processed in the background. Here, it is the state of the call the is no real notion of processing anything. > > > + case RIL.CALL_STATE_WAITING: > > > + return nsITelephone.CALL_STATE_HELD; // XXX This may not be right... > > > > Please open a follow-up and put the bug number in a TODO comment instead of > > adding an XXX comment. > > I believe philikon responded here already. He didn't add the bug number in the comment ;)

Comment on attachment 585955 [details] [diff] [review] Patch, v3 Review of attachment 585955 [details] [diff] [review]: ----------------------------------------------------------------- I still believe there are a few nits but given that it might a matter of taste, I'm not going to hold this more. The real issues have been fixed.

Comment on attachment 585955 [details] [diff] [review] Patch, v3 Drop the donotdisturb stuff. As far as I understand it's not a network feature, and so we might as well implement it in JS in the dialer-app, rather than as a feature of the DOM interface. I'm surprised that we need to do so much work for all the event handlers. I would have imagined that they were owned by the EventListenerManager these days which would take care of initializing and storing them, and so we'd only need to implement the getters/setters. But if this is the state of the art then this I guess we'll have to do it.

Jonas, what's your opinion about renaming |liveCalls| to |currentCalls| and |readyState| to |state|?

I think I prefer readyState to state since that's consistent with a pile of other APIs. I don't really care about liveCalls vs. currentCalls other than that shorter is better. How about just "calls"?

(In reply to Jonas Sicking (:sicking) from comment #51) > I think I prefer readyState to state since that's consistent with a pile of > other APIs. Actually, that's the reason why I believe we should name this |state| instead of |readyState|. The state isn't reflecting any readiness and seems in no way similar to the other interfaces using |readyState|. For example, there is no success/error state, it's really about the call state and a call isn't ready/close to ready/not ready. Also, there is no |result| or error code. Playing the consistency card here seems wrong. > I don't really care about liveCalls vs. currentCalls other than that shorter > is better. How about just "calls"? I think currentCalls is more understandable for non-native speakers than liveCalls. For me the former is really clear but when I read the later, I thought "it should be that". I guess |calls| would be okay too. BTW, |active| should be renamed |activeCall| IMO.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #52) > Actually, that's the reason why I believe we should name this |state| > instead of |readyState|. The state isn't reflecting any readiness and seems > in no way similar to the other interfaces using |readyState|. Hmm.. good point. This state can bounce around in all sorts of manners, compared to readyState which tends to have a single path through a set of states. Ok, "state" is fine with me. > I think currentCalls is more understandable for non-native speakers than > liveCalls. For me the former is really clear but when I read the later, I > thought "it should be that". > I guess |calls| would be okay too. "calls" seems like the most direct name since we don't have other lists of calls (like unactiveCalls or previouslyCurrentCallsThatAreNoLongerCurrectCalls). > BTW, |active| should be renamed |activeCall| IMO. Works for me.

Had a build problem on Linux: dom/telephony/CallEvent.cpp:88:40: error: conversion from ‘nsRefPtr<mozilla::dom::telephony::TelephonyCall>’ to non-scalar type ‘nsCOMPtr<nsIDOMTelephonyCall>’ requested Attached patch fixes. (Note that this code is disabled for default Firefox builds, so it wasn't affecting Firefox Linux builds at all)

Pushed fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/517e687ce17e

https://hg.mozilla.org/mozilla-central/rev/bac673bc7211 https://hg.mozilla.org/mozilla-central/rev/517e687ce17e

Guys (et al) we have not completed a security review on this as requested, as such I don't think this bug should be resolved as we may have issue that need change. Some please correct me if I am wrong with the process here?

(In reply to Curtis Koenig [:curtisk] from comment #58) > Guys (et al) we have not completed a security review on this as requested, > as such I don't think this bug should be resolved as we may have issue that > need change. Some please correct me if I am wrong with the process here? RESOLVED/FIXED generally means the patch(es) has/have landed. This is the case here. We typically deal with revisions, regardless of their nature, in follow-up bugs.

Thanks philikon, I guess the next step is to get the review scheduled. available dates can be found on https://mail.mozilla.com/home/ckoenig@mozilla.com/Security%20Review.html

Curtis: It doesn't make sense to do the security review of this (and many other WebAPIs) until we have more of the security model in place. We're working hard on that, but I don't think we should stress about doing the security review until that is done.

We have lots of API docs for this now, and a very small sample. I'm calling this doc complete for the time being, until such time as things have stabilized on the B2G side enough for us to do something more involved. See: https://developer.mozilla.org/en/DOM/window.navigator.mozTelephony https://developer.mozilla.org/en/API/WebTelephony https://developer.mozilla.org/en/DOM/Telephony https://developer.mozilla.org/en/DOM/TelephonyCall https://developer.mozilla.org/en/DOM/CallEvent

Thanks, sheppy!

Review Scheduled: https://mail.mozilla.com/home/ckoenig@mozilla.com/Security%20Review.html?view=month&action=view&invId=130654-130653&pstat=AC&exInvId=130654-181939&useInstance=1&instStartTime=1338840000000&instDuration=3600000

Dietrich, why are you removing all those follow-up bugs?

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #65) > Dietrich, why are you removing all those follow-up bugs? We removed all non-blocking bugs from the dependency tree, so our vision wrt blockers wasn't too clouded. Doesn't mean these bugs can't happen, but they're not a priority and don't block v1 of the phone.

(In reply to Philipp von Weitershausen [:philikon] from comment #66) > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #65) > > Dietrich, why are you removing all those follow-up bugs? > > We removed all non-blocking bugs from the dependency tree, so our vision wrt > blockers wasn't too clouded. Doesn't mean these bugs can't happen, but > they're not a priority and don't block v1 of the phone. Forgot to comment on that: dependencies/follow ups/whatever has nothing to do with blocking. We have blocker flags. Bug 716856 isn't blocking but is clearly a follow-up from that bug.