[webkit-reviews] review denied: [Bug 62698] [DOM] Support for battery status event : [Attachment 97255] Delete blank line and comment line (Fix for comment #3, #5

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Kihong Kwon
<kihong.kwon at samsung.com>'s request for review:
Bug 62698: [DOM] Support for battery status event
https://bugs.webkit.org/show_bug.cgi?id=62698

Attachment 97255: Delete blank line and comment line (Fix for comment #3, #5
https://bugs.webkit.org/attachment.cgi?id=97255&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.