915604 - Hide RIL DOM APIs behind prefs

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

+++ This bug was initially created as a clone of Bug #914182 +++

From bug 914182 comment 15, we should apply whatever method landed in bug 914182 to other RIL specific APIs like MobileConnection, Icc, MobileMessage, CellBroadcast and Voicemail.

Comment 1

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

However, some of them haven't been moved to WebIDL so we may not be able to apply the technique directly.  See also bug 865403 (Cell Broadcast), bug 859764 (Mobile Message), bug 898445 (Mobile Connection), bug 904588 (Icc).

Comment 2

[Boris Zbarsky [:bzbarsky]]

If they haven't moved to WebIDL, are they even exposed on the global?  I see no props on Window with names from comment 0.

Comment 3

[Masatoshi Kimura [:emk]]

These APIs are hidden behind |#ifdef MOZ_B2G_RIL| atm. However the b2g team are going to reduce compile-time options.

Comment 4

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

(In reply to Masatoshi Kimura [:emk] from comment #3)
> These APIs are hidden behind |#ifdef MOZ_B2G_RIL| atm. However the b2g team
> are going to reduce compile-time options.

No, that's a request from DOM peer, Mounir.  See bug 859616 comment 13:

  The Mozilla project is heading in the exact opposite direction:
  we want to reduce the number of compile-time options because those
  options are a pain to maintain.

Comment 5

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #4)
> (In reply to Masatoshi Kimura [:emk] from comment #3)
> > These APIs are hidden behind |#ifdef MOZ_B2G_RIL| atm. However the b2g team
> > are going to reduce compile-time options.
> 
> No, that's a request from DOM peer, Mounir.  See bug 859616 comment 13:
> 
>   The Mozilla project is heading in the exact opposite direction:
>   we want to reduce the number of compile-time options because those
>   options are a pain to maintain.

So the b2g team are going to do that, no? I don't see how the statement is inaccurate.

Comment 6

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

(In reply to :Ms2ger (back and backlogged from being away) from comment #5)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #4)
> > No, that's a request from DOM peer, Mounir.  See bug 859616 comment 13:
> 
> So the b2g team are going to do that, no? I don't see how the statement is
> inaccurate.

I just wanted to clarify that idea, to reduce compile-time options, was not originated from RIL people.  I do agree the purpose of this bug is absolutely right.

BTW, in bug 920551, Gaia people want "navigator.mozTelephony" completely disappear even from the point of view of pages with "telephony" permission.  The same thing applies to other APIs that rely on hardware modem.

Comment 7

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

Attachment: patch

Comment 8

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

Comment on attachment 811039 [details] [diff] [review]
patch

This patch uses per API preference to determine the availability of each API.  We still depend on MOZ_B2G_RIL flag to hide non-EventTarget symbols.  To remove such dependency, we have to move all interfaces to WebIDL.  For now, only WebTelephony had been fully converted to WebIDL, works to Voicemail and CellBroadcast are partially done, and little progress in Icc, MobileConnection, and SMS.  See bug 898445 for mobileconnection, bug 904588 for icc, bug 859764 for sms, bug 906398 for cellbroadcast...

This patch will serve as a base stone for bug 920551, which will make MOZ_B2G_RIL an optional feature and we might not have RIL related interfaces when building B2G.

Hiding SMS is not in the scope of this bug.  Instead, in bug 859614.  SMS is quite different because it's currently built on every platform and none of the related interfaces has been moved to WebIDL.

Changes:
1) Remove override to "dom.sms.strict7BitEncoding", "dom.sms.requestStatusReport" for they have the same values with those in modules/libpref/src/init/all.js.

2) Move "dom.mms.requestStatusReport", "wap.*" into all.js as well.  Default values not overridden in B2G.

3) Add controlling preferences for CellBroadcast/Icc/MobileConnection/Voicemail.

Full try: https://tbpl.mozilla.org/?tree=Try&rev=94337eb098be

Comment 9

[Masatoshi Kimura [:emk]]

Updating the summary to reflect the reality.

Comment 10

[Masatoshi Kimura [:emk]]

Comment on attachment 811039 [details] [diff] [review]
patch

::: dom/tests/mochitest/general/test_interfaces.html
>+               (entry.pref && !prefs.getBoolPref(entry.pref))) {

I would not like to test the availability based on prefs. See bug 906432 comment #8.

Comment 11

[Masatoshi Kimura [:emk]]

I still think these interfaces should be visible only if the corresponding permission is granted.
I had to introduce a pref for Telephony because SpecialPowers.pushPermissions() chouldn't change the availabliity dynamically. But permissions will not be changed dynamically outside the test harness. It is a bit silly to introduce a pref to avoid the limitation of the test.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> because SpecialPowers.pushPermissions() chouldn't change the availabliity dynamically. 

Uh... it should work.  You just have to create a new iframe after the pushPermissions() call is done.

Comment 13

[Masatoshi Kimura [:emk]]

(In reply to Boris Zbarsky [:bz] from comment #12)
> > because SpecialPowers.pushPermissions() chouldn't change the availabliity dynamically. 
> 
> Uh... it should work.  You just have to create a new iframe after the
> pushPermissions() call is done.

Yeah, I could fix the mochitest using an iframe, but it had no effect on marionette somehow.
Should I file a bug about that?

Comment 14

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

(In reply to Masatoshi Kimura [:emk] from comment #11)
> I still think these interfaces should be visible only if the corresponding
> permission is granted.
> I had to introduce a pref for Telephony because
> SpecialPowers.pushPermissions() chouldn't change the availabliity
> dynamically. But permissions will not be changed dynamically outside the
> test harness. It is a bit silly to introduce a pref to avoid the limitation
> of the test.

I do agree a permission based check makes sense, but yet there is a problem that we want to disable all RIL related interfaces on platforms/devices that do not have RIL functions at all.  This should also apply to apps with required permissions.  So, checking whether the interfaces are built (by prefs) and whether a certain app is granted with access to the interfaces are both necessary here.  In this bug we have the former, and probably with some fixes to Marionette, we have the latter.  What do you think?

Comment 15

[Masatoshi Kimura [:emk]]

Comment on attachment 811039 [details] [diff] [review]
patch

OK, but please add "b2g: true" to make sure that these APIs are disabled on non-b2g. An example:
    {name: "Telephony", b2g: true, pref: "dom.telephony.enabled"},

Comment 16

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

Nominate for koi+ because we'll need this to disable WebIDL interfaces in bug 920551, which is a koi+.

Comment 17

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

Attachment: patch : v2

Address comment 15, add "b2g: true" back.  Re-run Mochitest https://tbpl.mozilla.org/?tree=Try&rev=bf30cbe6a793

Comment 19

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

https://hg.mozilla.org/integration/b2g-inbound/rev/125d25e3662e

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/125d25e3662e

Comment 21

[Andrew Overholt [:overholt]]

Blocks a blocker

Comment 22

[Ed Morley [:emorley]]

[/c/src-gecko/aurora]$ transplant 125d25e3662e
searching for changes
applying 125d25e3662e
patching file b2g/app/b2g.js
Hunk #1 FAILED at 382
Hunk #4 FAILED at 813
2 out of 4 hunks FAILED -- saving rejects to file b2g/app/b2g.js.rej
patching file dom/tests/mochitest/general/test_interfaces.html
Hunk #1 FAILED at 94
Hunk #2 FAILED at 141
Hunk #3 FAILED at 271
Hunk #7 FAILED at 594
Hunk #8 FAILED at 611
5 out of 8 hunks FAILED -- saving rejects to file dom/tests/mochitest/general/test_interfaces.html.rej
patching file dom/webidl/MozCellBroadcast.webidl
Hunk #1 succeeded at 5 with fuzz 2 (offset 0 lines).
patching file modules/libpref/src/init/all.js
Hunk #3 FAILED at 4448
1 out of 3 hunks FAILED -- saving rejects to file modules/libpref/src/init/all.js.rej
patch failed to apply

Comment 23

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

Attachment: [aurora] patch

Comment 24

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

try before landing: https://tbpl.mozilla.org/?tree=Try&rev=33f6aef13c56

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/60c2427efbca