Bug 100334 – [WK2] Make WebVibrationProxy to be a member of WebPageProxy.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Byungwoo Lee]

There is below assertion after r132386.

ASSERTION FAILED: !decoder.destinationID()
/home/buildslave-1/webkit-buildslave/efl-linux-64-debug/build/Source/WebKit2/Platform/CoreIPC/MessageReceiverMap.cpp(79) : bool CoreIPC::MessageReceiverMap::dispatchMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)

To fix the assertion problem,
adding/removing messageReceiver for WebVibrationProxy need to use new addMessageReceiver() function with destinationID for it's parameter.

[Byungwoo Lee]

Created attachment 170557 [details]
Patch

[Early Warning System Bot]

Comment on attachment 170557 [details]
Patch

Attachment 170557 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14559527

[Byungwoo Lee]

Created attachment 170560 [details]
Patch

[Early Warning System Bot]

Comment on attachment 170560 [details]
Patch

Attachment 170560 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14545524

[Byungwoo Lee]

Created attachment 170562 [details]
Patch

[Byungwoo Lee]

Created attachment 170599 [details]
Patch

Modified title and change log more clearly.

[Byungwoo Lee]

Created attachment 170610 [details]
Patch

Oh my god, what happened to me. I need some rest today :(
Correct the title of the change log DownloadProxy -> WebVibrationProxy.

[Anders Carlsson]

Comment on attachment 170610 [details]
Patch

I don't like this approach. The whole point of introducing named messages and message receiver maps is so we can get rid of a bunch of #ifdefs and let the relevant objects themselves be in charge of listening for messages.

WebVibrationProxy objects seem to be per context but they still use page destination IDs? Maybe they should be per page? If not, I'd recommend not using the destination ID but instead passing the page ID as an explicit argument to the messages.

[Byungwoo Lee]

Yes! Actually, I wasn't sure this approach also :)
Thank your for the clarification.

Current Vibration API and proxy/provider doesn't use the PageID or page proxy.
Based on the current implementation, pageID is not needed.
So I'm not sure that explicitly passing pageID is ok.

If the page ID is important to the vibration operation, it will be better to follow the other way that you suggested : make WebVibrationProxy per page.

But I think that needs some discussion.

How about removing the page id to prevent the assertion, and making another bug that arranges the issue of Vibration and page id?
(Or, is it enough to pass the pageID explicitly?)

[Byungwoo Lee]

I found that, WebNetworkInfoManagerProxy also has similar issue.

Some of the IPC messages of the proxy use page ID as destination ID, but some of it don't.

This seems to be arranged also.

And until these issue is cleared, 'Passing the page ID explicitly' will be better not to lose the information from the WebProcess.

How about this approach?

[Byungwoo Lee]

'Adding explicit page ID' will be as : Bug 100473

[Byungwoo Lee]

Created attachment 171103 [details]
Patch

[Byungwoo Lee]

I updated new patch

[Byungwoo Lee]

I updated new patch that make WebVibrationProxy to be a member of WebPageProxy.

This patch includes API changes also.

[Byungwoo Lee]

Created attachment 171131 [details]
Patch

[Byungwoo Lee]

(In reply to comment #15)
> Created an attachment (id=171131) [details]
> Patch

This was uploaded again to make ews green, and to add more detail information in ChangeLog.

[Byungwoo Lee]

Created attachment 171610 [details]
Patch

[Byungwoo Lee]

Rebased with the latest source.

[Dominik Röttsches (drott)]

Please unskip the unit test after landing, see bug 100839.

[Anders Carlsson]

Comment on attachment 171610 [details]
Patch

This looks mostly good, but I think the WebVibrationProxy should be the message receiver. The whole point of the MessageReceiver design is to avoid having a bunch of #ifdefs inside didReceiveMessage.

This means that you'll have to create the WebVibrationProxy eagerly in the WebPageProxy constructor. I don't think that should be a big problem though.

[Byungwoo Lee]

(In reply to comment #20)
> (From update of attachment 171610 [details])
> This looks mostly good, but I think the WebVibrationProxy should be the message receiver. The whole point of the MessageReceiver design is to avoid having a bunch of #ifdefs inside didReceiveMessage.
> 
> This means that you'll have to create the WebVibrationProxy eagerly in the WebPageProxy constructor. I don't think that should be a big problem though.

Thanks for the comment.

I'm sorry but I cannot get your point clearly. Could you explain more?

WebVibrationProxy is already a MessageReceiver(the class inherits CoreIPC::MessageReceiver), and it is added to the message receiver map in WebPageProxy because the vibration IPC has page destination ID.

The reason to use #if ENABLE(VIBRATION) in the WebPageProxy::didReceiveMessage() is that, WebVibrationProxy class will not defined(and WebPageProxy will not have WebVibrationProxy) if VIBRATION feature is off.

[Anders Carlsson]

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 171610 [details] [details])
> > This looks mostly good, but I think the WebVibrationProxy should be the message receiver. The whole point of the MessageReceiver design is to avoid having a bunch of #ifdefs inside didReceiveMessage.
> > 
> > This means that you'll have to create the WebVibrationProxy eagerly in the WebPageProxy constructor. I don't think that should be a big problem though.
> 
> Thanks for the comment.
> 
> I'm sorry but I cannot get your point clearly. Could you explain more?
> 
> WebVibrationProxy is already a MessageReceiver(the class inherits CoreIPC::MessageReceiver), and it is added to the message receiver map in WebPageProxy because the vibration IPC has page destination ID.
> 

WeProcessProxy already has a message receiver map, the web vibration proxy should be added to that.

[Byungwoo Lee]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > (From update of attachment 171610 [details] [details] [details])
> > > This looks mostly good, but I think the WebVibrationProxy should be the message receiver. The whole point of the MessageReceiver design is to avoid having a bunch of #ifdefs inside didReceiveMessage.
> > > 
> > > This means that you'll have to create the WebVibrationProxy eagerly in the WebPageProxy constructor. I don't think that should be a big problem though.
> > 
> > Thanks for the comment.
> > 
> > I'm sorry but I cannot get your point clearly. Could you explain more?
> > 
> > WebVibrationProxy is already a MessageReceiver(the class inherits CoreIPC::MessageReceiver), and it is added to the message receiver map in WebPageProxy because the vibration IPC has page destination ID.
> > 
> 
> WeProcessProxy already has a message receiver map, the web vibration proxy should be added to that.

Oh! I got the point :)

Maybe you told about the message receiver map in the WebContext.
(Currently, WebProcessProxy has WebContext and WebContext has message receiver map)

I'll apply those. Thanks!

[Byungwoo Lee]

Created attachment 171776 [details]
Patch

[Byungwoo Lee]

Comment on attachment 171776 [details]
Patch

Applied the guide from Anders.

Previously, WebVibrationProxy was defined as MessageReceiver and added to the message receiver map, but the vibration IPC messages were not handled by the message receiver map. (Terrible mistake!)

As Anders pointed, WebVibrationProxy will be added to the message receiver map of WebContext and the vibration IPC messages will be handled by the message receiver map.

[Byungwoo Lee]

I made two bug for the EFL port issue : Bug 101132, Bug 101135
It will be clear to handle those in the separate issue.

[Byungwoo Lee]

(In reply to comment #26)
> I made two bug for the EFL port issue : Bug 101132, Bug 101135
> It will be clear to handle those in the separate issue.

Oops. this is wrong comment (Should be in Bug 98580)

[Gyuyoung Kim]

Comment on attachment 171776 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKPage.cpp:198
> +#else

Use UNUSED_PARAM(page)

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:35
> +#if ENABLE(VIBRATION)

I prefer to place #if ENABLE() below main include list.

[Byungwoo Lee]

(In reply to comment #28)
> (From update of attachment 171776 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171776&action=review
> 
> > Source/WebKit2/UIProcess/API/C/WKPage.cpp:198
> > +#else
> 
> Use UNUSED_PARAM(page)
Ok! I'll add it.

> 
> > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:35
> > +#if ENABLE(VIBRATION)
> 
> I prefer to place #if ENABLE() below main include list.
Ok. I'll move it to the below.

[Gyuyoung Kim]

Comment on attachment 171776 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:460
> +    evas_object_smart_callback_call(m_view, "vibrate", &vibrationTime);

I think you have to move this to VibrationClientEfl.cpp. In previous refactoring, each client can send a signal via a viewImpl object.

For example,

void VibrationClientEfl::vibrateCallback(WKVibrationRef, uint64_t vibrationTime, const void* clientInfo)
{ 
    toVibrationClient(clientInfo)->m_viewImpl->smartCallback<Vibration>().call(&vibrationTiem); // I think you can send signal here directly.
}

[Byungwoo Lee]

(In reply to comment #30)
> (From update of attachment 171776 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171776&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:460
> > +    evas_object_smart_callback_call(m_view, "vibrate", &vibrationTime);
> 
> I think you have to move this to VibrationClientEfl.cpp. In previous refactoring, each client can send a signal via a viewImpl object.
> 
> For example,
> 
> void VibrationClientEfl::vibrateCallback(WKVibrationRef, uint64_t vibrationTime, const void* clientInfo)
> { 
>     toVibrationClient(clientInfo)->m_viewImpl->smartCallback<Vibration>().call(&vibrationTiem); // I think you can send signal here directly.
> }

Yes, I have to rebase with the latest source. Thanks for pointing this :)

[Byungwoo Lee]

Created attachment 172788 [details]
Patch

[WebKit Review Bot]

Comment on attachment 172788 [details]
Patch

Clearing flags on attachment: 172788

Committed r133828: <http://trac.webkit.org/changeset/133828>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Byungwoo Lee]

*** Bug 100439 has been marked as a duplicate of this bug. ***