Bug 92964 – [EFL] Change return value of battey level

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kihong Kwon]

Battery level is defined like below, in the Battery Status API spec.
------------------------------------------------------------
level of type double, readonly
Represents the current battery level scaled from 0 to 1.0.
No exceptions.
------------------------------------------------------------
But EFL battery returns 0 ~ 100 level.

Therefore we need to fix this to 0 ~ 1.0.

[Kihong Kwon]

Created attachment 156027 [details]
Patch

[Ryuan Choi]

Comment on attachment 156027 [details]
Patch

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

> Source/WebCore/platform/efl/BatteryProviderEfl.cpp:165
>          levelChanged = true;
>      level = property->val.d;
>  
> -    WTF::RefPtr<BatteryStatus> batteryStatus = BatteryStatus::create(charging, chargingTime, dischargingTime, level);
> +    WTF::RefPtr<BatteryStatus> batteryStatus = BatteryStatus::create(charging, chargingTime, dischargingTime, level / 100);

If then, should we compare with clientBatteryStatus->level() * 100 and property->val.d ?

IMO, you'd better to assign property->val.d / 100 to `level` to reduce confusion.

[Kihong Kwon]

(In reply to comment #2)
> (From update of attachment 156027 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156027&action=review
> 
> > Source/WebCore/platform/efl/BatteryProviderEfl.cpp:165
> >          levelChanged = true;
> >      level = property->val.d;
> >  
> > -    WTF::RefPtr<BatteryStatus> batteryStatus = BatteryStatus::create(charging, chargingTime, dischargingTime, level);
> > +    WTF::RefPtr<BatteryStatus> batteryStatus = BatteryStatus::create(charging, chargingTime, dischargingTime, level / 100);
> 
> If then, should we compare with clientBatteryStatus->level() * 100 and property->val.d ?
> 
> IMO, you'd better to assign property->val.d / 100 to `level` to reduce confusion.

You are right. It was my mistake.

[Kihong Kwon]

Created attachment 156032 [details]
Patch

[Ryuan Choi]

Comment on attachment 156032 [details]
Patch

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

> Source/WebCore/platform/efl/BatteryProviderEfl.cpp:163
> -    if (!clientBatteryStatus || clientBatteryStatus->level() != property->val.d)
> +    if (!clientBatteryStatus || clientBatteryStatus->level() != property->val.d / 100)
>          levelChanged = true;
> -    level = property->val.d;
> +    level = property->val.d / 100;

I am not sure why you use level, local variable.

IMO, you can remove level variable or assign it before 161 line with declaration.

[Kihong Kwon]

Created attachment 156037 [details]
Patch

[Kihong Kwon]

Created attachment 156051 [details]
Patch

[Ryuan Choi]

Comment on attachment 156051 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        No new tests, no behavior changed.

I think that this comment is wrong.

This patch changes behavior but we can not make automated tests for this because we can not emulate E_Ukit.

> Source/WebCore/platform/efl/BatteryProviderEfl.cpp:155
> +    double level = property->val.d / 100;

I think that property of "Percentage" is not assigned yet.

[Kihong Kwon]

Created attachment 156053 [details]
Patch

[WebKit Review Bot]

Comment on attachment 156053 [details]
Patch

Clearing flags on attachment: 156053

Committed r124450: <http://trac.webkit.org/changeset/124450>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.