I found this from Bug 939052, now MOZ_NFC is enabled by default when building B2G, even on the platform NFC isn't supported, i.e. emulator, Unagi, .... Should we only enable MOZ_NFC only for some specific device, like Nexus-4 ? Or at least we should enable MOZ_NFC for Nexus-4 *for now*, so other devs won't suffer from a feature which is not supported on his platform.

ni? for gps, Hi gps, can you kindly provide some suggestion here? Thank you :)

Well what are we doing with RIL?

How far does .tmp-config --> .config go at the top B2G.git/config.sh file? One such flag is "MAKE_FLAGS=j10" as in 10 jobs.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2) > Well what are we doing with RIL? In gonk-misc/Android.mk [1], we have: export CONFIGURE_ARGS="$(GECKO_CONFIGURE_ARGS)" && \ So basically set GECKO_CONFIGURE_ARGS in any Android.mk or, more preferable, per device BoardConfig.mk will do the job. Besides, considering that NFC is not supported on most of the Firefox OS devices by now, maybe that should be disabled by default in gecko/configure.in. However, there is also opinions about elimination of these configure arguments, and a unified Gecko binary for all Firefox OS devices. [1]: https://github.com/mozilla-b2g/gonk-misc/blob/master/Android.mk#L281

MOZ_NFC requires --enable-nfc. You'll need to set up your mozconfigs (or whatever your build system uses) to only pass --enable-nfc if the build you are producing wants NFC support.

The flag --enable-nfc used to be in gonk-misc/default-gecko-config. We'd have to fork it though.

request for 1.3+ for now all B2G devices (emulator, unagi, ...) will enable NFC by default. We need to turn it on only when NFC is supported by platform.

(In reply to Garner Lee from comment #6) > The flag --enable-nfc used to be in gonk-misc/default-gecko-config. We'd > have to fork it though. You don't. See comment 4.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #9) > You don't. See comment 4. Ah. Something like this? BoardConfig.mk: TARGET_GLOBAL_CFLAGS += -mfpu=neon -mfloat-abi=softfp -DMOZ_NFC=1

(In reply to Garner Lee from comment #10) > Ah. Something like this? > BoardConfig.mk: > TARGET_GLOBAL_CFLAGS += -mfpu=neon -mfloat-abi=softfp -DMOZ_NFC=1 No. Add following line into BoardConfig.mk: GECKO_CONFIGURE_ARGS := --enable-nfc

device/lge/mako repo, add GECKO_CONFIGURE_ARGS to BoardConfig.mk.

Comment on attachment 8334981 [details] [diff] [review] gecko/configure.in: Remove MOZ_NFC=1 from android 15, 17, 18 Although it looks reasonable, but I'm not one of the Build peers. This patch doesn't contain "--enable-nfc in device specific BoardConfig.mk file" part, so I would suggest you to remove it.

Comment on attachment 8334985 [details] [diff] [review] Mako specific --enable-nfc flag setting. I don't know where to apply this patch, and mwu owns Gonk bits.

Comment on attachment 8334981 [details] [diff] [review] gecko/configure.in: Remove MOZ_NFC=1 from android 15, 17, 18 We do not support per device geckos.

(In reply to Michael Wu [:mwu] from comment #16) > We do not support per device geckos. Then we are not supposed to have GECKO_CONFIGURE_ARGS in gonk-misc/Android.mk, isn't it? Please give alternatives to distinguish "platform has no NFC" & "platform has NFC but dead now".

I'll throw some ideas out for discussion, if it hasn't already been had many times over: On IRC, one suggestion was: Exponential backoff, was a max retry or time limit, so we don't end up with a bunch of events in the log trying to connect with a non-existent NFC daemon. The check isn't CPU intensive at all, but the timer does wake the device CPU(s) up. Or as Kyle asked, what does RIL do? For example, a future FirefoxOS tablet won't necessarily have have a cell radio. Some kind of configurable system variable that isn't in gecko indicating presence of NFC (one gecko build config to rule them all, except for really platform specific stuff like cameras maybe).

Yeah this is exactly the same problem as RIL on tablets without 3G/data. We should find solutions for both. I think we should actually do both backoff with max retry plus some sort of system property. Trying forever to reconnect is only going to burn power unnecessarily, so it seems like a good idea to limit reconnection attempts regardless. However, it's also good to be explicit about whether one should expect RIL or NFC. For reference, it looks like android maintains a list of features in /system/etc/permissions . We don't use that, so the next closest thing is probably system properties.

(In reply to Michael Wu [:mwu] from comment #16) > Comment on attachment 8334981 [details] [diff] [review] > gecko/configure.in: Remove MOZ_NFC=1 from android 15, 17, 18 > > We do not support per device geckos. 1. As I know that some works in Gecko will refer to preference for doing different jobs. ex: low memory killer attributes, cell broadcast, mute during voice call from Audio HAL or RIL (planning) 2. Different Gonk-Versions also made gecko can't be compatible to different devices. (So basically the current Gecko already be a device based object.) 3. Also consider to more low memory and storage device, we need to configure each system attribute and remove unused modules for reducing spaces. 4. Expect more device types will join FxOS (ex: TV, setup box, POS), I think Gecko should be customized for each type by different configuration and modules combination. I suggest Gecko can be customized then it can't be compatible to each devices.

The thing I don't really understand is, why Mozilla disallows per device Geckos? Neither does Mozilla manufacture any device, nor deliver updates to any one of them. Why different Gecko binaries is ever a concern to Mozilla? As a device maker, can't I re-compile Gecko with RealView if I want to? Debian has IceWeasel. Does it somehow breaks per-distribution-Geckos' rule?

Any decisions on what to do yet given 1.3 cutoff is/was approaching?

Dimi, please keep handling this. Thanks.

Comment on attachment 8340985 [details] [diff] [review] [WIP]Gecko: Propose solution for runtime enable/disable NFC Attachment is proposed solution if we are going to runtime enable/disable NFC 1. Use system property "ro.nfc.enabled" to check if nfc should be enabled or not. Configured in device/lge/mako/init.mako.rc 2. Check system property for mozNfc webidl 3. Check system property for Nfc.js & NfcContentHelper.js (not include in the patch)

Some comments on WIP: SettingsUI currently checks for the existance of navigator.mozNfc to decide to show NFC or not. Based on the "Per device Gecko configure options" discussion posted on mozilla-dev-b2g, should the mozNfc object get created at all (gecko/dom/nfc/nsNfc.js)? If not, we need a chrome side check (somehow) for "ro.nfc.enabled" there as well to not return an object. If it still needs to be there, the DOM functions need to be no-ops.

Comment on attachment 8340988 [details] [diff] [review] [WIP]device: Propose solution for runtime enable/disable NFC Review of attachment 8340988 [details] [diff] [review]: ----------------------------------------------------------------- The recommended way of adding properties is to add them to the PRODUCT_PROPERTY_OVERRIDES in device files like https://github.com/mozilla-b2g/device-mako/blob/b2g-4.3_r2.1/device.mk . Some devices also have a system.prop file you can edit, which is even better.

Comment on attachment 8340985 [details] [diff] [review] [WIP]Gecko: Propose solution for runtime enable/disable NFC Review of attachment 8340985 [details] [diff] [review]: ----------------------------------------------------------------- Seems reasonable though I don't know this code very well. However, you might have to pay attention to making sure that the check for nfc is done on the parent process side - I don't think we're allowed to rely on property_get in the child process. ::: dom/base/Navigator.cpp @@ +1807,5 @@ > { > nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(aGlobal); > + > + char value[PROPERTY_VALUE_MAX]; > + property_get("ro.nfc.enabled", value, NULL); "0" might be a better default value than NULL.

Comment on attachment 8340985 [details] [diff] [review] [WIP]Gecko: Propose solution for runtime enable/disable NFC Review of attachment 8340985 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Navigator.cpp @@ +1807,5 @@ > { > nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(aGlobal); > + > + char value[PROPERTY_VALUE_MAX]; > + property_get("ro.nfc.enabled", value, NULL); If this is a new Mozilla-only property, it should be "ro.moz.nfc.enabled" just like all other "ro.moz.ril.*".

Comment on attachment 8340985 [details] [diff] [review] [WIP]Gecko: Propose solution for runtime enable/disable NFC Review of attachment 8340985 [details] [diff] [review]: ----------------------------------------------------------------- Should we also need to check is Nfc enable in ipc/nfc/Nfc.cpp? So the socket won't keep reconnecting nfcd all the time.

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #32) > Should we also need to check is Nfc enable in ipc/nfc/Nfc.cpp? > So the socket won't keep reconnecting nfcd all the time. Absolutely.

^-- Maybe add something like "isEnabledOnPlatform()" interface as a pure virtual to UnixSocketConsumer so RIL is forced to get this check too?

(In reply to Fabrice Desré [:fabrice] from comment #33) > (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #32) > > > Should we also need to check is Nfc enable in ipc/nfc/Nfc.cpp? > > So the socket won't keep reconnecting nfcd all the time. > > Absolutely. Just discussed with Dimi, if Nfc.js won't call SystemWorkerManager.registerNfcWorker, then the socket won't be established.

There are two part to fix this issue: 1. Show/Hide mozNfc webidl 2. Enable/Disable NFC worker Create bug 947100 for Enable/Disable NFC worker

Comment on attachment 8344547 [details] [diff] [review] [WIP]Gecko: Propose solution for runtime enable/disable NFC v2 This patch is used to show/hide mozNfc at runtime. The idea is send a dummy message to NFC module. If NFC module exist then the return value of SendSyncMessage will be a non-zero array. NFC module enable/disable is implement in bug 947100 Could you help provide some suggestions ? Thanks !

Comment on attachment 8345215 [details] [diff] [review] Enable/Disable mozNfc at runtime This patch is used to enable/disable mozNfc at runtime. Previous proposed solution is check system property in HasNfcSupport function and Michael Wu comment about we should not check system property in child process. So this patch uses another way to know NFC is enable or disable in HasNfcSupport. The idea is send a dummy message to NFC module, if NFC module is enabled then return result will be a non-zero array. The patch depends on fixed in bug 947100.

Comment on attachment 8345215 [details] [diff] [review] Enable/Disable mozNfc at runtime >+ // If NFC module exist then the return value will be a non-zero array. s/exist/exists/. I assume you mean "nonempty array", not "non-zero array", right? >+ nsCOMPtr<nsISyncMessageSender> cpmm = do_GetService("@mozilla.org/childprocessmessagemanager;1"); Will this return null in the chrome process? Do we want to claim that nfc is unsupported there? >+ if (!cpmm) >+ return false; Braces around if bodies, please. This comes up several times in this patch. >+ JS::Value aRetval; It's not an argument, so no "a" prefix, please. Also, this needs to be a JS::Rooted<JS::Value>. >+ nsIPrincipal* principal = sop ? principal = sop->GetPrincipal() : NULL; sop can never be null here, so no need to null-check. >+ if (NS_OK != cpmm->SendSyncMessage(NS_LITERAL_STRING("NFC:Dummy"), JSVAL_NULL, >+ JSVAL_NULL, principal, aCx, 0, &aRetval)) { This is the part that definitely gets the r-. If you look at SendSyncMessage, it will throw exceptions on aCx in many cases in which it returns non-ok values. But your code here will just leave the exception on aCx and return to the caller without telling it an exception was thrown, which will put the JSContext in a bad state. The _next_ code that tries to run on it will get a mysterious exception randomly. You need to either report or clear the exception if there is one. As far as nits, JS::NullValue(), and fix the indent please. And you probably want NS_FAILED instead of direct compares to NS_OK. >+ JS::Rooted<JSObject*> obj(aCx, JSVAL_TO_OBJECT(aRetval)); What if the value is not an object? I think you probably want something more like: if (!retval.isObject()) { return false; } JS::Rooted<JSObject*>(aCx, &retval.toObject()); >+ if (!JS_IsArrayObject(aCx, obj) || !JS_GetArrayLength(aCx, obj, &length)) JS_GetArrayLength can throw; that's what a false return from it means. See comments about unhandled exceptions on aCx above. I can't speak to the Nfc.js changes, and in particular whether they would in fact return a nonempty array. Should I be scared that we're not doing a permission check for this property right now, even though we have a function to do so? :(

This is defect for NFC feature. Should be fixed in 1.3.

(In reply to Boris Zbarsky [:bz] from comment #43) > Comment on attachment 8345215 [details] [diff] [review] > Enable/Disable mozNfc at runtime > > >+ // If NFC module exist then the return value will be a non-zero array. > > s/exist/exists/. > > I assume you mean "nonempty array", not "non-zero array", right? > > >+ nsCOMPtr<nsISyncMessageSender> cpmm = do_GetService("@mozilla.org/childprocessmessagemanager;1"); > > Will this return null in the chrome process? Do we want to claim that nfc > is unsupported there? > > >+ if (!cpmm) > >+ return false; > > Braces around if bodies, please. This comes up several times in this patch. > > >+ JS::Value aRetval; > > It's not an argument, so no "a" prefix, please. Also, this needs to be a > JS::Rooted<JS::Value>. > > >+ nsIPrincipal* principal = sop ? principal = sop->GetPrincipal() : NULL; > > sop can never be null here, so no need to null-check. > > >+ if (NS_OK != cpmm->SendSyncMessage(NS_LITERAL_STRING("NFC:Dummy"), JSVAL_NULL, > >+ JSVAL_NULL, principal, aCx, 0, &aRetval)) { > > This is the part that definitely gets the r-. > > If you look at SendSyncMessage, it will throw exceptions on aCx in many > cases in which it returns non-ok values. But your code here will just leave > the exception on aCx and return to the caller without telling it an > exception was thrown, which will put the JSContext in a bad state. The > _next_ code that tries to run on it will get a mysterious exception randomly. > > You need to either report or clear the exception if there is one. > > As far as nits, JS::NullValue(), and fix the indent please. And you > probably want NS_FAILED instead of direct compares to NS_OK. > > >+ JS::Rooted<JSObject*> obj(aCx, JSVAL_TO_OBJECT(aRetval)); > > What if the value is not an object? I think you probably want something > more like: > > if (!retval.isObject()) { > return false; > } > JS::Rooted<JSObject*>(aCx, &retval.toObject()); > > >+ if (!JS_IsArrayObject(aCx, obj) || !JS_GetArrayLength(aCx, obj, &length)) > > JS_GetArrayLength can throw; that's what a false return from it means. See > comments about unhandled exceptions on aCx above. > > I can't speak to the Nfc.js changes, and in particular whether they would in > fact return a nonempty array. > > Should I be scared that we're not doing a permission check for this property > right now, even though we have a function to do so? :( About the permission check there is another bug working on it - bug 946047. Thanks for your comment. Vicamo suggested another approach which is much more straightforward so i will update another patch to review.

Comment on attachment 8345721 [details] [diff] [review] Enable/Disable mozNfc at runtime v2 Based on bug 947100, if platform does not support NFC, NFC content helper will not be created. So we can use do_GetService to know if NFC is supported.

Comment on attachment 8345721 [details] [diff] [review] Enable/Disable mozNfc at runtime v2 r=me

if the device does not include NFC hardware, how in Gaia we will detect it? the mozNfc object will not present in navigator? or should we try to get an error some place on the API calls?

(In reply to Jose M. Cantera from comment #49) > if the device does not include NFC hardware, how in Gaia we will detect it? > the mozNfc object will not present in navigator? mozNfc being missing simply means there's no permission declared for "nfc", checked by the MozNfc.webidl HasnfcSupport (implemented in Navigator.cpp). I believe Bug 947100 is intended to directly answer the remaining question: > > or should we try to get an error some place on the API calls?

Is there something preventing this patch from landing? If not and you have no commit rights, please use checkin-neeeded.

(In reply to Fabrice Desré [:fabrice] from comment #51) > Is there something preventing this patch from landing? If not and you have > no commit rights, please use checkin-neeeded. I was waiting for bug 949370 before and it is clear now.

It also depends on bug 946047.

status-b2g-v1.3t fixed. remove [tarako]