[Webkit-unassigned] [Bug 62698] [DOM] Support for battery status event

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=62698


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #97255|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #8 from Eric Seidel <eric at webkit.org>  2011-06-15 07:07:15 PST ---
(From update of attachment 97255)
View in context: https://bugs.webkit.org/attachment.cgi?id=97255&action=review

It looks like you based this on the location services stuff.  I'm not sure if that's the best model or not for this design.

We need tests for this.  Even if you use a Mock like Location services does.

> Source/WebCore/ChangeLog:14
> +        No new tests. (OOPS!)

This will prevent using the commit-queue.  Normally this line is replaced by a list of tests or reasons why testing is impossible/impractical.

> Source/WebCore/dom/BatteryStatusClient.h:29
> +class BatteryStatusClient {

Why should this be a WebKit client as opposed to a WebCore platform interface?  I'm not sure that it matters, but I'm curious why you made the choice to go up through the WebKit layer instead of down through WebCore platform.

> Source/WebCore/dom/BatteryStatusEvent.cpp:29
> +BatteryStatusEvent::BatteryStatusEvent()

Why do we need an event subclass for this event?  Does it carry any interesting payload?  Many other APIs just send a named event w/o any subclass.  Then clients know to go look at some property on the Window object when they get the event.

> Source/WebCore/dom/BatteryStatusEvent.cpp:30
> +    : Event(eventNames().batterystatusEvent, false, true)

These false/true are meaningless.  Not your fault, but Event should eventually be changed to take enums instead.

> Source/WebCore/dom/BatteryStatusEvent.cpp:31
> +    , m_isPlugged(false), m_level(0)

Normally we do these one per line.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.