Last Comment Bug 738465 - implement device light sensor
: implement device light sensor
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Doug Turner (:dougt)
:
:
Mentors:
Depends on: 754199
Blocks: 842952
  Show dependency treegraph
 
Reported: 2012-03-22 14:41 PDT by Doug Turner (:dougt)
Modified: 2013-02-20 03:03 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v.1 (31.09 KB, patch)
2012-03-26 10:56 PDT, Doug Turner (:dougt)
bugs: review-
Details | Diff | Splinter Review
patch .2 (33.87 KB, patch)
2012-03-27 20:57 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
rebased (34.16 KB, patch)
2012-04-05 12:39 PDT, Doug Turner (:dougt)
bugs: review+
mounir: review+
blassey.bugs: review+
jonas: review+
Details | Diff | Splinter Review

Description Doug Turner (:dougt) 2012-03-22 14:41:41 PDT
We need to expose the light sensor to content.
Comment 1 Doug Turner (:dougt) 2012-03-26 10:56:35 PDT
Created attachment 609383 [details] [diff] [review]
patch v.1
Comment 2 Olli Pettay [:smaug] 2012-03-26 14:41:44 PDT
Comment on attachment 609383 [details] [diff] [review]
patch v.1


>+NS_IMETHODIMP nsDOMDeviceLightEvent::InitDeviceLightEvent(const nsAString & aEventTypeArg,
>+                                                                      bool aCanBubbleArg,
>+                                                                      bool aCancelableArg,
>+                                                                      double aValue)
NS_IMETHODIMP should be in its own line here and elsewhere.
Align parameters.


>+nsresult NS_NewDOMDeviceLightEvent(nsIDOMEvent** aInstancePtrResult,
>+                                         nsPresContext* aPresContext,
>+                                         nsEvent *aEvent) 
>+{
>+  NS_ENSURE_ARG_POINTER(aInstancePtrResult);
>+
>+  nsDOMDeviceLightEvent* it = new nsDOMDeviceLightEvent(aPresContext, aEvent);
>+  if (nsnull == it) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }
No need for this null check.

>+  if (aEventType.LowerCaseEqualsLiteral("devicelightevent"))
>+    return NS_NewDOMDeviceLightEvent(aDOMEvent, aPresContext, nsnull);
If possible, don't add this, but implement event ctor.

>+      } else if (aType == NS_DEVICE_LIGHT) {
>+        nsPIDOMWindow* window = GetInnerWindowForTarget();
>+        if (window)
>+          window->DisableDeviceSensor(SENSOR_LIGHT);
if (expr) {
  stmt;
}

>+[scriptable, uuid(c6f68e93-9eaf-4bba-852b-c9c534a8d079)]
>+interface nsIDOMDeviceLightEvent : nsIDOMEvent
>+{
>+  void initDeviceLightEvent(in DOMString eventTypeArg,
>+			    in boolean canBubbleArg,
>+			    in boolean cancelableArg,
>+			    in double value);
Make init*Event [noscript] and implement event ctor using a dictionary

> void
>+nsDeviceSensors::FireDOMLightEvent(nsIDOMDocument *domdoc,
>+				  nsIDOMEventTarget *target,
>+				  double value)
align parameters. Parameter names should be aName



>+{
>+  nsCOMPtr<nsIDOMEvent> event;
>+  bool defaultActionEnabled = true;
>+  domdoc->CreateEvent(NS_LITERAL_STRING("DeviceLightEvent"), getter_AddRefs(event));
This could just call NS_NewDOMDeviceLightEvent
Comment 3 Olli Pettay [:smaug] 2012-03-26 15:06:49 PDT
er, I was going to r-
Comment 4 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-03-26 15:10:55 PDT
I'd really like to expose the current light value somewhere, as well as a switch to turn on/off the events. I think it's a lot easier for applications to flip a boolean property than to add/remove event listeners any time they want to sample the current background light.
Comment 5 Mounir Lamouri (:mounir) 2012-03-26 15:54:55 PDT
(In reply to Jonas Sicking (:sicking) from comment #4)
> I'd really like to expose the current light value somewhere, as well as a
> switch to turn on/off the events. I think it's a lot easier for applications
> to flip a boolean property than to add/remove event listeners any time they
> want to sample the current background light.

+1. I think we should do something like the Battery API where you can do a direct access to the value. Otherwise, you are going to register for an event and I guess your implementation will fire an event immediately so the web page will know the current value. That looks wrong...
Comment 6 Doug Turner (:dougt) 2012-03-26 16:39:00 PDT
jonas and mounir - exposing a sensor like that is a bad idea as I have mentioned before - it requires that you return undefined before it is called and you have no way of informing the implementation that the sensor can be disabled.  Since your approach is concerned with not just this api, but every device api, lets please take it to the mailing list. even if I am wrong (which I doubt :) ), that work can be done in a follow up.
Comment 7 Mounir Lamouri (:mounir) 2012-03-26 17:34:42 PDT
I will let you reply to my message in dev-webapi but I don't see why you will have to return undefined unless there are some implementations limitations I'm not aware of.
Comment 8 Doug Turner (:dougt) 2012-03-26 18:22:43 PDT
> I will let you reply to my message in dev-webapi but I don't see why you will have to return undefined unless there are some implementations limitations I'm not aware of.

I'll write something up to express why doing this is an extremely bad idea.
Comment 9 Doug Turner (:dougt) 2012-03-27 20:57:26 PDT
Created attachment 609999 [details] [diff] [review]
patch .2
Comment 10 Doug Turner (:dougt) 2012-03-27 21:15:18 PDT
Jonas - responded there.  Per our conversation this afternoon, this is something that can be done as a follow up (if I am wrong).
Comment 11 Doug Turner (:dougt) 2012-04-05 12:39:57 PDT
Created attachment 612653 [details] [diff] [review]
rebased
Comment 12 Olli Pettay [:smaug] 2012-04-10 08:46:42 PDT
Comment on attachment 612653 [details] [diff] [review]
rebased

Someone else needs to review hal/ and android parts.
And I wish Jonas could give feedback for the API.
Comment 13 Mounir Lamouri (:mounir) 2012-04-11 03:07:35 PDT
Comment on attachment 612653 [details] [diff] [review]
rebased

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

r=me for hal changes. I can't review Android backend changes.
Comment 14 Doug Turner (:dougt) 2012-04-15 21:15:03 PDT
Comment on attachment 612653 [details] [diff] [review]
rebased

blassey for the android change
Comment 15 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-05-01 19:54:15 PDT
Comment on attachment 612653 [details] [diff] [review]
rebased

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

We need to document what the unit of .value is. Also, hide the initDeviceLightEvent function from web pages since we have the ctor (as commented in the proximity bug). r=me with that.
Comment 17 Ed Morley [:emorley] 2012-05-04 11:44:18 PDT
https://hg.mozilla.org/mozilla-central/rev/bbdf0f8c06a8

Note You need to log in before you can comment on or make changes to this bug.