Bug 107821 - [EFL][WK2] Use C API inside BatteryProvider and NetworkInfoProvider
Summary: [EFL][WK2] Use C API inside BatteryProvider and NetworkInfoProvider
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sudarsana Nagineni (babu)
URL:
Keywords:
Depends on:
Blocks: 107657
  Show dependency treegraph
 
Reported: 2013-01-24 06:19 PST by Sudarsana Nagineni (babu)
Modified: 2013-02-15 09:11 PST (History)
9 users (show)

See Also:


Attachments
Patch (9.28 KB, patch)
2013-01-24 08:29 PST, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff
patch (9.44 KB, patch)
2013-01-30 06:56 PST, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff
Patch (9.60 KB, patch)
2013-02-11 09:59 PST, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff
Patch (9.58 KB, patch)
2013-02-12 08:48 PST, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sudarsana Nagineni (babu) 2013-01-24 06:19:25 PST
As per Bug 107657, we should start using the C API instead of accessing internal C++ classes directly.
Comment 1 Sudarsana Nagineni (babu) 2013-01-24 08:29:19 PST
Created attachment 184507 [details]
Patch
Comment 2 Mikhail Pozdnyakov 2013-01-24 08:31:53 PST
Comment on attachment 184507 [details]
Patch

LGTM
Comment 3 Sudarsana Nagineni (babu) 2013-01-30 06:56:59 PST
Created attachment 185496 [details]
patch

Rebased.
Comment 4 Chris Dumez 2013-01-30 07:11:16 PST
Comment on attachment 185496 [details]
patch

LGTM.
Comment 5 Gyuyoung Kim 2013-01-30 18:35:23 PST
Comment on attachment 185496 [details]
patch

Looks fine.
Comment 6 Sudarsana Nagineni (babu) 2013-02-11 09:59:42 PST
Created attachment 187604 [details]
Patch

Rebased on master.
Comment 7 Kenneth Rohde Christiansen 2013-02-12 05:33:46 PST
Comment on attachment 187604 [details]
Patch

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

> Source/WebKit2/UIProcess/efl/BatteryProvider.cpp:94
> +    WKBatteryStatusRef wkBatteryStatus = WKBatteryStatusCreate(status->charging(), status->chargingTime(), status->dischargingTime(), status->level());

So why not WKRetainPtr? Are you releasing it manually later?
Comment 8 Chris Dumez 2013-02-12 05:40:29 PST
Comment on attachment 187604 [details]
Patch

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

>> Source/WebKit2/UIProcess/efl/BatteryProvider.cpp:94
>> +    WKBatteryStatusRef wkBatteryStatus = WKBatteryStatusCreate(status->charging(), status->chargingTime(), status->dischargingTime(), status->level());
> 
> So why not WKRetainPtr? Are you releasing it manually later?

Right. Good catch Kenneth. I also believe you need to adopt the returned value here.

> Source/WebKit2/UIProcess/efl/BatteryProvider.cpp:97
> +    WKBatteryManagerProviderDidChangeBatteryStatus(m_batteryManager.get(), wkEventType.get(), wkBatteryStatus);

Have you tried toAPI(eventType.impl()) ?
Comment 9 Sudarsana Nagineni (babu) 2013-02-12 07:35:18 PST
Comment on attachment 187604 [details]
Patch

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

>>> Source/WebKit2/UIProcess/efl/BatteryProvider.cpp:94
>>> +    WKBatteryStatusRef wkBatteryStatus = WKBatteryStatusCreate(status->charging(), status->chargingTime(), status->dischargingTime(), status->level());
>> 
>> So why not WKRetainPtr? Are you releasing it manually later?
> 
> Right. Good catch Kenneth. I also believe you need to adopt the returned value here.

Okay.

>> Source/WebKit2/UIProcess/efl/BatteryProvider.cpp:97
>> +    WKBatteryManagerProviderDidChangeBatteryStatus(m_batteryManager.get(), wkEventType.get(), wkBatteryStatus);
> 
> Have you tried toAPI(eventType.impl()) ?

I have not, but I will check it.
Comment 10 Sudarsana Nagineni (babu) 2013-02-12 08:48:14 PST
Created attachment 187877 [details]
Patch

Take comments from Kenneth and Chris into consideration.
Comment 11 Kenneth Rohde Christiansen 2013-02-15 08:19:07 PST
Comment on attachment 187877 [details]
Patch

LGTM
Comment 12 WebKit Review Bot 2013-02-15 09:11:13 PST
Comment on attachment 187877 [details]
Patch

Clearing flags on attachment: 187877

Committed r143008: <http://trac.webkit.org/changeset/143008>
Comment 13 WebKit Review Bot 2013-02-15 09:11:18 PST
All reviewed patches have been landed.  Closing bug.