[webkit-reviews] review denied: [Bug 83284] [chromium] Add support for Battery Status API : [Attachment 136056] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied sadrul at chromium.org's request for
review:
Bug 83284: [chromium] Add support for Battery Status API
https://bugs.webkit.org/show_bug.cgi?id=83284

Attachment 136056: Patch
https://bugs.webkit.org/attachment.cgi?id=136056&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=136056&action=review


This looks great.  Let's do one more round of polish and then I think we're set
to land this patch.  Thanks!

> Source/WebKit/chromium/public/WebBatteryStatus.h:39
> +	   , level(0.0) { }

Technically { and } should each be on their own line.

> Source/WebKit/chromium/public/WebWidget.h:46
> +class WebBatteryStatus;

This probably isn't needed anymore, right?

> Source/WebKit/chromium/src/BatteryClientChromium.cpp:44
> +BatteryClientChromium::BatteryClientChromium(WebBatteryStatusClient* client)


BatteryClientChromium -> I might have called this class BatteryClientImpl. 
It's in Chromium's WebKit layer, so it's a pretty safe bet that it's for
Chromium.  :)

> Source/WebKit/chromium/src/BatteryClientChromium.cpp:65
> +	   if (m_batteryStatus.charging != batteryStatus.charging)
> +	      
m_controller->didChangeBatteryStatus(WebCore::eventNames().chargingchangeEvent,
status);
> +	   else if (batteryStatus.charging && m_batteryStatus.chargingTime !=
batteryStatus.chargingTime)
> +	      
m_controller->didChangeBatteryStatus(WebCore::eventNames().chargingtimechangeEv
ent, status);
> +	   else if (!batteryStatus.charging && m_batteryStatus.dischargingTime
!= batteryStatus.dischargingTime)
> +	      
m_controller->didChangeBatteryStatus(WebCore::eventNames().dischargingtimechang
eEvent, status);
> +
> +	   if (m_batteryStatus.level != batteryStatus.level)
> +	      
m_controller->didChangeBatteryStatus(WebCore::eventNames().levelchangeEvent,
status);

As discussed, we should move this into WebCore.

> Source/WebKit/chromium/src/BatteryClientChromium.h:59
> +    WebBatteryStatus m_batteryStatus;

This member should probably move into WebCore::BatteryController (and transform
into the WebCore version of the class.