[EFL] Support for Battery Status API on the WebKit-Efl

[Kihong Kwon]

Add Battery status API implementation to the efl port.

[Kihong Kwon]

Created attachment 136170 [details]
Patch

I have written this patch with e_dbus library.
If you have any comments about this, please let me know.

[Gyuyoung Kim]

Comment on attachment 136170 [details]
Patch

Attachment 136170 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12367516

[Gyuyoung Kim]

EFL Build Bot needs to install EUKIT for this patch. If this patch is reasonable, I will install it to efl ews and buildbot.

[Gyuyoung Kim]

Comment on attachment 136170 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136170&action=review

> Source/WebKit/efl/ChangeLog:9
> +        (charging, chargingTime, dischargingTime, level)

What it line 9 ? It looks line 9 is duplicated description.

> Source/WebKit/efl/ewk/ewk_battery.cpp:32
> +static Eina_Bool ewk_battery_status_update(void *data);

Move '*' to data type side.

> Source/WebKit/efl/ewk/ewk_battery.cpp:33
> +static void ewk_battery_status_get(void *data, void *reply_data, DBusError *error);

ditto.

> Source/WebKit/efl/ewk/ewk_battery.cpp:34
> +static void ewk_battery_status_client_set(void *data, void *reply_data, DBusError *error);

ditto.

> Source/WebKit/efl/ewk/ewk_battery.cpp:58
> +static Eina_Bool ewk_battery_status_update(void *data)

ditto.

> Source/WebKit/efl/ewk/ewk_battery.cpp:67
> +static void ewk_battery_status_get(void *data, void *reply_data, DBusError *error)

ditto.

> Source/WebKit/efl/ewk/ewk_battery.cpp:82
> +static void ewk_battery_status_client_set(void *data, void *reply_data, DBusError *error)

Do not use _ in local function.

> Source/WebKit/efl/ewk/ewk_battery.cpp:84
> +    E_Ukit_Get_All_Properties_Return* ret = static_cast<E_Ukit_Get_All_Properties_Return*>(reply_data);

Do not use abbreviation. ret -> ?

> Source/WebKit/efl/ewk/ewk_battery_private.h:36
> +void* ewk_battery_update_start(WebCore::BatteryClientEfl*);

It looks you don't add public APIs for batter status. Do you have plan to add public APIs ? If not so, I think that we don't need to add ewk_battery_xxx files.

[Kihong Kwon]

Comment on attachment 136170 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136170&action=review

Thank you for your comments. Gyuyoung~~:)

>> Source/WebKit/efl/ChangeLog:9
>> +        (charging, chargingTime, dischargingTime, level)
> 
> What it line 9 ? It looks line 9 is duplicated description.

I missed it.
It will be fixed.

>> Source/WebKit/efl/ewk/ewk_battery.cpp:32
>> +static Eina_Bool ewk_battery_status_update(void *data);
> 
> Move '*' to data type side.

OK, It will be fixed.

>> Source/WebKit/efl/ewk/ewk_battery.cpp:33
>> +static void ewk_battery_status_get(void *data, void *reply_data, DBusError *error);
> 
> ditto.

OK, It will be fixed.

>> Source/WebKit/efl/ewk/ewk_battery.cpp:34
>> +static void ewk_battery_status_client_set(void *data, void *reply_data, DBusError *error);
> 
> ditto.

OK, It will be fixed.

>> Source/WebKit/efl/ewk/ewk_battery.cpp:58
>> +static Eina_Bool ewk_battery_status_update(void *data)
> 
> ditto.

OK, It will be fixed.

>> Source/WebKit/efl/ewk/ewk_battery.cpp:67
>> +static void ewk_battery_status_get(void *data, void *reply_data, DBusError *error)
> 
> ditto.

OK, It will be fixed.

>> Source/WebKit/efl/ewk/ewk_battery.cpp:82
>> +static void ewk_battery_status_client_set(void *data, void *reply_data, DBusError *error)
> 
> Do not use _ in local function.

OK, It will be fixed.

>> Source/WebKit/efl/ewk/ewk_battery.cpp:84
>> +    E_Ukit_Get_All_Properties_Return* ret = static_cast<E_Ukit_Get_All_Properties_Return*>(reply_data);
> 
> Do not use abbreviation. ret -> ?

OK.

>> Source/WebKit/efl/ewk/ewk_battery_private.h:36
>> +void* ewk_battery_update_start(WebCore::BatteryClientEfl*);
> 
> It looks you don't add public APIs for batter status. Do you have plan to add public APIs ? If not so, I think that we don't need to add ewk_battery_xxx files.

You are right.
I will move these to ewk_private.h

[Kihong Kwon]

Created attachment 136171 [details]
patch.

[Gyuyoung Kim]

(In reply to comment #5)

> >> Source/WebKit/efl/ewk/ewk_battery_private.h:36
> >> +void* ewk_battery_update_start(WebCore::BatteryClientEfl*);
> > 
> > It looks you don't add public APIs for batter status. Do you have plan to add public APIs ? If not so, I think that we don't need to add ewk_battery_xxx files.
> 
> You are right.
> I will move these to ewk_private.h

If you won't add public APIs, why don't you implement functions of ewk_battery.cpp in BatteryClientEfl directly ?

[Gyuyoung Kim]

Comment on attachment 136171 [details]
patch.

Attachment 136171 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12361937

[Kihong Kwon]

(In reply to comment #7)
> (In reply to comment #5)
> 
> > >> Source/WebKit/efl/ewk/ewk_battery_private.h:36
> > >> +void* ewk_battery_update_start(WebCore::BatteryClientEfl*);
> > > 
> > > It looks you don't add public APIs for batter status. Do you have plan to add public APIs ? If not so, I think that we don't need to add ewk_battery_xxx files.
> > 
> > You are right.
> > I will move these to ewk_private.h
> 
> If you won't add public APIs, why don't you implement functions of ewk_battery.cpp in BatteryClientEfl directly ?

You are right.
I will move all functionality of ewk_battery.cpp  to BatteryClientEfl.cpp.
That's better.
Thanks for comments.:)

[Kihong Kwon]

Created attachment 136191 [details]
Patch.

[Gyuyoung Kim]

Comment on attachment 136191 [details]
Patch.

Attachment 136191 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12368085

[Kihong Kwon]

Comment on attachment 136191 [details]
Patch.

This patch needs review first.
And after that, e_dbus library has to be installed in the buildbot before merging this patch.

[Gyuyoung Kim]

(In reply to comment #12)
> (From update of attachment 136191 [details])
> This patch needs review first.
> And after that, e_dbus library has to be installed in the buildbot before merging this patch.

Kihong, I add eukit path to ews. I hope your next patch won't meet red face on efl ews.

[Kihong Kwon]

Created attachment 137030 [details]
Patch.

[Gyuyoung Kim]

Comment on attachment 137030 [details]
Patch.

Attachment 137030 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12392753

[Kihong Kwon]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 136191 [details] [details])
> > This patch needs review first.
> > And after that, e_dbus library has to be installed in the buildbot before merging this patch.
> 
> Kihong, I add eukit path to ews. I hope your next patch won't meet red face on efl ews.

Could you check one more time for adding edbus package(eukit library) to the Efl port please?
Thank you for your support Gyuyoung.

[WebKit Review Bot]

Comment on attachment 137030 [details]
Patch.

Attachment 137030 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12399223

New failing tests:
svg/text/font-size-below-point-five.svg

[WebKit Review Bot]

Created attachment 137065 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dominik Röttsches (drott)]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 136191 [details] [details])
> > This patch needs review first.
> > And after that, e_dbus library has to be installed in the buildbot before merging this patch.
> 
> Kihong, I add eukit path to ews. I hope your next patch won't meet red face on efl ews.

I don't much about this library, but I think we should add this to jhbuild.modules instead of individual bot installations. Could you modify your patch to included this new dependency in jhbuild.modules so that it can be automatically retrieved and built before webkit is built?

[Kihong Kwon]

(In reply to comment #19)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (From update of attachment 136191 [details] [details] [details])
> > > This patch needs review first.
> > > And after that, e_dbus library has to be installed in the buildbot before merging this patch.
> > 
> > Kihong, I add eukit path to ews. I hope your next patch won't meet red face on efl ews.
> 
> I don't much about this library, but I think we should add this to jhbuild.modules instead of individual bot installations. Could you modify your patch to included this new dependency in jhbuild.modules so that it can be automatically retrieved and built before webkit is built?

I already added edbus library to jhbuild.modules in this patch.
I am not sure how build-bot works exactly, but what's shown, I guess, is due to the fact that the build-bot doesn't work with jhbuild.modules all the times.
Thank you for your comments~:-)

[Dominik Röttsches (drott)]

(In reply to comment #20)

> I already added edbus library to jhbuild.modules in this patch.

Alright - sorry, I didn't see that.

> I am not sure how build-bot works exactly, but what's shown, I guess, is due to the fact that the build-bot doesn't work with jhbuild.modules all the times.
> Thank you for your comments~:-)

Yes, good to know. More reason to fix bug 82048 - which should address this.

[Gyuyoung Kim]

Comment on attachment 137030 [details]
Patch.

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?

[Kihong Kwon]

Comment on attachment 137030 [details]
Patch.

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.

[Kihong Kwon]

Created attachment 137198 [details]
Patch.

[Gyuyoung Kim]

Comment on attachment 137198 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=137198&action=review

As far as I know, webkit coding style declares local variable where it is really used.

> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:48
> +    ASSERT(e_dbus_init());

I wonder why do you use ASSERT.

> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:49
> +    ASSERT(e_ukit_init());

ditto.

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

Move dischargingTimeChagned to line 134.

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

ditto.

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

Move levelChanged to line 156.

> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:122
> +    double level = 0;

ditto.

[Kihong Kwon]

Comment on attachment 137198 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=137198&action=review

Thank you very much for your review and support. Gyuyoung~;-)

>> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:48
>> +    ASSERT(e_dbus_init());
> 
> I wonder why do you use ASSERT.

I need to check by assertion to confirm init() is working well.
Because init() is failed, battery status api can't work properly.

>> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:49
>> +    ASSERT(e_ukit_init());
> 
> ditto.

ditto.

>> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:116
>> +    bool chargingTimeChanged = false;
> 
> Move dischargingTimeChagned to line 134.

I think all statements of getting battery status and setting variables are a piece of operation.
Otherwise There is no benefit to divide definition and IMHO, this looks better.:-)
How do you think about this?
If you still think I need to move these statements, I will do.

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

ditto.

>> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:118
>> +    bool levelChanged = false;
> 
> Move levelChanged to line 156.

ditto.

>> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:122
>> +    double level = 0;
> 
> ditto.

ditto.

[Gyuyoung Kim]

Comment on attachment 137198 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=137198&action=review

>>> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:48
>>> +    ASSERT(e_dbus_init());
>> 
>> I wonder why do you use ASSERT.
> 
> I need to check by assertion to confirm init() is working well.
> Because init() is failed, battery status api can't work properly.

ASSERT can work on debug build. Do you only need to check it on debug mode?

>>> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:49
>>> +    ASSERT(e_ukit_init());
>> 
>> ditto.
> 
> ditto.

ditto.

> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:100
> +

Remove empty line here as line 85.

>>> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:116
>>> +    bool chargingTimeChanged = false;
>> 
>> Move dischargingTimeChagned to line 134.
> 
> I think all statements of getting battery status and setting variables are a piece of operation.
> Otherwise There is no benefit to divide definition and IMHO, this looks better.:-)
> How do you think about this?
> If you still think I need to move these statements, I will do.

As far as I know, WebKit tends to declare variable when it is really used. So, I think it is better to follow it.

[Kihong Kwon]

(In reply to comment #27)
> (From update of attachment 137198 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137198&action=review
> 
> >>> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:48
> >>> +    ASSERT(e_dbus_init());
> >> 
> >> I wonder why do you use ASSERT.
> > 
> > I need to check by assertion to confirm init() is working well.
> > Because init() is failed, battery status api can't work properly.
> 
> ASSERT can work on debug build. Do you only need to check it on debug mode?
Yes, every init() call of efl libraries does not check ASSERT.
So, I think there is no needs to check on the release build.
If you want to delete ASSERT, that's no problem to me.

> 
> >>> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:49
> >>> +    ASSERT(e_ukit_init());
> >> 
> >> ditto.
> > 
> > ditto.
> 
> ditto.
> 
> > Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:100
> > +
> 
> Remove empty line here as line 85.
> 
> >>> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:116
> >>> +    bool chargingTimeChanged = false;
> >> 
> >> Move dischargingTimeChagned to line 134.
> > 
> > I think all statements of getting battery status and setting variables are a piece of operation.
> > Otherwise There is no benefit to divide definition and IMHO, this looks better.:-)
> > How do you think about this?
> > If you still think I need to move these statements, I will do.
> 
> As far as I know, WebKit tends to declare variable when it is really used. So, I think it is better to follow it.

OK. I will change their position.

[Gyuyoung Kim]

Comment on attachment 137198 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=137198&action=review

>>>>> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:48
>>>>> +    ASSERT(e_dbus_init());
>>>> 
>>>> I wonder why do you use ASSERT.
>>> 
>>> I need to check by assertion to confirm init() is working well.
>>> Because init() is failed, battery status api can't work properly.
>> 
>> ASSERT can work on debug build. Do you only need to check it on debug mode?
> 
> Yes, every init() call of efl libraries does not check ASSERT.
> So, I think there is no needs to check on the release build.
> If you want to delete ASSERT, that's no problem to me.

e_dbus_init() returns e_dbus init count when it is failed. So, I think you need to check if e_dbus_init is success.

ewk_main.cpp check it as well.
http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_main.cpp#L80

[Kihong Kwon]

(In reply to comment #29)
> (From update of attachment 137198 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137198&action=review
> 
> >>>>> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:48
> >>>>> +    ASSERT(e_dbus_init());
> >>>> 
> >>>> I wonder why do you use ASSERT.
> >>> 
> >>> I need to check by assertion to confirm init() is working well.
> >>> Because init() is failed, battery status api can't work properly.
> >> 
> >> ASSERT can work on debug build. Do you only need to check it on debug mode?
> > 
> > Yes, every init() call of efl libraries does not check ASSERT.
> > So, I think there is no needs to check on the release build.
> > If you want to delete ASSERT, that's no problem to me.
> 
> e_dbus_init() returns e_dbus init count when it is failed. So, I think you need to check if e_dbus_init is success.
> 
> ewk_main.cpp check it as well.
> http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_main.cpp#L80

OK, I will fix it.

[Kihong Kwon]

Created attachment 137663 [details]
Patch.

[Gyuyoung Kim]

Comment on attachment 137663 [details]
Patch.

Attachment 137663 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12426209

[Kihong Kwon]

Created attachment 137665 [details]
Patch.

[Gyuyoung Kim]

Comment on attachment 137665 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=137665&action=review

> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.h:49
> +    static void getBatteryStatus(void* data, void* replyData, DBusError*);

Is it really necessary to set as public ? It looks this functions are only used by BatteryClientEfl internally.

Generally, WebKit doesn't have get prefix. See also : http://www.webkit.org/coding/coding-style.html

[Thiago Marcos P. Santos]

(In reply to comment #34)
> (From update of attachment 137665 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137665&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.h:49
> > +    static void getBatteryStatus(void* data, void* replyData, DBusError*);
> 
> Is it really necessary to set as public ? It looks this functions are only used by BatteryClientEfl internally.
> 
> Generally, WebKit doesn't have get prefix. See also : http://www.webkit.org/coding/coding-style.html

Isn't it the opposite?

http://www.webkit.org/coding/coding-style.html#names-setter-getter

[Gyuyoung Kim]

(In reply to comment #35)
> (In reply to comment #34)
> > (From update of attachment 137665 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=137665&action=review
> > 
> > > Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.h:49
> > > +    static void getBatteryStatus(void* data, void* replyData, DBusError*);
> > 
> > Is it really necessary to set as public ? It looks this functions are only used by BatteryClientEfl internally.
> > 
> > Generally, WebKit doesn't have get prefix. See also : http://www.webkit.org/coding/coding-style.html
> 
> Isn't it the opposite?
> 
> http://www.webkit.org/coding/coding-style.html#names-setter-getter

Oops, I'm sorry. I had a confusion. In this case, get prefix is needed.

[Kihong Kwon]

Comment on attachment 137665 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=137665&action=review

>>>> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.h:49
>>>> +    static void getBatteryStatus(void* data, void* replyData, DBusError*);
>>> 
>>> Is it really necessary to set as public ? It looks this functions are only used by BatteryClientEfl internally.
>>> 
>>> Generally, WebKit doesn't have get prefix. See also : http://www.webkit.org/coding/coding-style.html
>> 
>> Isn't it the opposite?
>> 
>> http://www.webkit.org/coding/coding-style.html#names-setter-getter
> 
> Oops, I'm sorry. I had a confusion. In this case, get prefix is needed.

You are right.
These functions(getBatteryStatus() and setBatteryClient()) can be moved to private.
I will fix it.

[Kihong Kwon]

Created attachment 139596 [details]
Patch.

[Gyuyoung Kim]

Comment on attachment 139596 [details]
Patch.

Attachment 139596 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12593544

[Kihong Kwon]

Created attachment 139788 [details]
Patch.

[Gyuyoung Kim]

Comment on attachment 139788 [details]
Patch.

Attachment 139788 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12588829

[Kihong Kwon]

I think edbus library is not installed properly.
Gyuyoung! Could you check it please?

[Gyuyoung Kim]

(In reply to comment #42)
> I think edbus library is not installed properly.
> Gyuyoung! Could you check it please?

Hmm... I already fix it. In addition, there is no build error in EFL ews when I built webkit with latest patch. Plz try one more time.

[Kihong Kwon]

(In reply to comment #43)
> (In reply to comment #42)
> > I think edbus library is not installed properly.
> > Gyuyoung! Could you check it please?
> 
> Hmm... I already fix it. In addition, there is no build error in EFL ews when I built webkit with latest patch. Plz try one more time.

Thank you for your kind supporting.
I will try it again.

[Kihong Kwon]

Created attachment 139992 [details]
Patch.

[Gyuyoung Kim]

Comment on attachment 139992 [details]
Patch.

Attachment 139992 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12621056

[Gyuyoung Kim]

Comment on attachment 139992 [details]
Patch.

Looks fine now. However, I don't know why efl ews can't recognize eukit library though eukit was installed. I succeed to build this patch on EFL EWS. So, I would like to land this patch. Then, I will check build bot as soon as this patch is landed.

[Adam Barth]

Comment on attachment 139992 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=139992&action=review

> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:62
> +    if (!m_timer.isActive())
> +        return;

Does this mean we never started updating?  Is that an error, or can that occur normally?

[Kihong Kwon]

(In reply to comment #48)
> (From update of attachment 139992 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139992&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.cpp:62
> > +    if (!m_timer.isActive())
> > +        return;
> 
> Does this mean we never started updating?  Is that an error, or can that occur normally?

You are right, this return-statement naver be executed.
I didn't realize that. Thank you.

[Kihong Kwon]

Created attachment 140490 [details]
patch.

[Gyuyoung Kim]

Comment on attachment 140490 [details]
patch.

Attachment 140490 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12633925

[Chang Shu]

Comment on attachment 140490 [details]
patch.

it looks good over all. But fix the efl build first. :)

[Gyuyoung Kim]

(In reply to comment #52)
> (From update of attachment 140490 [details])
> it looks good over all. But fix the efl build first. :)

Hello Chang Shu,

Though I built EFL port with this patch on efl ews and buildbot, ews still notify us a build error. So, If you give r+ to this bug, I would like to check this patch on EFL buildbot.

Kihong, could you submit this patch again ? I just fixed ews again. Let's try to build this patch again.

[Kihong Kwon]

Created attachment 140671 [details]
Update patch for building on efl ews.

Thank you for your supporting. Gyuyoung.

[Gyuyoung Kim]

Comment on attachment 140671 [details]
Update patch for building on efl ews.

Attachment 140671 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12648236

[Chang Shu]

From the log of ews, the configuration wasn't complete. Does this patch introduce some new dependencies?
In addition, do you think this patch will fix some existing layout tests or maybe new tests are needed?

Failed to run "['Tools/Scripts/build-webkit', '--release', '--efl', '--makeargs="-j8"']" exit_code: 1
e-0.10>=0.10.30'
--   found gstreamer-plugins-base-0.10, version 0.10.35
-- Found GStreamer-Plugins-Base 
-- checking for module 'gstreamer-video-0.10>=0.10.30'
--   found gstreamer-video-0.10, version 0.10.35
-- Found GStreamer-Video 
-- Using platform-specific CMakeLists: /mnt/eflews/git/webkit/Source/WTF/wtf/PlatformEfl.cmake
-- Using platform-specific CMakeLists: /mnt/eflews/git/webkit/Source/JavaScriptCore/PlatformEfl.cmake
-- Using platform-specific CMakeLists: /mnt/eflews/git/webkit/Source/JavaScriptCore/shell/PlatformEfl.cmake
-- Using platform-specific CMakeLists: /mnt/eflews/git/webkit/Source/WebCore/PlatformEfl.cmake
-- Using platform-specific CMakeLists: /mnt/eflews/git/webkit/Source/WebKit/PlatformEfl.cmake
-- Checking to see if CXX compiler accepts flag -dumpversion
-- Checking to see if CXX compiler accepts flag -dumpversion - yes
-- Configuring incomplete, errors occurred!

[Kihong Kwon]

(In reply to comment #56)
> From the log of ews, the configuration wasn't complete. Does this patch introduce some new dependencies?

Yes, this patch makes a new dependency to edbus.

> In addition, do you think this patch will fix some existing layout tests or maybe new tests are needed?

I haven't thought there are needed more test cases for this patch, if this patch passes "LayoutTests/batterystatus".
Do you think we need to add other specific test case for this patch?

[Chang Shu]

(In reply to comment #57)
> (In reply to comment #56)
> > From the log of ews, the configuration wasn't complete. Does this patch introduce some new dependencies?
> 
> Yes, this patch makes a new dependency to edbus.

Good, we can fix ews later.
> 
> > In addition, do you think this patch will fix some existing layout tests or maybe new tests are needed?
> 
> I haven't thought there are needed more test cases for this patch, if this patch passes "LayoutTests/batterystatus".
> Do you think we need to add other specific test case for this patch?

Great, that's exactly what I mean. If the patch improves the pass rate for LayoutTests/batterystatus, we should see some changes in either Skipped file or perhaps efl test expectations. No need to work on new tests if we show previously failed tests are passing now.

[Kihong Kwon]

(In reply to comment #58)
> (In reply to comment #57)
> > (In reply to comment #56)
> > > From the log of ews, the configuration wasn't complete. Does this patch introduce some new dependencies?
> > 
> > Yes, this patch makes a new dependency to edbus.
> 
> Good, we can fix ews later.
> > 
> > > In addition, do you think this patch will fix some existing layout tests or maybe new tests are needed?
> > 
> > I haven't thought there are needed more test cases for this patch, if this patch passes "LayoutTests/batterystatus".
> > Do you think we need to add other specific test case for this patch?
> 
> Great, that's exactly what I mean. If the patch improves the pass rate for LayoutTests/batterystatus, we should see some changes in either Skipped file or perhaps efl test expectations. No need to work on new tests if we show previously failed tests are passing now.

There are no previously failed test cases.
I think my answer is not good enough to explain. I am sorry for that.
When I wrote Battery Status API for WebCore, I didn't implement actual working parts for the efl port.
This patch is continuous implementation of Battery Status API for the efl port.
But, I thought it is OK if this patch doesn't have problems with test cases which are already passed.
How do you think about this? Do we need another tests for this?
Thank you for your review. Chang Shu.

[Chang Shu]

Comment on attachment 140671 [details]
Update patch for building on efl ews.

Kihong explained that it's ok not to include tests in this patch offline:
"This patch is intended to be working for laptop or mobile device which has
battery.
Therefore, it doesn't seem to right to create specific test cases for the
desktop purposes as desktop PC doesn't run on battery, generally speaking.

For example, blackberry implemented this patch without having specific test
cases - https://bugs.webkit.org/show_bug.cgi?id=82615.
IMO, this would have not be a problem if I originally included this code in
my previous work, the main battery status API
(https://bugs.webkit.org/show_bug.cgi?id=62698)."

I think it's good to go.

[Gyuyoung Kim]

Comment on attachment 140671 [details]
Update patch for building on efl ews.

I will check efl build bot as soon as this patch is landed. Thank you for review.

[Kihong Kwon]

Thank you for your supporting Gyuyoung and Chang shu.;-)

[WebKit Review Bot]

Comment on attachment 140671 [details]
Update patch for building on efl ews.

Clearing flags on attachment: 140671

Committed r117378: <http://trac.webkit.org/changeset/117378>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.