Bug 97839 – [EFL][WK2] {Vibration,Battery,NetworkInfo}Provider should contain WKContextRef instead of proxy.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Byungwoo Lee]

There is an unclear relation between WebContext and {VibrationProvider, BatteryProvider, NetworkInfoProvider}

VibrationProvider has WKRetainPtr<WKVibrationRef> m_wkVibrationRef which has reference for object WebVibrationProxy.

But the WebVibrationProxy instance is a member of WebContext which also has RefPtr<WebVibrationProxy>

Two classes are sharing the reference for WebVibrationProxy but there is no relation between the two.

This can make a problem that,
When WebContext if deleted befor the VibrationProvider, WebVibrationProxy will be alive without WebContext,
because VibrationProvider also has the reference for WebVibrationProxy.

This is a problem because WebVibrationProxy should be a member of WebContext.

To prevent this,
there should be a relation between VibrationProvider and WebContext.

(And same about BatteryProvider, NetworkInfoProvider)

[Byungwoo Lee]

Created attachment 166142 [details]
Patch

[Chris Dumez]

Comment on attachment 166142 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        [EFL][WK2] Make relation between WebContext and {VibrationProvider,BatteryProvider,NetworkInfoProvider}.

The bug title is really not clear.

> Source/WebKit2/ChangeLog:16
> +        This can make a problem that, when WebContext is deleted befor the

"before"

> Source/WebKit2/ChangeLog:25
> +        Same about BatteryProvider and NetworkInfoProvider.

"Same for"

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:89
> +        vibrationProvider = VibrationProvider::create(context.get());

It looks like we are missing the networkInfoProvider, right? We should probably fix this in a separate patch.

[Chris Dumez]

(In reply to comment #2)
> (From update of attachment 166142 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166142&action=review
> 
> > Source/WebKit2/ChangeLog:3
> > +        [EFL][WK2] Make relation between WebContext and {VibrationProvider,BatteryProvider,NetworkInfoProvider}.
> 
> The bug title is really not clear.
> 
> > Source/WebKit2/ChangeLog:16
> > +        This can make a problem that, when WebContext is deleted befor the
> 
> "before"
> 
> > Source/WebKit2/ChangeLog:25
> > +        Same about BatteryProvider and NetworkInfoProvider.
> 
> "Same for"
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:89
> > +        vibrationProvider = VibrationProvider::create(context.get());
> 
> It looks like we are missing the networkInfoProvider, right? We should probably fix this in a separate patch.

Addressing the missing networkInfoProvider initialization issue in Bug 97865. This patch will need rebasing afterwards.

[Byungwoo Lee]

Comment on attachment 166142 [details]
Patch

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

>> Source/WebKit2/ChangeLog:3
>> +        [EFL][WK2] Make relation between WebContext and {VibrationProvider,BatteryProvider,NetworkInfoProvider}.
> 
> The bug title is really not clear.

Yes. How about this?
[EFL][WK2] {Vibration,Battery,NetworkInfo}Provider should contain WKContextRef instead of proxy.

>> Source/WebKit2/ChangeLog:16
>> +        This can make a problem that, when WebContext is deleted befor the
> 
> "before"

Oops~

>> Source/WebKit2/ChangeLog:25
>> +        Same about BatteryProvider and NetworkInfoProvider.
> 
> "Same for"

Ok

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:89
>> +        vibrationProvider = VibrationProvider::create(context.get());
> 
> It looks like we are missing the networkInfoProvider, right? We should probably fix this in a separate patch.

Yes I couldn't find a code about networkInfoProvider. it should be fixed in other patch :)

[Chris Dumez]

(In reply to comment #4)
> (From update of attachment 166142 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166142&action=review
> 
> >> Source/WebKit2/ChangeLog:3
> >> +        [EFL][WK2] Make relation between WebContext and {VibrationProvider,BatteryProvider,NetworkInfoProvider}.
> > 
> > The bug title is really not clear.
> 
> Yes. How about this?
> [EFL][WK2] {Vibration,Battery,NetworkInfo}Provider should contain WKContextRef instead of proxy.

Yes, I think it is better.

> >> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:89
> >> +        vibrationProvider = VibrationProvider::create(context.get());
> > 
> > It looks like we are missing the networkInfoProvider, right? We should probably fix this in a separate patch.
> 
> Yes I couldn't find a code about networkInfoProvider. it should be fixed in other patch :)

This has been fixed in 97865, please rebase.

[Byungwoo Lee]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 166142 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=166142&action=review
> > 
> > >> Source/WebKit2/ChangeLog:3
> > >> +        [EFL][WK2] Make relation between WebContext and {VibrationProvider,BatteryProvider,NetworkInfoProvider}.
> > > 
> > > The bug title is really not clear.
> > 
> > Yes. How about this?
> > [EFL][WK2] {Vibration,Battery,NetworkInfo}Provider should contain WKContextRef instead of proxy.
> 
> Yes, I think it is better.
> 
> > >> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:89
> > >> +        vibrationProvider = VibrationProvider::create(context.get());
> > > 
> > > It looks like we are missing the networkInfoProvider, right? We should probably fix this in a separate patch.
> > 
> > Yes I couldn't find a code about networkInfoProvider. it should be fixed in other patch :)
> 
> This has been fixed in 97865, please rebase.

Ok~ I'll apply it :)

[Byungwoo Lee]

Created attachment 166186 [details]
Patch

[Byungwoo Lee]

Comment on attachment 166186 [details]
Patch

Need apply bug 97528

[Gyuyoung Kim]

Comment on attachment 166186 [details]
Patch

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

[Byungwoo Lee]

Created attachment 166209 [details]
Patch

[Chris Dumez]

Comment on attachment 166209 [details]
Patch

LGTM, thanks.

[Kenneth Rohde Christiansen]

Comment on attachment 166209 [details]
Patch

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

> Source/WebKit2/ChangeLog:16
> +        This can make a problem that, when WebContext is deleted before the

This can become a problem when We...

> Source/WebKit2/ChangeLog:23
> +        To prevent this, changed the VibrationProvider to have

To prevent this, I changed

[Byungwoo Lee]

(In reply to comment #12)
> (From update of attachment 166209 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166209&action=review
> 
> > Source/WebKit2/ChangeLog:16
> > +        This can make a problem that, when WebContext is deleted before the
> 
> This can become a problem when We...
> 
> > Source/WebKit2/ChangeLog:23
> > +        To prevent this, changed the VibrationProvider to have
> 
> To prevent this, I changed

Ok~ I'll change it.

[Byungwoo Lee]

Created attachment 166260 [details]
Patch

[Byungwoo Lee]

Created attachment 166262 [details]
Patch

[WebKit Review Bot]

Comment on attachment 166262 [details]
Patch

Clearing flags on attachment: 166262

Committed r129922: <http://trac.webkit.org/changeset/129922>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.