802004 - add support for magnetic field sensor

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Andreas Gal :gal]

See GonkSensor.cpp FIXME comment

Comment 1

[Rui You [:ryou]]

Attachment: Magnetic field sensor support patch

Comment 2

[Rui You [:ryou]]

Attachment: Magnetic field sensor support patch

Comment 3

[Rui You [:ryou]]

Attachment: Magnetic field sensor support patch

Comment 4

[Rui You [:ryou]]

Attachment: Add support for magnetic field sensor

Comment 5

[Rui You [:ryou]]

Comment on attachment 682382 [details] [diff] [review]
Add support for magnetic field sensor

TBPL results: https://tbpl.mozilla.org/?tree=Try&rev=4919fa858271

Comment 6

[Doug Turner (:dougt)]

Ray - We should propose this as another device sensor to the DAP W3C WG.  Did you want to send an email to that list, or should I?

Comment 7

[Rui You [:ryou]]

(In reply to Doug Turner (:dougt) from comment #6)
> Ray - We should propose this as another device sensor to the DAP W3C WG. 
> Did you want to send an email to that list, or should I?

Hi Doug, if it's OK, I would like to start a discussion on the mailing list of DAP.

Comment 8

[Doug Turner (:dougt)]

That's great.

Comment 9

[Doug Turner (:dougt)]

Ray - any feedback so far?

Comment 10

[Rui You [:ryou]]

(In reply to Doug Turner (:dougt) from comment #9)
> Ray - any feedback so far?

Hi Doug,

We are working on a wiki page to prepare for the discussion on proposal: https://wiki.mozilla.org/Magnetic_Field_Events

Any advice?

Comment 11

[Doug Turner (:dougt)]

clearing blocking-kilimanjaro flag,  I have no idea what it means.

Ray You, great!   Do you want to send an email to the dap wg regarding this API?  It is very fitting with the rest of the Device APIs.  You can send mail to: [e-mail].

Comment 12

[Rui You [:ryou]]

(In reply to Doug Turner (:dougt) from comment #11)
> clearing blocking-kilimanjaro flag,  I have no idea what it means.
> 
> Ray You, great!   Do you want to send an email to the dap wg regarding this
> API?  It is very fitting with the rest of the Device APIs.  You can send
> mail to: [e-mail].

I have sent the email. Thanks.

http://lists.w3.org/Archives/Public/public-device-apis/2012Dec/0072.html

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 682382 [details] [diff] [review]
Add support for magnetic field sensor

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

i am passing the review here -- sorry that I have sat on this, i was waiting for w3 feedback.  I think we probably can go ahead an impl.

josh - would you mind reviewing here?

Comment 14

[Josh Matthews [:jdm]]

I'm not familiar with most of the code being changed here. I don't think I'm a good reviewer.

Comment 15

[Doug Turner (:dougt)]

ugh.  I'll look at this very soon (by Tuesday evening)

Comment 16

[Doug Turner (:dougt)]

Comment on attachment 682382 [details] [diff] [review]
Add support for magnetic field sensor

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

At a high level, this looks great.  Please create a follow up bug for android support.

I am asking smaug to review the event changes.  I am thinking we probably want to add this as a webidl, but not sure.

::: dom/interfaces/events/nsIDOMDeviceMagneticFieldEvent.idl
@@ +13,5 @@
> +                                    in double x,
> +                                    in double y,
> +                                    in double z);
> +
> +  readonly attribute double x;

These are all in micro Tesla units, right?  probably want to say that here.

::: dom/system/nsDeviceSensors.h
@@ +74,5 @@
> +                                 double x,
> +                                 double y,
> +                                 double z);
> +
> +

extra new line.

::: widget/android/nsAppShell.cpp
@@ +381,2 @@
>              values.AppendElement(curEvent->X());
>              values.AppendElement(curEvent->Y()); 

Please kill this trailing white space since you're here.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 682382 [details] [diff] [review]
Add support for magnetic field sensor

update uuid of nsIDOMWindow.

The patch to make event codegen to handle both .idl and .webidl is still waiting for a review, so no need to add .webidl now. (I'll do it later)

You don't need to add anything to nsIDOMEvent. GeneratedEvents.h will have
NS_NewDOMDeviceMagneticFieldEvent method.

Parameter names in C++ should be in form aParamName

Comment 18

[Rui You [:ryou]]

Attachment: Add support for magnetic field sensor

Comment 19

[Rui You [:ryou]]

Comment on attachment 754699 [details] [diff] [review]
Add support for magnetic field sensor

Sorry for bothering. I uploaded the wrong patch. New patch will be updated later.

Comment 20

[Rui You [:ryou]]

I have created bug 876599, the follow up bug for android support.

Comment 21

[Rui You [:ryou]]

Attachment: Add support for magnetic field sensor

@dougt
1. Yes, all values are in micro Tesla units. Document added in nsIDOMDeviceMagneticFieldEvent.idl
2. Extra new line removed.
3. Trailing white space killed.

@smaug

(In reply to Olli Pettay [:smaug] from comment #17)
> Comment on attachment 682382 [details] [diff] [review]
> Add support for magnetic field sensor
> 
> update uuid of nsIDOMWindow.

Done.

> 
> The patch to make event codegen to handle both .idl and .webidl is still
> waiting for a review, so no need to add .webidl now. (I'll do it later)
> 
> You don't need to add anything to nsIDOMEvent. GeneratedEvents.h will have
> NS_NewDOMDeviceMagneticFieldEvent method.

Remove NS_NewDOMDeviceMagneticFieldEvent in nsIDOMEvent.idl

> 
> Parameter names in C++ should be in form aParamName

Change the parameter names of initDeviceMagneticFieldEvent in nsIDOMDeviceMagneticFieldEvent.idl

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 755165 [details] [diff] [review]
Add support for magnetic field sensor

>+nsDeviceSensors::FireDOMMagneticFieldEvent(mozilla::dom::EventTarget* aTarget,
>+                                           double x,
>+                                           double y,
>+                                           double z)
aX, aY, aZ


Doug, do we have a spec for magnetic field?

Comment 23

[Rui You [:ryou]]

Attachment: Add support for magnetic field sensor

(In reply to Olli Pettay [:smaug] from comment #22)
> Comment on attachment 755165 [details] [diff] [review]
> Add support for magnetic field sensor
> 
> >+nsDeviceSensors::FireDOMMagneticFieldEvent(mozilla::dom::EventTarget* aTarget,
> >+                                           double x,
> >+                                           double y,
> >+                                           double z)
> aX, aY, aZ
> 

Done.

> 
> Doug, do we have a spec for magnetic field?

Comment 24

[BMO Automation]

Firefox OS is not being worked on