[webkit-reviews] review denied: [Bug 83284] [chromium] Add support for Battery Status API : [Attachment 135836] 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 135836: Patch
https://bugs.webkit.org/attachment.cgi?id=135836&action=review

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


> Source/WebKit/chromium/public/WebBatteryStatus.h:60
> +    bool charging;
> +    double chargingTime;
> +    double dischargingTime;
> +    double level;

I would have expected this class to be a wrapper around WebCore::BatteryStatus,
like WebNode is a wrapper around WebCore::Node.

> Source/WebKit/chromium/public/WebViewClient.h:328
> +    // Battery Status ------------------------------------------------------

> +
> +    // Notifies the client to start or stop sending battery status updates.
> +    virtual void startUpdatingBatteryStatus() { }
> +    virtual void stopUpdatingBatteryStatus() { }

Should these functions be on a WebBatteryStatusClient? (Compare with how
Geolocation works.)  The battery status isn't really a concern of the WebView. 
The WebView is just the context object.

> Source/WebKit/chromium/public/WebWidget.h:212
> +    virtual void handleBatteryStatusUpdate(const WebBatteryStatus&) { }

Why on the WebWidget?  I would have expected this function to be on the
WebView.

Also, why not call it updateBatteryStatus?  "Update" is a much stronger verb
than "handle".

> Source/WebKit/chromium/src/BatteryClientChromium.cpp:52
> +void BatteryClientChromium::setBatteryStatus(const WebBatteryStatus&
webstatus)

webstatus -> webStatus ?

> Source/WebKit/chromium/src/BatteryClientChromium.cpp:58
> +	   RefPtr<WebCore::BatteryStatus> status =
WebCore::BatteryStatus::create(webstatus.charging, webstatus.chargingTime,
webstatus.dischargingTime, webstatus.level);

WebCore::BatteryStatus <-- If we're importing the whole WebCore namespace, the
WebCore:: prefix shouldn't be needed.  I think we're supposed to import WebCore
symbols individually though.

> Source/WebKit/chromium/src/BatteryClientChromium.cpp:64
> +	   if (m_batteryStatus.charging != webstatus.charging)
> +	      
m_controller->didChangeBatteryStatus(eventNames().chargingchangeEvent, status);

> +	   else if (webstatus.charging && m_batteryStatus.chargingTime !=
webstatus.chargingTime)
> +	      
m_controller->didChangeBatteryStatus(eventNames().chargingtimechangeEvent,
status);
> +	   else if (!webstatus.charging && m_batteryStatus.dischargingTime !=
webstatus.dischargingTime)
> +	      
m_controller->didChangeBatteryStatus(eventNames().dischargingtimechangeEvent,
status);

This work seems like it should be done by WebCore.  Does the
BatteryStatusController not have an updateBatteryStatus method?

>>> Source/WebKit/chromium/src/BatteryClientChromium.h:57
>>> +	 WebViewClient* m_client;
>> 
>> refptrs or ownptrs here?
> 
> I am not sure about this one. I don't believe this controller owns the
client, and I am unsure if it needs to hold a refptr on the client, since the
WebViewClient owns the BatteryClientChromium.

Normally, BatteryClientChromium would hold a pointer to the WebBatteryClient
directly rather than to the whole WebViewClient.

> Source/WebKit/chromium/src/BatteryClientChromium.h:65
> +#endif // BatteryClientChromium_h

We usually have a blank line above here.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1365
> +void WebViewImpl::handleBatteryStatusUpdate(const WebBatteryStatus& status)
> +{
> +    m_batteryClient->setBatteryStatus(status);
> +}

Why does the name change here?	It seems like we can call both functions
updateBatteryStatus.