Closed Bug 920551 Opened 6 years ago Closed 6 years ago

B2G RIL: allow building B2G without all RIL functions

Categories

(Firefox OS Graveyard :: RIL, defect, P1)

Other
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:koi+, firefox26 wontfix, firefox27 wontfix, firefox28 fixed, b2g-v1.2 fixed)

RESOLVED FIXED
1.2 C3(Oct25)
blocking-b2g koi+
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: vicamo, Assigned: vicamo)

References

Details

(Whiteboard: [Flatfish only, FT:RIL])

Attachments

(22 files, 54 obsolete files)

1.69 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
2.11 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
12.14 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
37.40 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
15.55 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
1.40 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
3.08 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
4.90 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
2.79 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
58.83 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
34.06 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
1.70 KB, patch
Details | Diff | Splinter Review
2.11 KB, patch
Details | Diff | Splinter Review
12.15 KB, patch
Details | Diff | Splinter Review
37.18 KB, patch
Details | Diff | Splinter Review
15.46 KB, patch
Details | Diff | Splinter Review
1.35 KB, patch
Details | Diff | Splinter Review
2.86 KB, patch
Details | Diff | Splinter Review
4.61 KB, patch
Details | Diff | Splinter Review
56.09 KB, patch
Details | Diff | Splinter Review
21.66 KB, patch
Details | Diff | Splinter Review
2.42 KB, patch
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #911684 +++

The way Gaia determines how a RIL related page is present is to test whether "navigator.mozTelephony" is undefined.  Although we have already bug 914182, which hides WebTelephony APIs from non-privileged users, that isn't the case we have to deal with here because Gaia FTU has already "telephony" permission.  We need some way to disable MOZ_B2G_RIL even when we're building B2G with Gonk[1].

[1]: https://mxr.mozilla.org/mozilla-central/source/configure.in#1939
Might relate to bug 915604.
I still can't have clean source build on flatfish.  Device configuration is still unavailable in |B2G/config.sh|.  Will investigate work on emulator first.
Assignee: nobody → vyang
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #0)
> "navigator.mozTelephony" is undefined.  Although we have already bug 914182,
> which hides WebTelephony APIs from non-privileged users,

Unfortunately I failed to hide the API from non-privileged users on B2G (see bug 914182 comment #28).
Looks like marionette and mochitest is executed as a certiied app which has most permissions, and I couldn't change the exposure dynamically.
(In reply to Masatoshi Kimura [:emk] from comment #3)
> Unfortunately I failed to hide the API from non-privileged users on B2G (see
> bug 914182 comment #28).
> Looks like marionette and mochitest is executed as a certiied app which has
> most permissions, and I couldn't change the exposure dynamically.

Well, from another point of view, that might make this bug a little bit easier :D
Blocks: 911676
Blocks: flatfish
Blocks: 921912
Whiteboard: [Flatfish only]
Depends on: 915604
Still discuss a way to pass dom/tests/mochitest/general/test_interfaces.html in bug 915604, if you're interested.
Depend on bug 814556 to clean up MOZ_B2G_RIL in navigator & package-manifest.in first.
Depends on: 814556
Target Milestone: --- → 1.3 Sprint 3 - 10/25
Depends on: 928112
this issue raised potential risk to impact Flatfish schedule. P1 for current Flatfish progress
Priority: -- → P1
Attached patch patch : WIP (obsolete) — Splinter Review
Attached patch part 1/3: clean up MOZ_B2G_RIL (obsolete) — Splinter Review
This patch mainly draws a line between MOZ_B2G_RIL and MOZ_WIDGET_GONK.  Those really belong to RIL functions are now protected with MOZ_B2G_RIL, while those for Gonk system are protected with MOZ_WIDGET_GONK.  Only cellbroadcast/icc/mobileconnection/voicemail/wappush are now considered part of RIL, others like wifi and networkstats belong to Gonk.
Attachment #818960 - Attachment is obsolete: true
With this patch, MOZ_B2G_RIL is not always set along with MOZ_WIDGET_GONK.  Platform might give a hint about RIL support, but MOZ_B2G_RIL can still be explicitly disabled.
Attachment #819310 - Attachment description: part 1/2: clean up MOZ_B2G_RIL → part 1/3: clean up MOZ_B2G_RIL
Attachment #819312 - Attachment description: part 2.a/2: Allow building b2g without MOZ_B2G_RIL → part 2/3: Allow building b2g without MOZ_B2G_RIL
Attachment #819312 - Attachment filename: bug-920551-2-a.patch → bug-920551-2.patch
Remove a FIXME comment added in previous revision.
Attachment #819310 - Attachment is obsolete: true
Attachment #819340 - Flags: review?(khuey)
update commit summary only
Attachment #819312 - Attachment is obsolete: true
Attachment #819341 - Flags: review?(khuey)
Attached patch part 3.a/3: fix Bluetooth (obsolete) — Splinter Review
This patch fixes compile errors in dom/bluetooth when b2g is built with --disable-b2g-ril by simply guards all RIL related code with MOZ_B2G_RIL.  However, I'm a little bit wondering whether HFP still mean a lot when the phone just have no RIL.
Attachment #819342 - Flags: feedback?(gyeh)
This patch fixes compile errors in dom/system/gonk/GonkGPSGeolocationProvider.* when b2g is built with --disable-b2g-ril.  By simply guarding all RIL related code with MOZ_B2G_RIL, it still have the same problem with Bluetooth -- some call paths become actually dead or meaningless.
Attachment #819343 - Flags: feedback?(kchen)
Attached patch part 3.c/3: fix MobileMessage (obsolete) — Splinter Review
This patch fixes compile errors in dom/system/gonk/GonkGPSGeolocationProvider.* when b2g is built with --disable-b2g-ril.

Files under gonk/ were based on the assumption that RIL is always available on gonk.  It's certainly not true now.  Remove |test_smsdatabaseservice.xul| as well because it's completely out-dated and has been disabled for a long long time.
Attachment #819348 - Flags: review?(khuey)
Attachment #819348 - Flags: review?(gene.lian)
Attached patch part 3.d/3: fix MobileConnection (obsolete) — Splinter Review
Don't include |MobileConnection.h| directly.  Use |mozilla/dom/network/MobileConnection.h| instead.
Attachment #819349 - Flags: review?(khuey)
This patch fixes compile errors in dom/system/gonk/SystemWorkerManager.* when b2g is built with --disable-b2g-ril.

1) Removed all MOZ_WIDGET_GONK guards because this folder is only built when MOZ_WIDGET_GONK is defined.

2) Move RIL IPC code to ipc/ril/Ril.cpp.  Two static member functions RilConsumer::Register and RilConsumer::Shutdown are created as interfaces to SystemWorkerManager.
Attachment #819350 - Flags: review?(kyle)
Attached patch part 3./f3: fix Telephony (obsolete) — Splinter Review
This patch removes files under dom/telephony/gonk from installation when b2g is built with --disable-b2g-ril.
Attachment #819351 - Flags: review?(khuey)
Attachment #819351 - Flags: review?(htsai)
Blocks: 928643
Bug 911684 and bug 921912 are actually blocked by bug 928643.
No longer blocks: 911684, 921912
typo: s/aRaw/mRawData/
Attachment #819350 - Attachment is obsolete: true
Attachment #819350 - Flags: review?(kyle)
Attachment #819472 - Flags: review?(khuey)
Comment on attachment 819343 [details] [diff] [review]
part 3.b/3: fix GonkGPSGeolocationProvider

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

Yeah, like you said, you could also make the code path reasonable without the RIL path. Is this patch your final version?
Attachment #819343 - Flags: feedback?(kchen) → feedback+
Sorry, I'm going to restruct these patches, break the first one into rest ones so that we may have a better understanding what's going on in each component.
Update commit summary only.  No change since attachment 9819341.
Attachment #819341 - Attachment is obsolete: true
Attachment #819341 - Flags: review?(khuey)
Attachment #819627 - Flags: review?(khuey)
Update commit summary only.  No change since attachment 819348 [details] [diff] [review].
Attachment #819348 - Attachment is obsolete: true
Attachment #819348 - Flags: review?(khuey)
Attachment #819348 - Flags: review?(gene.lian)
Attachment #819628 - Flags: review?(khuey)
Attachment #819628 - Flags: review?(gene.lian)
Attached patch part 2.b/2: fix Telephony : v2 (obsolete) — Splinter Review
1) Don't guard mozTelephony with MOZ_B2G_RIL.  It should be built on all platforms and is only enabled when "dom.telephony.enabled" turns true.

2) Like MobileMessage, dom/telephony/gonk is guarded by both MOZ_WIDGET_GONK and MOZ_B2G_RIL.
Attachment #819351 - Attachment is obsolete: true
Attachment #819351 - Flags: review?(khuey)
Attachment #819351 - Flags: review?(htsai)
Attachment #819631 - Flags: review?(khuey)
Attachment #819631 - Flags: review?(htsai)
Attached patch part 2.c/2: fix CellBroadcast (obsolete) — Splinter Review
Move |pref("dom.cellbroadcast.enabled", true);| into modules/libpref/src/init/all.js and be conditionally selected by MOZ_B2G_RIL.
Attachment #819634 - Flags: review?(htsai)
Attached patch part 2.d/2: fix ICC (obsolete) — Splinter Review
1) Move |pref("dom.icc.enabled", true);| into modules/libpref/src/init/all.js and be conditionally selected by MOZ_B2G_RIL.

2) Group |MozStkCommandEvent.webidl| with other MOZ_B2G_RIL guarded WebIDL files.
Attachment #819636 - Flags: review?(khuey)
Attachment #819636 - Flags: review?(htsai)
Attached patch part 2.e/2: fix MobileConnection (obsolete) — Splinter Review
1) Move |pref("dom.mobileconnection.enabled", true);| into modules/libpref/src/init/all.js and be conditionally selected by MOZ_B2G_RIL.

2) Use "mozilla/dom/network/MobileConnection.h" instead of "MobileConnection.h".

3) Guard nsIDOMDataErrorEvent.idl and nsIDOMUSSDReceivedEvent.idl with MOZ_B2G_RIL as well.
Attachment #819349 - Attachment is obsolete: true
Attachment #819349 - Flags: review?(khuey)
Attachment #819637 - Flags: review?(khuey)
Attachment #819637 - Flags: review?(htsai)
Attached patch part 2.f/2: fix Voicemail (obsolete) — Splinter Review
1) Move |pref("dom.voicemail.enabled", true);| into modules/libpref/src/init/all.js and be conditionally selected by MOZ_B2G_RIL.

2) Group |MozVoicemail.webidl| with other MOZ_B2G_RIL guarded WebIDL files.
Attachment #819639 - Flags: review?(htsai)
Attached patch part 2.g/2: fix WAP Push (obsolete) — Splinter Review
1) Only build WAP Push when MOZ_B2G_RIL is enabled.

2) Like MobileMessage, files under dom/wappush/src/gonk should only be installed when MOZ_WIDGET_GONK is defined as well.
Attachment #819340 - Attachment is obsolete: true
Attachment #819340 - Flags: review?(khuey)
Attachment #819642 - Flags: review?(khuey)
Attachment #819642 - Flags: review?(htsai)
1) Removed all MOZ_WIDGET_GONK guards because this folder is only built when MOZ_WIDGET_GONK is defined.

2) Move RIL IPC code to ipc/ril/Ril.cpp.  Two static member functions RilConsumer::Register and RilConsumer::Shutdown are created as interfaces to SystemWorkerManager.

3) Components other that RIL are built along with MOZ_WIDGET_GONK.
Attachment #819472 - Attachment is obsolete: true
Attachment #819472 - Flags: review?(khuey)
Attachment #819644 - Flags: review?(vchang)
Attachment #819644 - Flags: review?(kyle)
Attachment #819644 - Flags: review?(khuey)
Attachment #819644 - Flags: review?(htsai)
Attached patch part 2.i/2: fix Bluetooth (obsolete) — Splinter Review
No change since attachment 819342 [details] [diff] [review].  Update commit summary only.

Per offline discuss with Gina, we'll prefer looking into implementation details, not simply wrapping RIL related parts with MOZ_B2G_RIL.
Attachment #819342 - Attachment is obsolete: true
Attachment #819342 - Flags: feedback?(gyeh)
Update commit summary only.  No change since attachment 819343 [details] [diff] [review].
Attachment #819343 - Attachment is obsolete: true
Blocks: 928860
(In reply to Kan-Ru Chen [:kanru] from comment #21)
> Yeah, like you said, you could also make the code path reasonable without
> the RIL path. Is this patch your final version?

I mean, should I disable AGPS completely?  Will AGPS somehow work even without RIL?
Flags: needinfo?(kchen)
Comment on attachment 819628 [details] [diff] [review]
part 2.a/2: fix MobileMessage : v2

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

Looks good to me.
Attachment #819628 - Flags: review?(gene.lian) → review+
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #34)
> (In reply to Kan-Ru Chen [:kanru] from comment #21)
> > Yeah, like you said, you could also make the code path reasonable without
> > the RIL path. Is this patch your final version?
> 
> I mean, should I disable AGPS completely?  Will AGPS somehow work even
> without RIL?

Yes, you could disable AGPS completely. It won't work without RIL currently.
Flags: needinfo?(kchen)
This patch hides everything related to AGPS when building without MOZ_B2G_RIL.  Verified with dom/system/gonk/tests/marionette/test_geolocation.js
Attachment #819647 - Attachment is obsolete: true
Attachment #819784 - Flags: review?(kchen)
Attached patch part 2.i/2: fix Bluetooth : v2 (obsolete) — Splinter Review
Hide as much code as possible.  DOM API BluetoothAdapter::AnswerWaitingCall, BluetoothAdapter::IgnoreWaitingCall, BluetoothAdapter::ToggleCalls always throw NS_ERROR_NOT_IMPLEMENTED when built without MOZ_B2G_RIL.
Attachment #819645 - Attachment is obsolete: true
Attachment #819851 - Flags: review?(echou)
Duplicate of this bug: 795424
Comment on attachment 819644 [details] [diff] [review]
part 2.h/2: fix dom/system/gonk/* : v3

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

While the patch seems good, are we sure we want to completely remove the ability to build dom/system/gonk on desktop? I don't have any specific objections, just want to make sure we've covered all the possible use cases.

After this patch, I think that involves

- Audio (which never worked, so non-issue)
- Volume Mounting (which never worked but dhylands is talking about a possible fake mounter that would?)
- RIL (used to work on desktop, I think the network forwarder got ripped out though, and since this patch is by the RIL owner, I trust them to make that decision :) )
- WiFi (mrbkap says there was a shim for it to work on desktop at one point?)

Assuming we're ok with yanking those capabilities from desktop, I'll r+.

::: dom/system/moz.build
@@ +13,5 @@
>  elif toolkit == 'cocoa':
>      DIRS += ['mac']
>  elif toolkit == 'android':
>      DIRS += ['android']
> +elif toolkit == 'gonk':

Making this change means we lose dom/system/gonk on b2g-desktop. Does anyone still test wifi or RIL on desktop? Might want to check on dev-b2g or something.

::: ipc/ril/Ril.cpp
@@ +58,5 @@
> +    {
> +        MOZ_ASSERT(NS_IsMainThread());
> +
> +        if ((sRilConsumers.Length() <= mClientId) ||
> +                !sRilConsumers[mClientId] ||

Nit: indentation should line up

@@ +110,5 @@
> +        }
> +
> +        uint32_t type = JS_GetArrayBufferViewType(obj);
> +        if (type != js::ArrayBufferView::TYPE_INT8 &&
> +                type != js::ArrayBufferView::TYPE_UINT8 &&

Nit: indentation should line up
Attachment #819644 - Flags: review?(kyle)
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #40)
> part 2.h/2: fix dom/system/gonk/* : v3
> 
> Review of attachment 819644 [details] [diff] [review]:
> -----------------------------------------------------------------
> - RIL (used to work on desktop, I think the network forwarder got ripped out
> though, and since this patch is by the RIL owner, I trust them to make that
> decision :) )

I'm seeking chances to include oFono support on desktop, mostly blocked on trying to share ipc/dbus/* with components other than bluetooth.  FxOS Simulator support is also on the plan.  So basically I would like to have RIL working natively on desktop, not just a debug bridge for Android devices.

> - WiFi (mrbkap says there was a shim for it to work on desktop at one point?)

AFAIK, some WiFi/NetworkManager code are moving to either dom/network or dom/wifi, leaving dom/system/gonk home for those are really bounded to Gonk.

> ::: dom/system/moz.build
> @@ +13,5 @@
> >  elif toolkit == 'cocoa':
> >      DIRS += ['mac']
> >  elif toolkit == 'android':
> >      DIRS += ['android']
> > +elif toolkit == 'gonk':
> 
> Making this change means we lose dom/system/gonk on b2g-desktop. Does anyone
> still test wifi or RIL on desktop? Might want to check on dev-b2g or
> something.

MOZ_B2G_RIL was used to guard dom/system/gonk, however, it's only default enabled along with MOZ_WIDGET_GONK.  For official b2g-desktop builds on Mozilla ftp, MOZ_B2G_RIL is *NOT* enabled.  That is, we don't build dom/system/gonk for b2g-deskto no matter this patch is landed or not.
Is it safe to review these patches yet?  I don't want to start if you're going to rewrite them all again ;-)
Flags: needinfo?(vyang)
Attachment #819784 - Flags: review?(kchen) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #42)
> Is it safe to review these patches yet?  I don't want to start if you're
> going to rewrite them all again ;-)

It's ready, thank you ;)
Flags: needinfo?(vyang)
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #40)
> Comment on attachment 819644 [details] [diff] [review]
> part 2.h/2: fix dom/system/gonk/* : v3
> 
> Review of attachment 819644 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> While the patch seems good, are we sure we want to completely remove the
> ability to build dom/system/gonk on desktop? I don't have any specific
> objections, just want to make sure we've covered all the possible use cases.
> 
> After this patch, I think that involves
> 
> - Audio (which never worked, so non-issue)
> - Volume Mounting (which never worked but dhylands is talking about a
> possible fake mounter that would?)
> - RIL (used to work on desktop, I think the network forwarder got ripped out
> though, and since this patch is by the RIL owner, I trust them to make that
> decision :) )
> - WiFi (mrbkap says there was a shim for it to work on desktop at one point?)
> 
> Assuming we're ok with yanking those capabilities from desktop, I'll r+.
> 
> ::: dom/system/moz.build
> @@ +13,5 @@
> >  elif toolkit == 'cocoa':
> >      DIRS += ['mac']
> >  elif toolkit == 'android':
> >      DIRS += ['android']
> > +elif toolkit == 'gonk':
> 
> Making this change means we lose dom/system/gonk on b2g-desktop. Does anyone
> still test wifi or RIL on desktop? Might want to check on dev-b2g or
> something.
> 

I am the one who is still using b2g-desktop to run ril xpcshell tests. It's sad that there's gonna lose a faster way to those tests. We need to run them on emulator afterwards. However, it makes sense to me there's no dom/system/gonk on b2g-desktop as this build doesn't support gonk at all.

> ::: ipc/ril/Ril.cpp
> @@ +58,5 @@
> > +    {
> > +        MOZ_ASSERT(NS_IsMainThread());
> > +
> > +        if ((sRilConsumers.Length() <= mClientId) ||
> > +                !sRilConsumers[mClientId] ||
> 
> Nit: indentation should line up
> 
> @@ +110,5 @@
> > +        }
> > +
> > +        uint32_t type = JS_GetArrayBufferViewType(obj);
> > +        if (type != js::ArrayBufferView::TYPE_INT8 &&
> > +                type != js::ArrayBufferView::TYPE_UINT8 &&
> 
> Nit: indentation should line up
Comment on attachment 819851 [details] [diff] [review]
part 2.i/2: fix Bluetooth : v2

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

I have concerns about inserting too many #ifdef into BluetoothHfpManager.cpp / BluetoothHfpManager.h. It not only makes the code a little bit annoying, but also hid some code which may not be really necessary to be hidden. Let's see if I can make a patch for this tomorrow morning.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #41)

> MOZ_B2G_RIL was used to guard dom/system/gonk, 

Yup. That was the fault of the original RIL developers, who started just blocking RIL code with it then told other people it was a good way to block platform specifics. Damn me, er, them. :)

> however, it's only default
> enabled along with MOZ_WIDGET_GONK.  For official b2g-desktop builds on
> Mozilla ftp, MOZ_B2G_RIL is *NOT* enabled.  That is, we don't build
> dom/system/gonk for b2g-deskto no matter this patch is landed or not.

Keyword there being "default". I've had --enable-b2g-ril and --enable-b2g-bt in my mozconfigs since they came into existence. However, we didn't plan on them working for tbpl, and never filed blockers on them being broken, we just kind of silently fixed things as we went along. That's why I said to ask on dev.b2g or somewhere similar, because there may be other devs that do something similar, and while I realize there shouldn't be expectation of it to work since it wasn't default, now might be a good time to clarify what kind of desktop usage /is/ needed.
Comment on attachment 819627 [details] [diff] [review]
part 1/2: Allow building b2g without MOZ_B2G_RIL : v3

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

::: configure.in
@@ +7267,5 @@
>  if test -n "$MOZ_B2G_RIL"; then
> +    if test -n "$_PLATFORM_HAVE_RIL"; then
> +        AC_DEFINE(MOZ_B2G_RIL)
> +    else
> +        AC_MSG_WARN([b2g-ril not enabled because target platform doesn't support it.])

This should be AC_MSG_ERROR.  We don't like configure options that magically disable themselves.
Attachment #819627 - Flags: review?(khuey) → review+
Comment on attachment 819628 [details] [diff] [review]
part 2.a/2: fix MobileMessage : v2

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

::: b2g/installer/package-manifest.in
@@ +404,5 @@
> +@BINPATH@/components/MmsService.manifest
> +@BINPATH@/components/MobileMessageDatabaseService.js
> +@BINPATH@/components/MobileMessageDatabaseService.manifest
> +#endif // MOZ_B2G_RIL
> +#else // !MOZ_WIDGET_GONK

Please continue to use a separate ifndef block for the addonManager stuff below.  The addon manager and MMS don't have any logical relation to each other.
Attachment #819628 - Flags: review?(khuey) → review+
When we are done here please post to dev-b2g and whatever other lists are relevant telling people to be careful about the difference between MOZ_WIDGET_GONK and MOZ_B2G_RIL from now on.
Comment on attachment 819644 [details] [diff] [review]
part 2.h/2: fix dom/system/gonk/* : v3

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

I think you should create a new define (MOZ_UNIXSOCKET?) for "MOZ_B2G_RIL || MOZ_B2G_BT || MOZ_WIDGET_TOOLKIT == 'gonk'".  That's a bit of a mouthful.  But it's up to you.

The build bits look fine.  I assume the others are reviewing the C++ changes in dom/system/gonk.
Attachment #819644 - Flags: review?(khuey) → review+
Attachment #819631 - Flags: review?(htsai) → review+
Attachment #819634 - Flags: review?(htsai) → review+
Attachment #819636 - Flags: review?(htsai) → review+
Attachment #819637 - Flags: review?(htsai) → review+
Comment on attachment 819639 [details] [diff] [review]
part 2.f/2: fix Voicemail

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

It looks good to me. But we will need build peer's review as I am not. Kyle, may I have your review on this as well? Thank you.
Attachment #819639 - Flags: review?(khuey)
Attachment #819639 - Flags: review?(htsai)
Attachment #819639 - Flags: review+
Attachment #819642 - Flags: review?(htsai) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #47)
> part 1/2: Allow building b2g without MOZ_B2G_RIL : v3
> -----------------------------------------------------------------
> 
> ::: configure.in
> @@ +7267,5 @@
> > +        AC_MSG_WARN([b2g-ril not enabled because target platform doesn't support it.])
> 
> This should be AC_MSG_ERROR.  We don't like configure options that magically
> disable themselves.

Yup! I told to Yoshi yesterday, if I were to review this, I'd like to replace it with AC_MSG_ERROR as well.  I'll update that message as well. :D
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #49)
> When we are done here please post to dev-b2g and whatever other lists are
> relevant telling people to be careful about the difference between
> MOZ_WIDGET_GONK and MOZ_B2G_RIL from now on.

Already done: https://groups.google.com/d/msg/mozilla.dev.b2g/HCbVkIC7_Ro/9N3Eq6nisbAJ
ril enabled desktop builds are also used to build contact databases for reference workloads.
(In reply to Dave Hylands [:dhylands] from comment #54)
> ril enabled desktop builds are also used to build contact databases for
> reference workloads.

[Bug 806598, attachment 709105 [details] [diff] [review]] Looks like it can be converted to a chrome Marionette test case, or even better, use content Marionette and don't try to use internal APIs. Hmm...
Comment on attachment 819851 [details] [diff] [review]
part 2.i/2: fix Bluetooth : v2

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

ok, r+ per offline discussion with Vicamo and Gina. We'll do some cleanup stuff on Gecko Bluetooth afterwards, trying to decouple RIL and HFP. Gina will be in charge of this.
Attachment #819851 - Flags: review?(echou) → review+
I'll file a follow-up and update the bug number soon. ;)
Comment on attachment 819644 [details] [diff] [review]
part 2.h/2: fix dom/system/gonk/* : v3

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

It's a bit annoying to have #ifdef spreading around NetworkManager.js. Hopefully we could clean it up in Bug 904514.

r=me (with the comment addressed) for this patch except the parts of SystemWorkerManager.cpp/.h and Ril.cpp/.h. I believe qdot is the best person for reviewing them. Thanks :)

::: dom/system/gonk/NetworkManager.js
@@ +248,3 @@
>              // Update data connection when Wifi connected/disconnected
>              if (network.type == Ci.nsINetworkInterface.NETWORK_TYPE_WIFI) {
>                this.mRIL.getRadioInterface(0).updateRILNetworkInterface();

Aha, it reminds me the bug for handling this in DSDS is missing. ;)

@@ +291,1 @@
>        case TOPIC_INTERFACE_REGISTERED:

If we don't care this topic, then we shall even not add observer to this topic at all.

@@ +292,5 @@
>          let regNetwork = subject.QueryInterface(Ci.nsINetworkInterface);
>          // Add extra host route. For example, mms proxy or mmsc.
>          this.setExtraHostRoute(regNetwork);
>          break;
>        case TOPIC_INTERFACE_UNREGISTERED:

ditto.
Attachment #819644 - Flags: review?(htsai) → review+
Whiteboard: [Flatfish only] → [Flatfish only, FT:RIL]
Target Milestone: 1.3 Sprint 3 - 10/25 → 1.2 C3(Oct25)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #50)
> Comment on attachment 819644 [details] [diff] [review]
> part 2.h/2: fix dom/system/gonk/* : v3
> -----------------------------------------------------------------
> I think you should create a new define (MOZ_UNIXSOCKET?) for "MOZ_B2G_RIL ||
> MOZ_B2G_BT || MOZ_WIDGET_TOOLKIT == 'gonk'".  That's a bit of a mouthful. 
> But it's up to you.

I think ipc/moz.build is the only place that has such line, so will be the newly created MOZ_UNIXSOCKET.  I think it's unnecessary to have a new global config variable that applies to only one place.  Thank you.
Address comment 47 -- use AC_MSG_ERROR instead.
Attachment #819627 - Attachment is obsolete: true
Attachment #820921 - Flags: review+
Address review comment 48.
Attachment #819628 - Attachment is obsolete: true
Attachment #820922 - Flags: review+
Attached patch part 2.b/2: fix Telephony : v3 (obsolete) — Splinter Review
Rebase only.
Attachment #819631 - Attachment is obsolete: true
Attachment #820924 - Flags: review+
Rebase only.
Attachment #819634 - Attachment is obsolete: true
Attachment #820927 - Flags: review+
Attached patch part 2.d/2: fix ICC : v2 (obsolete) — Splinter Review
Rebase
Attachment #819636 - Attachment is obsolete: true
Attachment #820929 - Flags: review+
Rebase
Attachment #819637 - Attachment is obsolete: true
Attachment #820930 - Flags: review+
Attached patch part 2.f/2: fix Voicemail : v2 (obsolete) — Splinter Review
Rebase
Attachment #819639 - Attachment is obsolete: true
Attachment #820931 - Flags: review+
Address comment 40, comment 58 and rebase.
Attachment #819644 - Attachment is obsolete: true
Attachment #819644 - Flags: review?(vchang)
Attachment #820934 - Flags: review?(vchang)
Will update commit summary r= for all patches before landing.
No longer depends on: 928112
Duplicate of this bug: 928112
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #55)
> (In reply to Dave Hylands [:dhylands] from comment #54)
> > ril enabled desktop builds are also used to build contact databases for
> > reference workloads.
> 
> Bug 806598 attachment 709105 [details] [diff] [review] Looks like it can
> be converted to a chrome Marionette test case, or even better, use content
> Marionette and don't try to use internal APIs. Hmm...

Will work-out an replacement for this before landing as well.
Actually, I use a RIL-enabled B2G desktop build to create the SMS/MMS reference workload databases.
My idea here is we may create a python "test case" that populates SMS database and pull that binary sqlite file out by |self.marionette.qemu._run_adb(...)|.

Just got an strange error -- JavaScript Error: "Permission denied to access object" {file: "jar:file:///system/b2g/omni.ja!/components/MobileMessageDatabaseService.js" line: 1120}
(In reply to Jon Hylands [:jhylands] from comment #71)
> Actually, I use a RIL-enabled B2G desktop build to create the SMS/MMS
> reference workload databases.

You can still do it with previous version of Gecko.  Bug 806598 attachment 709105 [details] [diff] [review] is out-dated and doesn't run at all.
Going to upload another series of rebased patches.  Sorry for the possible spam in advance.
Attachment #820922 - Attachment is obsolete: true
Attachment #821729 - Flags: review+
Attached patch part 2.b/2: fix Telephony : v4 (obsolete) — Splinter Review
Attachment #820924 - Attachment is obsolete: true
Attachment #821731 - Flags: review+
Attachment #820927 - Attachment is obsolete: true
Attachment #821732 - Flags: review+
Attached patch part 2.d/2: fix ICC : v3 (obsolete) — Splinter Review
Attachment #820929 - Attachment is obsolete: true
Attachment #821733 - Flags: review+
Attachment #820930 - Attachment is obsolete: true
Attachment #821734 - Flags: review+
Attachment #819642 - Attachment is obsolete: true
Attachment #821736 - Flags: review+
Attached patch part 2.f/2: fix Voicemail : v3 (obsolete) — Splinter Review
Attachment #820931 - Attachment is obsolete: true
Attachment #821737 - Flags: review+
Attachment #820934 - Attachment is obsolete: true
Attachment #820934 - Flags: review?(vchang)
Attachment #821738 - Flags: review?(vchang)
Attached patch part 2.i/2: fix Bluetooth : v3 (obsolete) — Splinter Review
Attachment #819851 - Attachment is obsolete: true
Attachment #821739 - Flags: review+
Attachment #819784 - Attachment is obsolete: true
Attachment #821740 - Flags: review+
Full try: (with RIL enabled as usual)
https://tbpl.mozilla.org/?tree=Try&rev=8d39f7502e19

Full try: (with RIL disabled, expecting RIL test cases failures due to bug 928860)
https://tbpl.mozilla.org/?tree=Try&rev=9d19e19c2457
Duplicate of this bug: 770698
Comment on attachment 821738 [details] [diff] [review]
part 2.h/2: fix dom/system/gonk/* : v5

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

Looks good for me. 
After discuss with vicamo, there is a plan to clean up host route related stuff in NetworkManager.js. 
So that we can avoid MOZ_B2G_RIL flag in NetworkManager.js.
Attachment #821738 - Flags: review?(vchang) → review+
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #72)
> WIP: add test case for easy SMS reference workload generation
> 
> Just got an strange error -- JavaScript Error: "Permission denied to access
> object" {file:
> "jar:file:///system/b2g/omni.ja!/components/MobileMessageDatabaseService.js"
> line: 1120}

It seems to be a xpconnect object wrapper issue here.  Kyle Huey & Bobby Holley helped a lot on this failure yesterday, and it will take a bit more time to conquer it.  Since the original method is not working right now either, I've filed bug 930839 to address this separately.
Blocks: 930839
Comment on attachment 821166 [details] [diff] [review]
WIP: add test case for easy SMS reference workload generation

Moved to bug 930839.
Attachment #821166 - Attachment is obsolete: true
(Can reland after rebase)
Rebase
Attachment #821729 - Attachment is obsolete: true
Attachment #822705 - Flags: review+
Attachment #821731 - Attachment is obsolete: true
Attachment #822706 - Flags: review+
Attachment #821732 - Attachment is obsolete: true
Attachment #822707 - Flags: review+
dom/dom-config.mk was removed.  Remove path dom/icc/src from LOCAL_INCLUDES in dom/base/moz.build instead.
Attachment #821733 - Attachment is obsolete: true
Attachment #822708 - Flags: review+
Attachment #821734 - Attachment is obsolete: true
Attachment #822709 - Flags: review+
Attachment #821737 - Attachment is obsolete: true
Attachment #822710 - Flags: review+
dom/dom-config.mk was removed.  Correct preprocessor guard in dom/base/moz.build instead.
Attachment #821738 - Attachment is obsolete: true
Attachment #822711 - Flags: review+
Rebased patches ready, waiting b2g-inbound to open again.  Aurora patches are prepared, too.  Verifying.
Rebase after bug 929864
Attachment #822711 - Attachment is obsolete: true
Attachment #823841 - Flags: review+
Rebase after bug 913372
Attachment #821739 - Attachment is obsolete: true
Attachment #823842 - Flags: review+
Going to rebase previous aurora patches and upload.
Attached patch [aurora] part 2.a/2 (obsolete) — Splinter Review
Attached patch [aurora] part 2.b/2 (obsolete) — Splinter Review
Attached patch [aurora] part 2.c/2 (obsolete) — Splinter Review
Attached patch [aurora] part 2.d/2 (obsolete) — Splinter Review
Attached patch [aurora] part 2.e/2 (obsolete) — Splinter Review
Attached patch [aurora] part 2.f/2 (obsolete) — Splinter Review
Attached patch [aurora] part 2.h/2 (obsolete) — Splinter Review
Attached patch [aurora] part 2.i/2 (obsolete) — Splinter Review
Actually, this requires more fixing up for b2g26 than I'm comfortable doing. You'll have to take care of it. Sorry :(

This is the repo you need to rebase on top of and push to:
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/
Flags: needinfo?(vyang)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #120)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #119)
> So uh...B2G v1.2 isn't on Aurora anymore. It's on b2g26. Backed out.
> https://hg.mozilla.org/releases/mozilla-aurora/rev/9535f227ec80

Well, looks like B2G Landing wiki changed [1] the day before.

[1]: https://wiki.mozilla.org/index.php?title=Release_Management%2FB2G_Landing&diff=741590&oldid=741589
Flags: needinfo?(vyang)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #122)
> Well, looks like B2G Landing wiki changed [1] the day before.
> 
> [1]:
> https://wiki.mozilla.org/index.
> php?title=Release_Management%2FB2G_Landing&diff=741590&oldid=741589

Sorry, merge day was Monday, don't know what to tell you :\
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #123)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #122)
> > Well, looks like B2G Landing wiki changed [1] the day before.
> 
> Sorry, merge day was Monday, don't know what to tell you :\

Not at all.  Uplifting patches should be a serious enough action that one/I should have a quick check on B2G Landing page first.
Attachment #825190 - Attachment description: [aurora] part 1/2 → [b2g26_v1_2] part 1/2
Attachment #825192 - Attachment is obsolete: true
Attachment #825195 - Attachment is obsolete: true
Attachment #825196 - Attachment is obsolete: true
Attachment #825197 - Attachment is obsolete: true
Attachment #825198 - Attachment is obsolete: true
Attached patch [b2g26_v1_2] part 2.f/2 (obsolete) — Splinter Review
Attachment #825199 - Attachment is obsolete: true
Attachment #825200 - Attachment description: [aurora] part 2.g/2 → [b2g26_v1_2] part 2.g/2
Attachment #825209 - Attachment is obsolete: true
Attached patch [b2g26_v1_2] part 2.i/2 (obsolete) — Splinter Review
Attachment #825212 - Attachment is obsolete: true
Attachment #825214 - Attachment description: [aurora] part 2.j/2 → [b2g26_v1_2] part 2.j/2
Attached patch [b2g26_v1_2] part 2.i/2 : v2 (obsolete) — Splinter Review
Sync with uncommitted local changes.  Fix Bluetooth build failure.
Attachment #826273 - Attachment is obsolete: true
Oops, picked wrong file.
Attachment #826274 - Attachment is obsolete: true
previous revision accidentally included preferences from m-c and fail mochitest-8, or more precisely dom/tests/mochitest/general/test_interfaces.html.
Attachment #826271 - Attachment is obsolete: true
Thanks for sorting it all out :)
Blocks: 943758
Blocks: 939672
Blocks: 947116
You need to log in before you can comment on or make changes to this bug.