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 tree / graph
 
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
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

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.