Closed Bug 738465 Opened 8 years ago Closed 7 years ago

implement device light sensor

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: dougt, Assigned: dougt)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

We need to expose the light sensor to content.
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Attached patch patch v.1 (obsolete) — Splinter Review
Attachment #609383 - Flags: review?(bugs)
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
Attachment #609383 - Flags: review?(bugs) → review+
Attachment #609383 - Flags: review+ → review-
er, I was going to r-
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.
(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...
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.
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 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.
Attached patch patch .2 (obsolete) — Splinter Review
Attachment #609383 - Attachment is obsolete: true
Jonas - responded there.  Per our conversation this afternoon, this is something that can be done as a follow up (if I am wrong).
Attached patch rebasedSplinter Review
Attachment #609999 - Attachment is obsolete: true
Attachment #612653 - Flags: review?(bugs)
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.
Attachment #612653 - Flags: review?(bugs)
Attachment #612653 - Flags: review+
Attachment #612653 - Flags: feedback?(jonas)
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.
Attachment #612653 - Flags: review+
Comment on attachment 612653 [details] [diff] [review]
rebased

blassey for the android change
Attachment #612653 - Flags: review?(blassey.bugs)
Attachment #612653 - Flags: review?(blassey.bugs) → review+
Attachment #612653 - Flags: feedback?(jonas) → review?(jonas)
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.
Attachment #612653 - Flags: review?(jonas) → review+
https://hg.mozilla.org/mozilla-central/rev/bbdf0f8c06a8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Depends on: 754199
Depends on: 1359124
You need to log in before you can comment on or make changes to this bug.