[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 #23 from Kihong Kwon <kihong.kwon at samsung.com>  2012-04-13 21:44:08 PST ---
(From update of attachment 137030)
View in context: https://bugs.webkit.org/attachment.cgi?id=137030&action=review

>> 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 ?

OK. Will be fixed.

>> 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.

OK. I will change variable name.

>> 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.

OK, It will be moved.

>> 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.

I don't think so.
IMHO, from set property variable(line 124) to else statemenet(line 132) are one block of operation for get battery state information.
So, I think current position of variables declaration is not bad.
How do you think about this?

>> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:126
>> +        return;
> 
> Add an empty line.

As I said, these statements are one block of operations.
I think empty line doesn't need here.
If you don't agree, please let me know.

>> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:158
>> +        return;
> 
> Please add an empty line.

Ditto.

>> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.h:58
>> +    const double m_batteryStatusRefreshInterval;
> 
> It is good to mention time unit. 10.0 msec or sec?

Opps! That's my test code. I have checked 0.1 to 10 sec to determine this.
I think 1 sec is not bad to report battery status, because battery status usually is not changed rapidly.
But if you have other opinion for this, it can be changed.

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