[Webkit-unassigned] [Bug 83254] [EFL] Support for Battery Status API on the WebKit-Efl

[bugzilla-daemon at webkit.org]

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





--- Comment #22 from Gyuyoung Kim <gyuyoung.kim at samsung.com>  2012-04-13 07:20:46 PST ---
(From update of attachment 137030)
View in context: https://bugs.webkit.org/attachment.cgi?id=137030&action=review

First of all, I'm sorry for unfixing eflews. It looks Dependencies folder on eflews was not updated. I update Dependencies folder by this patch. I hope eflews supports eukit library in next patch.

By the way, it looks basically logic is reasonable. However, I think this patch has some C coding style still. Please fix them. And also, If you explain how to working this patch, it will help to review this patch.

> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:77
> +    E_DBus_Connection* econ = e_dbus_bus_get(DBUS_BUS_SYSTEM);

Please use fullname for variable. econ -> connection or econnection ?

> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:84
> +    E_Ukit_Get_All_Devices_Return* eUkitGetAllDeviceReturn = static_cast<E_Ukit_Get_All_Devices_Return*>(replyData);

It seems to me eUkitGetAllDeviceReturn name is not meaningful.

> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:100
> +    bool chargingChanged = false;

Hmm, it looks this is C coding style, not C++ style. You need to declare variable at real place. I think you can move this variable declaration to 127 line.

> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:101
> +    bool chargingTimeChanged = false;

ditto.

> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:102
> +    bool dischargingTimeChanged = false;

ditto.

> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:103
> +    bool levelChanged = false;

ditto.

> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:123
> +    BatteryStatus* clientBatteryStatus = client->batteryStatus();

I think you don't need to declare this variable to here. Move above two lines to 126 line.

> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:126
> +        return;

Add an empty line.

> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:158
> +        return;

Please add an empty line.

> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.h:58
> +    const double m_batteryStatusRefreshInterval;

It is good to mention time unit. 10.0 msec or sec?

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