Masatoshi Kimura [:emk] Assignee
	Description • 11 years ago

I don't think we can ship this yet.

	Masatoshi Kimura [:emk] Assignee
	Comment 1 • 11 years ago

Vicamo, please review Telephony bits.
Kyle, please review the interface change.

Green except a known failure:
https://tbpl.mozilla.org/?tree=Try&rev=14ed6cb1d3d7

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 2 • 11 years ago

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

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

Thank you Masatoshi.

r=me

	Vicamo Yang [:vicamo][:vyang]
	Comment 3 • 11 years ago

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

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

Thank you.

	Masatoshi Kimura [:emk] Assignee
	Comment 4 • 11 years ago

I found a better function to determine the availability of Telephony API. We don't have to add a pref to complicate things further. Also this patch will reduce the use of the compile-time option.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=4bb73289d110

	Vicamo Yang [:vicamo][:vyang]
	Comment 5 • 11 years ago

Comment on attachment 803084 [details] [diff] [review]
patch v2

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

Today I have a problem to deal with.  In B2G tablet build, we runs master Gaia and master Gecko.  Gaia expects |navigator.mozTelephony == undefined| on tablet builds and Gaia dialer app certainly has "telephony" permission.  This patch simply overthrows one possible solution to this problem.  See bug 911684.

Besides, why is the existence of a certain navigator attribute controlled by the app?  Should we treat it a bug that when an app happens to be able to claim the permission to a certain API set which we want to explicitly disable on a platform/build, and navigator does allow it?  I know the pre-condition itself is a serious security bug, but will its result be a bug, too?

	Masatoshi Kimura [:emk] Assignee
	Comment 6 • 11 years ago

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #5)
> Today I have a problem to deal with.  In B2G tablet build, we runs master
> Gaia and master Gecko.  Gaia expects |navigator.mozTelephony == undefined|
> on tablet builds and Gaia dialer app certainly has "telephony" permission. 
> This patch simply overthrows one possible solution to this problem.  See bug
> 911684.

Actually this patch will help to fix bug 911684. You can change Navigator::HasTelephonySupport function whatever you like (for example, always return false on desktop and tablet). This is much more flexible than a sigle pref.

> Besides, why is the existence of a certain navigator attribute controlled by
> the app?

This patch doesn't change the behavior of a navigator attribute. This patch will just change the existence of global objects (Telephony, TelephonyCall, TelephonyCallGroup, CallEvent). These objects will be present iff navigator.mozTelephony is present. There is no point exposing these global objects when the navigator property is not present.

	Masatoshi Kimura [:emk] Assignee
	Comment 7 • 11 years ago

FYI, I didn't write Navigator::HasTelephonySupport function.
> Besides, why is the existence of a certain navigator attribute controlled by
> the app?
So this behavior was introduced by someone wrote Navigator::HasTelephonySupport function, not me.

	Masatoshi Kimura [:emk] Assignee
	Comment 8 • 11 years ago

Per hg annotate, :bz write the function and :bent and :sicking reviewed.

	Masatoshi Kimura [:emk] Assignee
	Comment 9 • 11 years ago

Changing the subject based on what I really meant to.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 11 years ago

The pre-webidl code exposed the property unconditionally #ifdef MOZ_B2G_RIL.  Attempting to get the property would check the "telephony" permission and if granted return the object.  

The post-webidl code does something similar: the mozTelephony pref on Navigator is #ifdef MOZ_B2G_RIL and even then only defined in globals with the permission.  Basically, I tried to not rock the boat too much.  ;)  The difference was that lack of permission means undefined, not null, after the WebIDL patch.

> Gaia expects |navigator.mozTelephony == undefined| 

The attached patch does not change the behavior of navigator.mozTelephony.

> Should we treat it a bug that when an app happens to be able to claim the permission to
> a certain API set which we want to explicitly disable on a platform/build,

I'm not sure what this is asking...  .mozTelephony is always undefined #ifndef MOZ_B2G_RIL, no matter what HasTelephonySupport() returns.

	Masatoshi Kimura [:emk] Assignee
	Comment 11 • 11 years ago

Comment on attachment 803084 [details] [diff] [review]
patch v2

This patch does not change the behavior of navigator.mozTelephony. If you are not comfortable with the current navigator.mozTelephony, please file a bug about that.
I don't care much about the behavior in apps as long as Telephony API is hidden from regular Web pages. Note that the v1 patch didn't hide the API from Web pages on B2G. It's impossible to distinguish apps from Web pages by using a simple pref. Again, please file a separate bug and change Navigator::HasTelephonySupport as you wish.

	Vicamo Yang [:vicamo][:vyang]
	Comment 12 • 11 years ago

Comment on attachment 803084 [details] [diff] [review]
patch v2

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

::: dom/base/Navigator.h
@@ +87,2 @@
>  class CellBroadcast;
>  #endif

This might need a rebase on bug 910568, which is still in b2g-inbound though.

	Vicamo Yang [:vicamo][:vyang]
	Comment 13 • 11 years ago

Hi Masatoshi, could you also confirm we need to apply the same thing to other permission-required DOM APIs like MobileConnection, Voicemail, MobileMessage, CellBroadcast, etc., as well?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 14 • 11 years ago

(In reply to Masatoshi Kimura [:emk] from comment #8)
> Per hg annotate, :bz write the function and :bent and :sicking reviewed.

I'm not sure what you're asking for exactly, but since this is all behind an #ifdef I don't see us gaining much from this change. It doesn't seem like it will hurt either, though.

	Masatoshi Kimura [:emk] Assignee
	Comment 15 • 11 years ago

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #13)
> Hi Masatoshi, could you also confirm we need to apply the same thing to
> other permission-required DOM APIs like MobileConnection, Voicemail,
> MobileMessage, CellBroadcast, etc., as well?

Yes, but I'll do it in a follow-up bug because the priority is lower than this bug. Unlike Telephony, global objects for those APIs are (unfortunately) already exposed on release versions of Firefox.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 16 • 11 years ago

Comment on attachment 803084 [details] [diff] [review]
patch v2

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

r=me

Thanks Masatoshi.

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 11 years ago

> but since this is all behind an #ifdef

The existence of window.Telephony/TelephonyCall/TelephonyCallGroup is not behind an ifdef.  It's just there in desktop Firefox.

	Masatoshi Kimura [:emk] Assignee
	Comment 18 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/492fdd7c06fd

	Masatoshi Kimura [:emk] Assignee
	Comment 19 • 11 years ago

Reverted unrelated changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b84bbad669d

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 21 • 11 years ago

https://tbpl.mozilla.org/php/getParsedLog.php?id=27793283&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=27793063&tree=Mozilla-Inbound

These were failures on the original commit. Were they fixed in the followup, or do we need to back it all out?

	Masatoshi Kimura [:emk] Assignee
	Comment 22 • 11 years ago

Need a backout for the marionette failure. (I thought I ran marionette on try, but needed a different token from desktop.)

	Masatoshi Kimura [:emk] Assignee
	Comment 23 • 11 years ago

But not sure why mochitest failed... (need clobber?)

	Masatoshi Kimura [:emk] Assignee
	Comment 24 • 11 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 25 • 11 years ago

Backed out in 
https://hg.mozilla.org/integration/mozilla-inbound/rev/68d2ab5621f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/87300a32ff77

	Masatoshi Kimura [:emk] Assignee
	Comment 26 • 11 years ago

Looks like the patch was broken by something in the following range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5417e5da2ceb&tochange=d5fc994ca2ed

	Masatoshi Kimura [:emk] Assignee
	Comment 27 • 11 years ago

Mochtest-8 starts failing since https://hg.mozilla.org/mozilla-central/rev/564f0718eefe (bug 912612).
Marionette starts failing since https://hg.mozilla.org/mozilla-central/rev/dbf7b8418bd2 (bug 909638).

	Masatoshi Kimura [:emk] Assignee
	Comment 28 • 11 years ago

I was unable to make marionette test work.
I gave up and landed the v1 patch for now.
https://hg.mozilla.org/integration/mozilla-inbound/rev/014958e3c8c9

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/014958e3c8c9

	Vicamo Yang [:vicamo][:vyang]
	Comment 30 • 11 years ago

nominate for koi+ for it blocks koi+ bug 914182.

	Vicamo Yang [:vicamo][:vyang]
	Comment 31 • 11 years ago

correct: bug 915604

	Vicamo Yang [:vicamo][:vyang]
	Comment 32 • 11 years ago

Aready in mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/014958e3c8c9