Last Comment Bug 743008 - WebTelephony: support multiprocess
: WebTelephony: support multiprocess
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla15
Assigned To: Hsin-Yi Tsai [:hsinyi]
:
Mentors:
Depends on:
Blocks: e10s-device-apis webtelephony
  Show dependency treegraph
 
Reported: 2012-04-05 15:10 PDT by Philipp von Weitershausen [:philikon]
Modified: 2012-04-29 20:27 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposal: WebTelephony support multiprocess (15.48 KB, patch)
2012-04-12 01:00 PDT, Hsin-Yi Tsai [:hsinyi]
philipp: feedback+
Details | Diff | Review
Part 1: IDL and DOM impl (6.82 KB, patch)
2012-04-17 02:07 PDT, Hsin-Yi Tsai [:hsinyi]
no flags Details | Diff | Review
Part 2: JS impl (14.23 KB, patch)
2012-04-17 02:08 PDT, Hsin-Yi Tsai [:hsinyi]
philipp: review-
Details | Diff | Review
Part 1 (v2): IDL and DOM impl (6.43 KB, patch)
2012-04-18 04:40 PDT, Hsin-Yi Tsai [:hsinyi]
philipp: review+
Details | Diff | Review
Part 2 (v2): JS impl (12.97 KB, patch)
2012-04-18 04:42 PDT, Hsin-Yi Tsai [:hsinyi]
philipp: review+
Details | Diff | Review
Patch (v3) (24.94 KB, patch)
2012-04-24 07:30 PDT, Hsin-Yi Tsai [:hsinyi]
no flags Details | Diff | Review

Description Philipp von Weitershausen [:philikon] 2012-04-05 15:10:44 PDT

    
Comment 1 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-04-05 15:37:22 PDT
Hm... Why do we need multiprocess for phone stuff? I was under the impression that this would only live in the main process.
Comment 2 Philipp von Weitershausen [:philikon] 2012-04-05 15:43:00 PDT
(In reply to ben turner [:bent] from comment #1)
> Hm... Why do we need multiprocess for phone stuff? I was under the
> impression that this would only live in the main process.

Is the dialer app going to run in the content process? What about the homescreen (which listens for incoming call events)?

It doesn't seem so clear cut to me, so I just filed this as a placeholder. If it turns out to be INVALID or WONTFIX, so be it.
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-05 17:54:59 PDT
All web-app code will run in content processes.  We need to fix this.
Comment 4 Philipp von Weitershausen [:philikon] 2012-04-09 02:16:27 PDT
Hsinyi is going to take a stab at this. I suggest we make use of the RILContentHelper that I already introduced in bug 729173.
Comment 5 Hsin-Yi Tsai [:hsinyi] 2012-04-12 01:00:25 PDT
Created attachment 614294 [details] [diff] [review]
Proposal: WebTelephony support multiprocess

Thanks for Philipp's suggestion! I made this patch based on the RILContentHelper. 
The major concept of this patch is that telephony-related interfaces are moved from nsIRadioInterfaceLayer to nsIRILContentHelper. Then, use |cpmm| and |ppmm| introduced in Bug 744319 to communicate between chrome process and content process. 

Please note that the patch is NOT completed yet. It's just my current thoughts about how I am going to fix this issue. It would be great help that I obtain some feedback before diving into implementation. Thanks :)
Comment 6 Philipp von Weitershausen [:philikon] 2012-04-12 01:50:33 PDT
Comment on attachment 614294 [details] [diff] [review]
Proposal: WebTelephony support multiprocess

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

Great start!

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +133,5 @@
>  
>    nsIDOMDOMRequest getNetworks(in nsIDOMWindow window);
> +  
> +  void registerCallback(in nsIRILTelephonyCallback callback);
> +  void unregisterCallback(in nsIRILTelephonyCallback callback);

While we're at it, I would like to rename these two methods to [un]registerTelephonyCallback.

Also, don't forget to update the UUIDs for nsIRadioInterfaceLayer and nsIRILContentHelper!

::: dom/system/gonk/RILContentHelper.js
@@ +93,5 @@
>      throw Cr.NS_ERROR_NOT_IMPLEMENTED;
>    },
> +  
> +   _callbacks: null,
> +   _enumerationCallbacks: null,

Don't forget to initialize those to empty objects in the constructor.

@@ +101,5 @@
> +  },
> +   
> +  unregisterCallback: function unregisterCallback(callback) {
> +    //TODO  
> +  },

You can just move those two methods straight from RadioInterfaceLayer.js.

@@ +110,5 @@
> +  },
> +  
> +  dial(): function dial(number) {
> +    //TODO
> +    Service.cpmm.sendAsyncMessage("RIL:Dial", number);

Note that we decided not to define Services.{cpmm|ppmm} in bug 734018, so you'll just have to use the 'cpmm' reference that I defined in my updated patches for bug 729173.

@@ +148,5 @@
> +  },
> +  
> +  set microphoneMuted(value) {
> +    //TODO
> +    Service.cpmm.sendAsyncMessage("RIL:SetMicrophoneMuted", value);  

Why send messages for these? I think we can just manipulate the audio state from the content process here.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +177,5 @@
>    Services.ppmm.addMessageListener("RIL:GetRadioState", this);
> +  
> +  for each (let msgname in RIL_IPC_MSG_NAMES) {
> +    Services.cpmm.addMessageListener(msgname, this);
> +  }

You can get rid of the line that registers the "RIL:GetRadioState" listener.

::: dom/telephony/Telephony.cpp
@@ +518,4 @@
>    nsIInterfaceRequestor* ireq = SystemWorkerManager::GetInterfaceRequestor();
>    NS_ENSURE_TRUE(ireq, NS_ERROR_UNEXPECTED);
>  
> +  nsCOMPtr<nsIRILContentHelper> ril = do_GetInterface(ireq);

The nsIRadioInterfaceLayer singleton was looked up from the SystemWorkerManager. This is not the case with nsIRILContentHelper. Have a look at bug 729173 part 4 for an example. In essence, all you have to do is:

  nsCOMPtr<nsIRILContentHelper> ril = do_GetService(NS_RILCONTENTHELPER_CONTRACTID);
Comment 7 Hsin-Yi Tsai [:hsinyi] 2012-04-12 03:35:43 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #6)
> Comment on attachment 614294 [details] [diff] [review]
> Proposal: WebTelephony support multiprocess
> 
> Review of attachment 614294 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great start!
> 
> ::: dom/system/gonk/nsIRadioInterfaceLayer.idl
> @@ +133,5 @@
> >  
> >    nsIDOMDOMRequest getNetworks(in nsIDOMWindow window);
> > +  
> > +  void registerCallback(in nsIRILTelephonyCallback callback);
> > +  void unregisterCallback(in nsIRILTelephonyCallback callback);
> 
> While we're at it, I would like to rename these two methods to
> [un]registerTelephonyCallback.
> 

Sounds good. Will do that in the final patch.

> Also, don't forget to update the UUIDs for nsIRadioInterfaceLayer and
> nsIRILContentHelper!
> 
yap, I remember that. Thanks for reminding!
> 
> @@ +110,5 @@
> > +  },
> > +  
> > +  dial(): function dial(number) {
> > +    //TODO
> > +    Service.cpmm.sendAsyncMessage("RIL:Dial", number);
> 
> Note that we decided not to define Services.{cpmm|ppmm} in bug 734018, so
> you'll just have to use the 'cpmm' reference that I defined in my updated
> patches for bug 729173.
> 

OK, would apply latest patches from 729173.

> @@ +148,5 @@
> > +  },
> > +  
> > +  set microphoneMuted(value) {
> > +    //TODO
> > +    Service.cpmm.sendAsyncMessage("RIL:SetMicrophoneMuted", value);  
> 
> Why send messages for these? I think we can just manipulate the audio state
> from the content process here.
> 

I thought AudioManager is in the chrome process, isn't it? 
Besides, the audio state changes with the call state, which is handled in the chrome process. So, I think it might be good doing the audio-related stuff in the chrome process. 

> ::: dom/telephony/Telephony.cpp
> @@ +518,4 @@
> >    nsIInterfaceRequestor* ireq = SystemWorkerManager::GetInterfaceRequestor();
> >    NS_ENSURE_TRUE(ireq, NS_ERROR_UNEXPECTED);
> >  
> > +  nsCOMPtr<nsIRILContentHelper> ril = do_GetInterface(ireq);
> 
> The nsIRadioInterfaceLayer singleton was looked up from the
> SystemWorkerManager. This is not the case with nsIRILContentHelper. Have a
> look at bug 729173 part 4 for an example. In essence, all you have to do is:
> 
>   nsCOMPtr<nsIRILContentHelper> ril =
> do_GetService(NS_RILCONTENTHELPER_CONTRACTID);

Got it. Thanks for pointing this out!
Comment 8 Hsin-Yi Tsai [:hsinyi] 2012-04-17 02:07:22 PDT
Created attachment 615632 [details] [diff] [review]
Part 1: IDL and DOM impl

Thanks for the comments above. This patch considers them all.

Please also note the patch is based on Philipp's patches introduced in Bug 729173. Please apply those first for building correctly. Thanks.
Comment 9 Hsin-Yi Tsai [:hsinyi] 2012-04-17 02:08:10 PDT
Created attachment 615633 [details] [diff] [review]
Part 2: JS impl
Comment 10 Philipp von Weitershausen [:philikon] 2012-04-17 21:05:25 PDT
Comment on attachment 615632 [details] [diff] [review]
Part 1: IDL and DOM impl

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

Going to clear review on this since it looks like this code doesn't compile.

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +47,1 @@
>  interface nsIRILTelephonyCallback : nsISupports

We're not really changing the semantics of nsIRILTelephonyCallback, so I don't think we need to change this UUID.

::: dom/telephony/Telephony.cpp
@@ +573,5 @@
>    }
>  
> +  nsCOMPtr<nsIRILContentHelper> ril =
> +    do_GetService(NS_RILCONTENTHELPER_CONTRACTID);
> +  NS_ASSERTION(mRIL, "Could not acquire nsIRILContentHelper!");

Typo: mRIL. Did you even compile this?

Anyway, I think we should retain the existing behaviour with respect to failures and continue to do

  NS_ENSURE_TRUE(ril, NS_ERROR_UNEXPECTED);
Comment 11 Philipp von Weitershausen [:philikon] 2012-04-17 21:20:48 PDT
Comment on attachment 615633 [details] [diff] [review]
Part 2: JS impl

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

Not sure I like all the synchronous messages, but I can't think of a better way right now. We can always refactor if it becomes a problem.

r- because there's an unrelated piece in this patch that I'd like to discuss separately. Other than that just a few nits. Good work!

::: dom/system/gonk/RILContentHelper.js
@@ +223,5 @@
>          }
>          Services.obs.notifyObservers(null, kDataChangedTopic, null);
>          break;
> +      case "RIL:EnumerateCalls":
> +        this.handleEnumerateCalls(msg.json.calls, msg.json.activeCallIndex);

Just pass `msg.json` to `handleEnumerateCalls`.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +191,1 @@
>    Services.obs.addObserver(this, "xpcom-shutdown", false);

You also need to modify the `observe` method where the message listeners are unregistered.

@@ +472,4 @@
>        // This is now the active call.
>        this._activeCall = call;
> +    } else if (call.state == nsIRadioInterfaceLayer.CALL_STATE_HELD) {
> +      this._activeCall = null;

This is a bit unrelated to this patch. I would like to discuss this a bit further since it has implications on navigator.mozTelephony.active and the audio state. Can you please file a separate bug and attach a patch there? Thanks!

@@ +676,4 @@
>      this.worker.postMessage({type: "resumeCall", callIndex: callIndex});
>    },
>  
> +  setMicrophoneMuted: function setMicrophoneMuted(value) {

I would prefer to keep those getters and setters.

@@ +686,4 @@
>      gAudioManager.microphoneMuted = value;
>    },
>  
> +  setSpeakerEnabled: function setSpeakerEnabled(value) {

Ditto.
Comment 12 Hsin-Yi Tsai [:hsinyi] 2012-04-17 21:25:38 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #10)
> Comment on attachment 615632 [details] [diff] [review]
> Part 1: IDL and DOM impl
> 
> Review of attachment 615632 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Going to clear review on this since it looks like this code doesn't compile.
> 
> ::: dom/system/gonk/nsIRadioInterfaceLayer.idl
> @@ +47,1 @@
> >  interface nsIRILTelephonyCallback : nsISupports
> 
> We're not really changing the semantics of nsIRILTelephonyCallback, so I
> don't think we need to change this UUID.
> 
> ::: dom/telephony/Telephony.cpp
> @@ +573,5 @@
> >    }
> >  
> > +  nsCOMPtr<nsIRILContentHelper> ril =
> > +    do_GetService(NS_RILCONTENTHELPER_CONTRACTID);
> > +  NS_ASSERTION(mRIL, "Could not acquire nsIRILContentHelper!");
> 
> Typo: mRIL. Did you even compile this?
> 
> Anyway, I think we should retain the existing behaviour with respect to
> failures and continue to do
> 
>   NS_ENSURE_TRUE(ril, NS_ERROR_UNEXPECTED);

Oops...my fault! Even this was compiled successfully (because mRIL had been defined before), and I have tested all the telephony functions, which all worked fine. Going to submit a revised version later. Thanks.

 

(In reply to Philipp von Weitershausen [:philikon] from comment #10)
> Comment on attachment 615632 [details] [diff] [review]
> Part 1: IDL and DOM impl
> 
> Review of attachment 615632 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Going to clear review on this since it looks like this code doesn't compile.
> 
> ::: dom/system/gonk/nsIRadioInterfaceLayer.idl
> @@ +47,1 @@
> >  interface nsIRILTelephonyCallback : nsISupports
> 
> We're not really changing the semantics of nsIRILTelephonyCallback, so I
> don't think we need to change this UUID.
> 
> ::: dom/telephony/Telephony.cpp
> @@ +573,5 @@
> >    }
> >  
> > +  nsCOMPtr<nsIRILContentHelper> ril =
> > +    do_GetService(NS_RILCONTENTHELPER_CONTRACTID);
> > +  NS_ASSERTION(mRIL, "Could not acquire nsIRILContentHelper!");
> 
> Typo: mRIL. Did you even compile this?
> 
> Anyway, I think we should retain the existing behaviour with respect to
> failures and continue to do
> 
>   NS_ENSURE_TRUE(ril, NS_ERROR_UNEXPECTED);

Oops...my fault! Even this was compiled successfully (because mRIL had been defined before), and I have tested all the telephony functions, which all worked fine. Going to submit a revised version later. Thanks.
Comment 13 Hsin-Yi Tsai [:hsinyi] 2012-04-18 04:40:53 PDT
Created attachment 616075 [details] [diff] [review]
Part 1 (v2): IDL and DOM impl

Thanks for the comments. They are all addressed in the patches.
Comment 14 Hsin-Yi Tsai [:hsinyi] 2012-04-18 04:42:54 PDT
Created attachment 616077 [details] [diff] [review]
Part 2 (v2): JS impl
Comment 15 Hsin-Yi Tsai [:hsinyi] 2012-04-18 04:49:20 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #11)
> 
> @@ +472,4 @@
> >        // This is now the active call.
> >        this._activeCall = call;
> > +    } else if (call.state == nsIRadioInterfaceLayer.CALL_STATE_HELD) {
> > +      this._activeCall = null;
> 
> This is a bit unrelated to this patch. I would like to discuss this a bit
> further since it has implications on navigator.mozTelephony.active and the
> audio state. Can you please file a separate bug and attach a patch there?
> Thanks!
> 
I've filed a bug for this issue. Please see Bug 746496 for further discussion. Thanks.
Comment 16 Philipp von Weitershausen [:philikon] 2012-04-22 17:16:24 PDT
Comment on attachment 616075 [details] [diff] [review]
Part 1 (v2): IDL and DOM impl

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

Great, thanks! Please rebase this patch on the latest m-c. A few things have changed in the final version of bug 729173. Thanks! (No need to ask for review again.)
Comment 17 Philipp von Weitershausen [:philikon] 2012-04-22 17:28:13 PDT
Comment on attachment 616077 [details] [diff] [review]
Part 2 (v2): JS impl

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

Nice work! r=me

Since neither of the two patches aren't useful without the other one, I suggest merging them into one patch when you rebase them on top of m-c.
Comment 18 Hsin-Yi Tsai [:hsinyi] 2012-04-24 07:30:08 PDT
Created attachment 617860 [details] [diff] [review]
Patch (v3)

Updated after rebase. Thanks!
Comment 19 Philipp von Weitershausen [:philikon] 2012-04-24 08:53:23 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7bb8bb7d7a0
Comment 20 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-25 07:11:08 PDT
https://hg.mozilla.org/mozilla-central/rev/c7bb8bb7d7a0

Note You need to log in before you can comment on or make changes to this bug.