Closed Bug 864485 Opened 6 years ago Closed 6 years ago

B2G RIL: use ipdl as IPC in MozTelephony

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: vicamo, Assigned: vicamo)

References

Details

Attachments

(3 files, 27 obsolete files)

9.88 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
66.10 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
65.32 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
No description provided.
Attached patch WIP (obsolete) — Splinter Review
Based on bug 888592, WebTelephony WebIDL port.
Assignee: nobody → vyang
Depends on: 899885
Attached patch part 2/4: Interface changes (obsolete) — Splinter Review
Attachment #791668 - Attachment description: part 1/4: WIP2 → part 1/4: Rename nsITelephonyProvider to nsITelephonyService. WIP2
Attached patch part 3/4: DOM & IPC. WIP2 (obsolete) — Splinter Review
Works on B2G only.  Probably brings in build breaks on other platforms because IPDL parts are necessary in spite of targets.
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.
Attachment #791668 - Attachment is obsolete: true
Attachment #791776 - Flags: review?(htsai)
Attachment #791776 - Attachment description: part 1/4: Rename nsITelephonyProvider to nsITelephonyService. WIP2 → part 1/4: Rename nsITelephonyProvider to nsITelephonyService.
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.
Attachment #791669 - Flags: superreview?(htsai)
Attachment #791669 - Attachment description: part 2/4: Interface changes. WIP2 → part 2/4: Interface changes
Attached patch part 3/4: DOM & IPC (obsolete) — Splinter Review
Attachment #791670 - Attachment is obsolete: true
Attachment #791778 - Flags: review?(bent.mozilla)
Attached patch part 4/4: Gonk backend specific (obsolete) — Splinter Review
Move telephony related parts out of RadioInterfaceLayer.js into gonk/TelephonyService.js.
Attachment #791671 - Attachment is obsolete: true
Attachment #791779 - Flags: review?(htsai)
From bug 859616 comment 13, now telephony parts are also built but disabled on platforms other than Gonk.
(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 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.
Attachment #791669 - Flags: superreview?(htsai) → superreview-
(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.
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.
Attached patch part 3/4: DOM & IPC : v2 (obsolete) — Splinter Review
Address comment 12 and fix several code alignments.
Attachment #791778 - Attachment is obsolete: true
Attachment #791778 - Flags: review?(bent.mozilla)
Attachment #791986 - Flags: review?(bent.mozilla)
Attachment #791669 - Flags: review?(bent.mozilla)
Attached patch part 3/4: DOM & IPC : v3 (obsolete) — Splinter Review
using mozilla::ErrorResult;

https://tbpl.mozilla.org/?tree=Try&rev=cdbc09103f0c
Attachment #791986 - Attachment is obsolete: true
Attachment #791986 - Flags: review?(bent.mozilla)
Attachment #792552 - Flags: review?(htsai)
rebase after bug 772765 -- conference call
Attachment #791776 - Attachment is obsolete: true
Attachment #791776 - Flags: review?(htsai)
Attachment #794611 - Flags: review?(htsai)
Attached patch part 2/4: Interface changes : v2 (obsolete) — Splinter Review
rebase after bug 772765 -- conference call
Attachment #791669 - Attachment is obsolete: true
Attachment #791669 - Flags: review?(bent.mozilla)
Attachment #794612 - Flags: superreview?(htsai)
Attachment #794612 - Flags: review?(bent.mozilla)
Attached patch part 3/4: DOM & IPC : v4 (obsolete) — Splinter Review
rebase after bug 772765 -- conference call
Attachment #792552 - Attachment is obsolete: true
Attachment #792552 - Flags: review?(htsai)
Attachment #794613 - Flags: review?(bent.mozilla)
rebase after bug 772765 -- conference call
Attachment #791779 - Attachment is obsolete: true
Attachment #791779 - Flags: review?(htsai)
Attachment #794614 - Flags: review?(htsai)
Comment on attachment 794612 [details] [diff] [review]
part 2/4: Interface changes : v2

I'm going to defer to khuey here. Thanks!
Attachment #794612 - Flags: review?(bent.mozilla) → review?(khuey)
Attachment #794613 - Flags: review?(bent.mozilla) → review?(khuey)
I would like to know if Hsin-Yi is satisfied with comment 14 before I spend time reviewing this.
Flags: needinfo?(htsai)
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.
Attachment #794612 - Flags: superreview?(htsai) → superreview+
Flags: needinfo?(htsai)
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).
(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 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!
Attachment #794612 - Flags: superreview?(jonas)
Attachment #794612 - Flags: superreview+
Attachment #794612 - Flags: review+
(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.
(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 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 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.
Attachment #794613 - Flags: review?(khuey) → review+
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.
Attachment #794612 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
>@@ +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.
Blocks: 910568
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Attachment #794612 - Flags: superreview?(jonas) → superreview+
Rebase only
Attachment #794611 - Attachment is obsolete: true
Attachment #794611 - Flags: review?(htsai)
Attachment #797644 - Flags: review?(htsai)
Attached patch part 2/4: Interface changes : v3 (obsolete) — Splinter Review
Address comment 32 -- fix indentation and remove union IPCTelephonyRequest and struct IPCRequestEnumerateCalls completely.
Attachment #794612 - Attachment is obsolete: true
Attachment #797646 - Flags: superreview+
Attachment #797646 - Flags: review+
Attached patch part 3/4: DOM & IPC : v5 (obsolete) — Splinter Review
1) address various nits in comment 31
2) accommodate to removal of union IPCTelephonyRequest and struct IPCRequestEnumerateCalls.
3) rebase
Attachment #794613 - Attachment is obsolete: true
Attachment #797647 - Flags: review+
Rebase only.
Attachment #794614 - Attachment is obsolete: true
Attachment #794614 - Flags: review?(htsai)
Attachment #797648 - Flags: review?(htsai)
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.
Attachment #797644 - Flags: review?(htsai)
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.
Attachment #797648 - Flags: review?(htsai)
Attached patch part 1/3: Interface changes : v4 (obsolete) — Splinter Review
Don't rename nsITelephonyProvider for now.  Address comment 40.
Attachment #797644 - Attachment is obsolete: true
Attachment #797646 - Attachment is obsolete: true
Attachment #798447 - Flags: review?(htsai)
Attached patch part 2/3: DOM & IPC : v6 (obsolete) — Splinter Review
Stick with nsITelephonyProvider.
Attachment #797647 - Attachment is obsolete: true
Attachment #798450 - Flags: review+
1) Address comment 40.
2) Add TelephonyProvider._notifyAllListeners to save common loops and improve error handling.
Attachment #797648 - Attachment is obsolete: true
Attachment #798453 - Flags: review?(htsai)
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.
Attachment #798447 - Flags: review?(htsai) → review+
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.
Attachment #798453 - Flags: review?(htsai) → review+
(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 ;-)
1) Remove hsinyi from sr
2) Rebase after bug 888593 and bug 870676 (currently in b2g-inbound).
Attachment #798447 - Attachment is obsolete: true
Attachment #799264 - Flags: review+
Attached patch part 2/3: DOM & IPC : v7 (obsolete) — Splinter Review
Rebase after bug 888593 and bug 870676 (currently in b2g-inbound).
Attachment #798450 - Attachment is obsolete: true
Attachment #799265 - Flags: review+
1) Fix |gPowerManagerService| undefined.
2) Rebase after bug 888593 and bug 870676 (currently in b2g-inbound).
Attachment #798453 - Attachment is obsolete: true
Attachment #799266 - Flags: review+
(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.
Attached patch part 2/3: DOM & IPC : v8 (obsolete) — Splinter Review
fix debug build bustage.
Attachment #799265 - Attachment is obsolete: true
Attachment #799306 - Flags: review+
Attached patch part 2/3: DOM & IPC : v9 (obsolete) — Splinter Review
fix mochitest-3 bustage.
Attachment #799306 - Attachment is obsolete: true
Attachment #799589 - Flags: review+
Rebase after bug 903403 (currently in b2g-inbound)
Attachment #799266 - Attachment is obsolete: true
Attachment #799590 - Flags: review+
Blocks: 911986
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
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 :)
(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.
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.
Attached patch part 2/3: DOM & IPC : v10 (obsolete) — Splinter Review
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.
Attachment #799589 - Attachment is obsolete: true
Attachment #800145 - Flags: review+
Avoid VPATH.
Attachment #799590 - Attachment is obsolete: true
Attachment #800146 - Flags: review+
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.
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla26 → ---
Rebase only.
Attachment #800145 - Attachment is obsolete: true
Attachment #801083 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6e322f9d3e53
https://hg.mozilla.org/mozilla-central/rev/7105ab972801
https://hg.mozilla.org/mozilla-central/rev/b119d237e59c
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(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?
Flags: needinfo?(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.
Flags: needinfo?(vyang)
(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.
Depends on: 914182
(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?
Flags: needinfo?(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?
Flags: needinfo?(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.
(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.
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.
Blocks: 914631
Blocks: 1064231
No longer blocks: 1064231
You need to log in before you can comment on or make changes to this bug.