864485 - B2G RIL: use ipdl as IPC in MozTelephony

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

Attachment: WIP

Based on bug 888592, WebTelephony WebIDL port.

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/4: Rename nsITelephonyProvider to nsITelephonyService. WIP2

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/4: Interface changes

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/4: DOM & IPC. WIP2

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4/4: Gonk backend specific. WIP2

Works on B2G only.  Probably brings in build breaks on other platforms because IPDL parts are necessary in spite of targets.

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/4: Rename nsITelephonyProvider to nsITelephonyService.

I named the interface nsITelephonyProvider because we thought we might have several instances of telephony providers for multisim arch in bug 834160.  Things turns out we have only one and each call to different underlying instances are identified by a session id.  The name "provider" no longer represents it very well, so here is the patch.

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 791669 [details] [diff] [review]
part 2/4: Interface changes

1. Add/modify IPDL
2. Don't return a boolean in nsITelephonyListener::enumerateCallState as a signal to continue the enumeration because we always return true.
3. Rename nsITelephonyService::{un,}registerTelephonyMsg to {un,}registerListener
4. Share nsITelephonyService::dial for both emergency and non-emergency calls.

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/4: DOM & IPC

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4/4: Gonk backend specific

Move telephony related parts out of RadioInterfaceLayer.js into gonk/TelephonyService.js.

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

From bug 859616 comment 13, now telephony parts are also built but disabled on platforms other than Gonk.

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

https://tbpl.mozilla.org/?tree=Try&rev=2a8e4c9ad2fd

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #11)
> https://tbpl.mozilla.org/?tree=Try&rev=2a8e4c9ad2fd

[-Werror,-Wdelete-non-virtual-dtor]
[-Werror,-Wsign-compare]

Comment 13

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 791669 [details] [diff] [review]
part 2/4: Interface changes

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

::: dom/telephony/nsITelephonyService.idl
@@ +53,5 @@
>     *        Indicates whether this call is outgoing or incoming.
>     *
>     * @return true to continue enumeration or false to cancel.
>     */
> +  void enumerateCallState(in unsigned long callIndex,

boolean provides a way to cancel. I understood that now Telephony and Bluetooth code always returns true, but I think it's nice to keep the flexible mechanism. sr- for this. Thank you.

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #13)
> Review of attachment 791669 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/telephony/nsITelephonyService.idl
> @@ +53,5 @@
> > +  void enumerateCallState(in unsigned long callIndex,
> 
> boolean provides a way to cancel. I understood that now Telephony and
> Bluetooth code always returns true, but I think it's nice to keep the
> flexible mechanism. sr- for this. Thank you.

Two things in concerned.  First, no one has used this cancel mechanism since telephony was first landed in bug 674726.  So basically I think time has proved that we don't need such convenience.

Second, with that return value, we have to re-type the corresponding IPDL call to a 'sync' one, which follows the chrome process has to wait for the return value before it can proceed to serve the next entry or other requests.  We're blocking chrome process for a feature that nobody uses.

If we still want to reserve it for future use, maybe at least file a bug, point out under what circumstances will we need such feature, and we're going to resolve it some day for sure.

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

If we enable return value of nsITelephonyListener::enumerateCallStat, and have corresponding change in IPDL protocol, we get following errors and fail to build gecko:

> dom/telephony/ipc/PTelephonyRequest.ipdl:28: error: sync parent-to-child messages
> are verboten (here, message `NotifyEnumerateCallState' in protocol `PTelephonyRequest')
> dom/telephony/ipc/PTelephonyRequest.ipdl:28: error: message `NotifyEnumerateCallState'
> requires more powerful send semantics than its protocol `PTelephonyRequest' provides
> Specification is not well typed.

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/4: DOM & IPC : v2

Address comment 12 and fix several code alignments.

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/4: DOM & IPC : v3

using mozilla::ErrorResult;

https://tbpl.mozilla.org/?tree=Try&rev=cdbc09103f0c

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/4: Rename nsITelephonyProvider to nsITelephonyService : v2

rebase after bug 772765 -- conference call

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/4: Interface changes : v2

rebase after bug 772765 -- conference call

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/4: DOM & IPC : v4

rebase after bug 772765 -- conference call

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4/4: Gonk backend specific : v2

rebase after bug 772765 -- conference call

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 794612 [details] [diff] [review]
part 2/4: Interface changes : v2

I'm going to defer to khuey here. Thanks!

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I would like to know if Hsin-Yi is satisfied with comment 14 before I spend time reviewing this.

Comment 24

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 794612 [details] [diff] [review]
part 2/4: Interface changes : v2

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

sr = me for the .idl change. Thank you.

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 794612 [details] [diff] [review]
part 2/4: Interface changes : v2

I know it's a drag and I hate to be a stickler but sr+ can only be granted by one of the folks on this list: http://www.mozilla.org/hacking/reviewers.html (I would recommend mrbkap or sicking, in this case, but anyone is fine).

Comment 26

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #25)
> Comment on attachment 794612 [details] [diff] [review]
> part 2/4: Interface changes : v2
> 
> I know it's a drag and I hate to be a stickler but sr+ can only be granted
> by one of the folks on this list:
> http://www.mozilla.org/hacking/reviewers.html (I would recommend mrbkap or
> sicking, in this case, but anyone is fine).

Thanks for the reminding, Ben. I was thinking this is an internal API and I should be able to review this. But yes, not supposed to do sr.

Comment 27

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 794612 [details] [diff] [review]
part 2/4: Interface changes : v2

.idl change looks good to me. Asking for Jonas' super review. Thank you!

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #25)
> Comment on attachment 794612 [details] [diff] [review]
> part 2/4: Interface changes : v2
> 
> I know it's a drag and I hate to be a stickler but sr+ can only be granted
> by one of the folks on this list:
> http://www.mozilla.org/hacking/reviewers.html (I would recommend mrbkap or
> sicking, in this case, but anyone is fine).

Just for that I'm going to renew my campaign to have you added to the sr list.

Comment 29

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #24)
> Comment on attachment 794612 [details] [diff] [review]
> part 2/4: Interface changes : v2
> 
> Review of attachment 794612 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> sr = me for the .idl change. Thank you.

Thanks Hsin-Yi.

Vicamo, I will review these patches tomorrow.

Comment 30

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 794612 [details] [diff] [review]
part 2/4: Interface changes : v2

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

::: dom/telephony/nsITelephonyService.idl
@@ +66,5 @@
>     * @param isOutgoing
>     *        Indicates whether this call is outgoing or incoming.
>     * @param isConference
>     *        Indicates whether this call is a conference call.
>     * @return true to continue enumeration or false to cancel.

Please remove the comment as well.

Comment 31

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 794613 [details] [diff] [review]
part 3/4: DOM & IPC : v4

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

::: dom/ipc/ContentParent.cpp
@@ +2178,5 @@
> +        return nullptr;
> +    }
> +
> +    TelephonyParent* actor = new TelephonyParent();
> +    actor->AddRef();

Just write NS_ADDREF(actor);

@@ +2185,5 @@
> +
> +bool
> +ContentParent::DeallocPTelephonyParent(PTelephonyParent* aActor)
> +{
> +    static_cast<TelephonyParent*>(aActor)->Release();

And NS_RELEASE(aActor);

::: dom/telephony/Telephony.cpp
@@ +27,3 @@
>  USING_TELEPHONY_NAMESPACE
>  using namespace mozilla::dom;
> +using mozilla::ErrorResult;

Why is this needed?

::: dom/telephony/TelephonyFactory.cpp
@@ +16,5 @@
> +  nsCOMPtr<nsITelephonyService> service;
> +
> +  if (XRE_GetProcessType() == GeckoProcessType_Content) {
> +    service = new TelephonyIPCService();
> +  }

Why doesn't this return anything useful in the parent?

::: dom/telephony/ipc/TelephonyIPCService.cpp
@@ +33,5 @@
> +
> +NS_IMETHODIMP
> +TelephonyIPCService::RegisterListener(nsITelephonyListener *aListener)
> +{
> +  NS_ENSURE_STATE(!mListeners.Contains(aListener));

I think this should be an assertion.

MOZ_ASSERT(!mListeners.Contains(aListener));

of course, you should test in a --enable-debug build to make sure it doesn't fire.

@@ +34,5 @@
> +NS_IMETHODIMP
> +TelephonyIPCService::RegisterListener(nsITelephonyListener *aListener)
> +{
> +  NS_ENSURE_STATE(!mListeners.Contains(aListener));
> +  NS_ENSURE_TRUE(mListeners.AppendElement(aListener), NS_ERROR_OUT_OF_MEMORY);

nsTArray uses infallible allocation by default, so this cannot fail.

@@ +45,5 @@
> +
> +NS_IMETHODIMP
> +TelephonyIPCService::UnregisterListener(nsITelephonyListener *aListener)
> +{
> +  NS_ENSURE_STATE(mListeners.Contains(aListener));

And this should be an assertion too.

@@ +46,5 @@
> +NS_IMETHODIMP
> +TelephonyIPCService::UnregisterListener(nsITelephonyListener *aListener)
> +{
> +  NS_ENSURE_STATE(mListeners.Contains(aListener));
> +  NS_ENSURE_TRUE(mListeners.RemoveElement(aListener), NS_ERROR_FAILURE);

And if the listener is in the array we cannot fail to remove it.

@@ +67,5 @@
> +}
> +
> +NS_IMETHODIMP
> +TelephonyIPCService::Dial(const nsAString& aNumber,
> +                          bool             aIsEmergency)

Nit: Don't line up the variable names.  Just put one space between the type and aFoo.

@@ +189,5 @@
> +                                      bool aIsOutgoing,
> +                                      bool aIsEmergency,
> +                                      bool aIsConference)
> +{
> +  for (uint32_t ii = 0; ii < mListeners.Length(); ii++) {

Just use 'i'?

@@ +200,5 @@
> +
> +NS_IMETHODIMP
> +TelephonyIPCService::ConferenceCallStateChanged(uint16_t aCallState)
> +{
> +  for (uint32_t ii = 0; ii < mListeners.Length(); ii++) {

Here too.

@@ +228,5 @@
> +NS_IMETHODIMP
> +TelephonyIPCService::NotifyCdmaCallWaiting(const nsAString& aNumber)
> +{
> +  for (uint32_t ii = 0; ii < mListeners.Length(); ii++) {
> +    mListeners[ii]->NotifyCdmaCallWaiting(aNumber);

And here.

@@ +238,5 @@
> +TelephonyIPCService::NotifyError(int32_t aCallIndex,
> +                                 const nsAString& aError)
> +{
> +  for (uint32_t ii = 0; ii < mListeners.Length(); ii++) {
> +    mListeners[ii]->NotifyError(aCallIndex, aError);

And here.

@@ +248,5 @@
> +TelephonyIPCService::SupplementaryServiceNotification(int32_t aCallIndex,
> +                                                      uint16_t aNotification)
> +{
> +  for (uint32_t ii = 0; ii < mListeners.Length(); ii++) {
> +    mListeners[ii]->SupplementaryServiceNotification(aCallIndex, aNotification);

And here.

::: dom/telephony/ipc/TelephonyIPCService.h
@@ +27,5 @@
> +protected:
> +  virtual ~TelephonyIPCService();
> +
> +private:
> +  nsTArray< nsCOMPtr<nsITelephonyListener> > mListeners;

super-nit: no space between nsTArray< and nsCOMPtr.

::: dom/telephony/ipc/TelephonyParent.cpp
@@ +16,5 @@
> +TelephonyParent::TelephonyParent()
> +  : mActorDestroyed(false)
> +  , mRegistered(false)
> +{
> +  // Do nothing.

No need for this comment.

@@ +53,5 @@
> +{
> +  TelephonyRequestParent* actor = new TelephonyRequestParent();
> +  // Add an extra ref for IPDL. Will be released in
> +  // TelephonyParent::DeallocPTelephonyRequestParent().
> +  actor->AddRef();

NS_ADDREF(actor);

@@ +79,5 @@
> +  NS_ENSURE_TRUE(!mRegistered, true);
> +
> +  nsCOMPtr<nsITelephonyService> service =
> +    do_GetService(TELEPHONY_SERVICE_CONTRACTID);
> +  NS_ENSURE_TRUE(service, true);

Why do we always return true if we can't get the service?  Why don't we return false?

::: dom/telephony/moz.build
@@ +11,5 @@
>  XPIDL_MODULE = 'dom_telephony'
>  
>  MODULE = 'dom'
>  
> +EXPORTS.mozilla.dom.telephony += [

Can we just stick these in mozilla/dom?  Seems unnecessary to have the extra namespacing, especially since these all start with "Call" or "Telephony" anyways.

Comment 32

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 794612 [details] [diff] [review]
part 2/4: Interface changes : v2

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

::: dom/telephony/ipc/PTelephony.ipdl
@@ +22,5 @@
> +};
> +
> +sync protocol PTelephony {
> +    manager PContent;
> +    manages PTelephonyRequest;

Two space indentation.

::: dom/telephony/ipc/PTelephonyRequest.ipdl
@@ +17,5 @@
> +
> +union IPCTelephonyReply
> +{
> +  IPCReplyEnumerateCallsComplete;
> +};

Are you planning to add more things here?  If you're not going to have multiple reply structs this union is kind of pointless.

Comment 33

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #31)
> Comment on attachment 794613 [details] [diff] [review]
> part 3/4: DOM & IPC : v4
> @@ +2185,5 @@
> > +
> > +bool
> > +ContentParent::DeallocPTelephonyParent(PTelephonyParent* aActor)
> > +{
> > +    static_cast<TelephonyParent*>(aActor)->Release();
> 
> And NS_RELEASE(aActor);

Actually that won't work because of the static_cast so just ignore this.

Comment 34

[Josh Matthews [:jdm]]

>@@ +79,5 @@
>> +  NS_ENSURE_TRUE(!mRegistered, true);
>> +
>> +  nsCOMPtr<nsITelephonyService> service =
>> +    do_GetService(TELEPHONY_SERVICE_CONTRACTID);
>> +  NS_ENSURE_TRUE(service, true);
>
>Why do we always return true if we can't get the service?  Why don't we return false?

Traditionally we avoid killing the content process for non-critical errors like this. Returning false is a big hammer.

Comment 35

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #31)
> Review of attachment 794613 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/telephony/moz.build
> @@ +11,5 @@
> >  XPIDL_MODULE = 'dom_telephony'
> >  
> >  MODULE = 'dom'
> >  
> > +EXPORTS.mozilla.dom.telephony += [
> 
> Can we just stick these in mozilla/dom?  Seems unnecessary to have the extra
> namespacing, especially since these all start with "Call" or "Telephony"
> anyways.

Will do in a follow-up bug 910568.

We have now B2G build errors due to FMRadio merge.  Will have an update to these patches later.

Comment 36

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/4: Rename nsITelephonyProvider to nsITelephonyService : v3

Rebase only

Comment 37

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/4: Interface changes : v3

Address comment 32 -- fix indentation and remove union IPCTelephonyRequest and struct IPCRequestEnumerateCalls completely.

Comment 38

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/4: DOM & IPC : v5

1) address various nits in comment 31
2) accommodate to removal of union IPCTelephonyRequest and struct IPCRequestEnumerateCalls.
3) rebase

Comment 39

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4/4: Gonk backend specific : v3

Rebase only.

Comment 40

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 797644 [details] [diff] [review]
part 1/4: Rename nsITelephonyProvider to nsITelephonyService : v3

I am concerned there might be changes to multisim WebAPI, so that we will create various instances of telephony providers. I am suggesting keeping the current name and changing it later in multisim telephony WebAPI patches if needed. Thank you.

Comment 41

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 797648 [details] [diff] [review]
part 4/4: Gonk backend specific : v3

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

I am sorry about comment 40.

Comment 42

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/3: Interface changes : v4

Don't rename nsITelephonyProvider for now.  Address comment 40.

Comment 43

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/3: DOM & IPC : v6

Stick with nsITelephonyProvider.

Comment 44

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/3: Gonk backend specific : v4

1) Address comment 40.
2) Add TelephonyProvider._notifyAllListeners to save common loops and improve error handling.

Comment 45

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 798447 [details] [diff] [review]
part 1/3: Interface changes : v4

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

r=me for the idl change.

Comment 46

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 798453 [details] [diff] [review]
part 3/3: Gonk backend specific : v4

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

Sorry for the delay. The changes look great. Thank you, thank you.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +903,3 @@
>          break;
>        case "callStateChange":
>          // This one will handle its own notifications.

This comment exists since the code landed at the first day. I feel the comment isn't proper anymore as every case right now handles its own notification. Would you please help remove the comment here and follows? Thanks.

::: dom/telephony/nsIGonkTelephonyProvider.idl
@@ +10,5 @@
> +        "@mozilla.org/telephony/gonktelephonyprovider;1"
> +%}
> +
> +[scriptable, uuid(0d106c7e-ba47-48ee-ba48-c92002d401b6)]
> +interface nsIGonkTelephonyProvider : nsITelephonyProvider

Let's keep the name here and see if we need to change it based on our multisim architecture. Thank you.

Comment 47

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #45)
> Comment on attachment 798447 [details] [diff] [review]
> part 1/3: Interface changes : v4
> 
> Review of attachment 798447 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me for the idl change.

Please remember to remove my name from sr ;-)

Comment 48

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/3: Interface changes : v5

1) Remove hsinyi from sr
2) Rebase after bug 888593 and bug 870676 (currently in b2g-inbound).

Comment 49

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/3: DOM & IPC : v7

Rebase after bug 888593 and bug 870676 (currently in b2g-inbound).

Comment 50

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/3: Gonk backend specific : v5

1) Fix |gPowerManagerService| undefined.
2) Rebase after bug 888593 and bug 870676 (currently in b2g-inbound).

Comment 51

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/db85ebf7b0dc
https://hg.mozilla.org/integration/b2g-inbound/rev/daaabb81c8f7
https://hg.mozilla.org/integration/b2g-inbound/rev/ed63ede44a82

Comment 52

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/b2g-inbound/rev/d13335b32659 for what (at least so far) looks to be debug-only build bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=27352159&tree=B2g-Inbound, https://tbpl.mozilla.org/php/getParsedLog.php?id=27352142&tree=B2g-Inbound, https://tbpl.mozilla.org/php/getParsedLog.php?id=27352795&tree=B2g-Inbound

Comment 53

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Phil Ringnalda (:philor) from comment #52)
> Backed out in
> https://hg.mozilla.org/integration/b2g-inbound/rev/d13335b32659 for what (at
> least so far) looks to be debug-only build bustage

Thank you.  I moved one header file inclusion into cpp file and causes that bustage.

Comment 54

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/3: DOM & IPC : v8

fix debug build bustage.

Comment 55

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9a72765987b8
https://hg.mozilla.org/integration/b2g-inbound/rev/9bd4fcd6d084
https://hg.mozilla.org/integration/b2g-inbound/rev/89113a5b091e

Comment 56

[Ed Morley [:emorley]]

Backed out for mochitest-3 failures:
https://tbpl.mozilla.org/?tree=B2g-Inbound&jobname=mochitest-3&rev=8b6235d4c2ac

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/6eca1326ff3f
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/833118c167e7
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/603699058e1b

Comment 57

[Vicamo Yang [:vicamo][:vyang]]

full try: https://tbpl.mozilla.org/?tree=Try&rev=ec0fd9a0a17e

Comment 58

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/3: DOM & IPC : v9

fix mochitest-3 bustage.

Comment 59

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/3: Gonk backend specific : v6

Rebase after bug 903403 (currently in b2g-inbound)

Comment 60

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/a638e7261ac3
https://hg.mozilla.org/integration/b2g-inbound/rev/5f709b46eebc
https://hg.mozilla.org/integration/b2g-inbound/rev/f6b637a621ac

Comment 61

[Ed Morley [:emorley]]

Backed out for conflicts with the changes on mozilla-central & for "adding more VPATH horror to the tree" (per glandium):
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/c1ea56013ce9
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/e273e39089cb
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/d545f426a57f

Comment 62

[Ed Morley [:emorley]]

edmorley|sheriff: glandium: hi, are you around? :-)
edmorley|sheriff: glandium: (boilerplate removal conflicts)
edmorley|sheriff: glandium: is this right? http://snag.gy/5XaEg.jpg or can I get rid in the rules.mk include too?
edmorley|sheriff: glandium: added by bug 864485
glandium: edmorley|sheriff: rules.mk can only go if it's the last line
glandium: yay for no build peer review
edmorley|sheriff: glandium: happy to back out bug 864485 if you'd prefer?
glandium: edmorley|sheriff: i'm tempted to say backout, because adding more VPATH horror is really not nice
edmorley|sheriff: glandium: yeah I was slightly surprised to see they'd done that tbh
NeilAway: edmorley|sheriff: possibly copied from dom/mobilemessage/src/Makefile.in
glandium: NeilAway: which, guess what... was not review by a build peer either
edmorley|sheriff: glandium: oh actually khuey did review lol
edmorley|sheriff: obsoleted patch
edmorley|sheriff: glandium: should I reland, or would you rather they fix?
glandium: edmorley|sheriff: you backed it out?
edmorley|sheriff: glandium: yes, I thought that was your preference :-)
glandium: edmorley|sheriff: it still is :)

Comment 63

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Ed Morley [:edmorley UTC+1] from comment #62)
> edmorley|sheriff: glandium: hi, are you around? :-)
> edmorley|sheriff: glandium: (boilerplate removal conflicts)
> edmorley|sheriff: glandium: is this right? http://snag.gy/5XaEg.jpg or can I
> get rid in the rules.mk include too?
> edmorley|sheriff: glandium: added by bug 864485
> glandium: edmorley|sheriff: rules.mk can only go if it's the last line

$ grep -nR config/rules.mk -A 3 dom/
dom/power/Makefile.in:14:include $(topsrcdir)/config/rules.mk
dom/power/Makefile.in-15-include $(topsrcdir)/ipc/chromium/chromium-config.mk
--
dom/time/Makefile.in:14:include $(topsrcdir)/config/rules.mk
dom/time/Makefile.in-15-include $(topsrcdir)/ipc/chromium/chromium-config.mk
--
dom/media/Makefile.in:23:include $(topsrcdir)/config/rules.mk
dom/media/Makefile.in-24-include $(topsrcdir)/ipc/chromium/chromium-config.mk
--
dom/camera/Makefile.in:13:include $(topsrcdir)/config/rules.mk
dom/camera/Makefile.in-14-include $(topsrcdir)/ipc/chromium/chromium-config.mk
--
dom/fmradio/ipc/Makefile.in:18:include $(topsrcdir)/config/rules.mk
dom/fmradio/ipc/Makefile.in-19-include $(topsrcdir)/ipc/chromium/chromium-config.mk
--
dom/src/events/Makefile.in:12:include $(topsrcdir)/config/rules.mk
dom/src/events/Makefile.in-13-
dom/src/events/Makefile.in-14-INCLUDES  += -I$(topsrcdir)/dom/base
dom/src/events/Makefile.in-15-INCLUDES  += -I$(topsrcdir)/content/base/src
--
dom/src/storage/Makefile.in:20:include $(topsrcdir)/config/rules.mk
dom/src/storage/Makefile.in-21-include $(topsrcdir)/ipc/chromium/chromium-config.mk
--
dom/src/notification/Makefile.in:19:include $(topsrcdir)/config/rules.mk
dom/src/notification/Makefile.in-20-include $(topsrcdir)/ipc/chromium/chromium-config.mk
--
dom/src/geolocation/Makefile.in:37:include $(topsrcdir)/config/rules.mk
dom/src/geolocation/Makefile.in-38-include $(topsrcdir)/ipc/chromium/chromium-config.mk
--
dom/browser-element/Makefile.in:15:include $(topsrcdir)/config/rules.mk
dom/browser-element/Makefile.in-16-include $(topsrcdir)/ipc/chromium/chromium-config.mk
dom/browser-element/Makefile.in-17-
dom/browser-element/Makefile.in-18-INCLUDES     += \
--
dom/bindings/Makefile.in:123:include $(topsrcdir)/config/rules.mk
dom/bindings/Makefile.in-124-include $(topsrcdir)/ipc/chromium/chromium-config.mk
...

Above are only part of the output.

> glandium: yay for no build peer review
> edmorley|sheriff: glandium: happy to back out bug 864485 if you'd prefer?
> glandium: edmorley|sheriff: i'm tempted to say backout, because adding more
> VPATH horror is really not nice
> edmorley|sheriff: glandium: yeah I was slightly surprised to see they'd done
> that tbh
> NeilAway: edmorley|sheriff: possibly copied from
> dom/mobilemessage/src/Makefile.in
> glandium: NeilAway: which, guess what... was not review by a build peer
> either
> edmorley|sheriff: glandium: oh actually khuey did review lol
> edmorley|sheriff: obsoleted patch
> edmorley|sheriff: glandium: should I reland, or would you rather they fix?
> glandium: edmorley|sheriff: you backed it out?
> edmorley|sheriff: glandium: yes, I thought that was your preference :-)
> glandium: edmorley|sheriff: it still is :)

With |moz.build|, I think it's possible to have a VPATH-free |Makefile.in|.  Will revise and re-land.

Comment 64

[Vicamo Yang [:vicamo][:vyang]]

Makefile.in without VPATH throughout m-c source tree are:

  ./memory/jemalloc/src/Makefile.in
    -> hand made Makefile.in actually

  ./config/makefiles/precompile/Makefile.in
    -> $(MAKE) -C other-dirs

  ./intl/icu/source
    -> not even Mozilla's

  ./python/mozbuild/mozbuild/test/backend/data
    -> empty files, 0L, 0C

|moz.build| code generator gives me |objdir-gecko/dom/telephony/backend.mk|, which needs at least `VPATH = @srcdir@` to work.

Comment 65

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/3: DOM & IPC : v10

1) Remove VPATH usage completely.  m-c change make us really VPATH-free.
2) Rebased after m-c to b2g-inbound merge.  No more m-c conflict, at least for now.

Comment 66

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/3: Gonk backend specific : v7

Avoid VPATH.

Comment 67

[Vicamo Yang [:vicamo][:vyang]]

Full try for bug 864485, bug 907585, and bug 873351: https://tbpl.mozilla.org/?tree=Try&rev=2b850251174f

Comment 68

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

fwiw I signed off on it because it was just copying what was being done elsewhere in bluetooth and other things, so it wasn't making anything worse.

Comment 69

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/e27c780e1ef5
https://hg.mozilla.org/integration/b2g-inbound/rev/20ef446b27cd
https://hg.mozilla.org/integration/b2g-inbound/rev/d6920653de69

Comment 70

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e27c780e1ef5
https://hg.mozilla.org/mozilla-central/rev/20ef446b27cd
https://hg.mozilla.org/mozilla-central/rev/d6920653de69

Comment 71

[Ryan VanderMeulen [:RyanVM]]

Backed out at gwagner's request because apparently we like it when phones are able to boot. Who knew.
https://hg.mozilla.org/integration/b2g-inbound/rev/dc4758d44b11

Comment 72

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/3: DOM & IPC : v11

Rebase only.

Comment 73

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/6e322f9d3e53
https://hg.mozilla.org/integration/b2g-inbound/rev/7105ab972801
https://hg.mozilla.org/integration/b2g-inbound/rev/b119d237e59c

Comment 74

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6e322f9d3e53
https://hg.mozilla.org/mozilla-central/rev/7105ab972801
https://hg.mozilla.org/mozilla-central/rev/b119d237e59c

Comment 75

[Masatoshi Kimura [:emk]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #58)
> Created attachment 799589 [details] [diff] [review]
> part 2/3: DOM & IPC : v9
> 
> fix mochitest-3 bustage.

Are these APIs enough stable to pollute the global namespace even on desktop Firefox?

Comment 76

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Masatoshi Kimura [:emk] from comment #75)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #58)
> > Created attachment 799589 [details] [diff] [review]
> > part 2/3: DOM & IPC : v9
> > 
> > fix mochitest-3 bustage.
> 
> Are these APIs enough stable to pollute the global namespace even on desktop
> Firefox?

Please see bug 859616 comment 13.  I had the same question and want to hide MobileMessage builds/symbols in other platform that doesn't support/want B2G RIL APIs.  But the answer I got in Mounir's comment is Mozilla doesn't like having more and more preprocessor definitions, so I released WebTelephony from MOZ_B2G_RIL just like WebSMS is not guarded by MOZ_WEBSMS_BACKEND.

If you feel otherwise, please join the discuss in bug 859616.  I'm glad to re-open that bug if there is any change to the conclusion there.  Before that, we can still file a bug to have a preference "dom.telephony.enabled" to imitate mobilemessage's behaviour.  Anyway, if DOM peers have a conclusion on these things, we'll follow.

Comment 77

[Masatoshi Kimura [:emk]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #76)
> Please see bug 859616 comment 13.  I had the same question and want to hide
> MobileMessage builds/symbols in other platform that doesn't support/want B2G
> RIL APIs.  But the answer I got in Mounir's comment is Mozilla doesn't like
> having more and more preprocessor definitions, so I released WebTelephony
> from MOZ_B2G_RIL just like WebSMS is not guarded by MOZ_WEBSMS_BACKEND.

I'm not saying anything about the compile-time option.

> Before
> that, we can still file a bug to have a preference "dom.telephony.enabled"
> to imitate mobilemessage's behaviour.

Please do.

> Anyway, if DOM peers have a
> conclusion on these things, we'll follow.

You should have asked a DOM peer *before* landing as written in the test file.

Comment 78

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Masatoshi Kimura [:emk] from comment #77)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #76)
> > Anyway, if DOM peers have a
> > conclusion on these things, we'll follow.
> 
> You should have asked a DOM peer *before* landing as written in the test
> file.

r=khuey was claimed, but that seems untrue. Why were significant changes made to the patch after review?

Comment 79

[Vicamo Yang [:vicamo][:vyang]]

(In reply to :Ms2ger (away 11-21 September) from comment #78)
> r=khuey was claimed, but that seems untrue. Why were significant changes
> made to the patch after review?

I don't understand.  I had addressed this problem in comment 10 and khuey reviewed and gives r+ in comment 31.  So what do you mean by "after" review?  You can trace all the changes in obsoleted patches, can't you?

Comment 80

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Masatoshi Kimura [:emk] from comment #77)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #76)
> > Please see bug 859616 comment 13.  I had the same question and want to hide
> > MobileMessage builds/symbols in other platform that doesn't support/want B2G
> > RIL APIs.  But the answer I got in Mounir's comment is Mozilla doesn't like
> > having more and more preprocessor definitions, so I released WebTelephony
> > from MOZ_B2G_RIL just like WebSMS is not guarded by MOZ_WEBSMS_BACKEND.
> 
> I'm not saying anything about the compile-time option.

And that affects more than compile-time option.  You can find SmsMessage/SmsEvent/MmsMessage/... interface in desktop Firefox as well.  Please note I'm the person who wanted to hide all the symbols.  This is not my intention at all.

> > Before
> > that, we can still file a bug to have a preference "dom.telephony.enabled"
> > to imitate mobilemessage's behaviour.
> 
> Please do.

Glad to.

> > Anyway, if DOM peers have a
> > conclusion on these things, we'll follow.
> 
> You should have asked a DOM peer *before* landing as written in the test file.

As replied above.  I don't accept this accusation.

Comment 81

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #79)
> (In reply to :Ms2ger (away 11-21 September) from comment #78)
> > r=khuey was claimed, but that seems untrue. Why were significant changes
> > made to the patch after review?
> 
> I don't understand.  I had addressed this problem in comment 10 and khuey
> reviewed and gives r+ in comment 31.  So what do you mean by "after" review?
> You can trace all the changes in obsoleted patches, can't you?

The only r+ from a DOM peer is on attachment 794612 [details] [diff] [review]. That file doesn't appear to touch test_interfaces.html.

Comment 82

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Hey folks, we're all on the same team here.  We'll fix the issue in Bug 914182.

It less obvious that it could be that this test is important and that changes to it need review.  We'll work on making that more clear.