Re: Review of the Battery Status Event specification

Hi,

Thanks for your thorough review! Comments inline.

On 25.5.2011, at 15.45, ext Robin Berjon wrote:

> Hi all,
> 
> we recently published a FPWD of the Battery Status Event, here are some comments on the current draft in the hope of making it progress towards the next steps.
> 
> First and foremost, I looked at existing battery APIs to get a sense for what's straightforward to implement.

[...]

> So the first conclusion I have is that we can kill timeRemaining. It doesn't seem to be available outside of desktop APIs (which are far more complete, including UPS support and all that).

Dropped timeRemaining.

> Also, the isBattery/isCharging distinction is unclear to me, and doesn't seem to map perfectly to what's available in existing APIs. All the use cases I can think of that need these involve minimising power consumption on devices that have a limited amount of power available. Could we replace these two with a simple boolean isPlugged? Is there anything that wouldn't cover?

Replaced isBattery/isCharging with isPlugged.

> # Abstract:
>  • "the battery status of the hosting device and associated auxiliary devices" What are those "associated auxiliary devices"? The first hit on Google is this spec :) I think we should either drop that part of the sentence of clarify it.

Dropped all the auxiliary stuff for now from the ED.

> # BatteryStatusEvent Interface:
>    • The initBatteryStatusEvent() method is copied from the style of other events, but it's unclear that we need all of that. Do we need a type name when there's only one event type? Do we need to state that it bubbles when its target is Window? Do we need to care whether it's cancellable given that it can't bubble and doesn't have a default action? Everyone else seems to be doing this in this exact way though, so I'm guessing we shouldn't worry too much about it.

My suggestion would be to keep it for now.

>    • Assuming we keep it, based on the above, the new IDL would look something like this:
> 
> [NoInterfaceObject]
> interface BatteryStatusEvent : Event {
>    readonly attribute boolean        isPlugged;
>    readonly attribute float?         level;
>    void initBatteryStatusEvent (DOMString type, boolean bubbles, boolean cancelable, boolean isPlugged, float? level);
> };

Done.

>    • "The initBatteryStatusEvent() method must initialize the event in a manner analogous to the similarly-named method in [DOM-LEVEL-3-EVENTS]." That's a bit too fuzzy. It should refer specifically to initEvent() in http://www.w3.org/TR/DOM-Level-3-Events/#events-event-type-initEvent and probably state that the isPlugged and level arguments initialise the attributes with the same names.

Clarified this.

>    • "If a change in the battery status of the hosting device occurs, then the User Agent must dispatch a BatteryStatusEvent event on the Window [HTML5] object." There are a few problems here. There should be a conformance section earlier in the document which defines the user agent as a conformance class, linked from such statements. Then the condition for event triggering is hard to pin down and test. We probably can't make it perfect, but I think that we can improve on it, especially by not making it happen whenever a 0.0000001 change takes place. There is a list of SHOULDs at the end of the section which I think could be merged with this statement here as they clarify it.

I added a conformance section and defined the "User Agent" (copied over from Contacts). If anyone has concrete proposals how to make conditions for event triggering testable, let us know.

>    • "If a change in the battery status of an auxiliary device occurs..." Do we need this notion? It seems to me that this could be added at a later stage, with a clearer view of what an auxiliary device is.

Auxiliary stuff is now gone.

>    • "The onbatterystatus event handler must be supported by Window objects" Do we want to have a piece of WebIDL here?

Flagging as a TODO item.

>    • "If an event listener is registered with the event type batterystatus" I'd say "When an event listener..."

Fixed.

> # The batterystatus Event
>    • We need to provide a more complete description of the event, perhaps similar to one of those: http://www.w3.org/TR/DOM-Level-3-Events/#event-type-load. Something like:
> 
> Type		batterystatus
> Interface	BatteryStatusEvent if generated by the user agent, Event otherwise.
> Sync / Async	Async 
> Bubbles		No
> Target 		defaultView
> Cancelable	No
> Default action	none 
> Context info
> 	• Event.target: defaultView
> 
> I could perhaps add support in ReSpec for the generation of such tables.

I added the table. I'd love to see ReSpec generate and pretti-fy this table for me ;)

>    • "User Agents must dispatch this event type to indicate a change in the battery status." This does not need to be there, the previous section gives enough information (or, conversely, the description in the previous section could go here).

Removed.

> # Examples
>    • These should be marked with "<pre class='example sh_javascript'>".

Done.

> I think that with these changes in place we'll be able to release a new draft. Given the simplicity of the spec, we have to think about LC soon. Thoughts?


The above changes are now in the ED:

  http://dev.w3.org/2009/dap/system-info/battery-status.html

After all the typos and such have been spotted and fixed we're ready for a new draft release.

In addition, once we're done with the items flagged as TODOs we're good to go to LC.

-Anssi

Received on Thursday, 26 May 2011 13:20:34 UTC