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.
Created attachment 170557 [details] Patch
Comment on attachment 170557 [details] Patch Attachment 170557 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14559527
Created attachment 170560 [details] Patch
Comment on attachment 170560 [details] Patch Attachment 170560 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14545524
Created attachment 170562 [details] Patch
Created attachment 170599 [details] Patch Modified title and change log more clearly.
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.
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.
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?)
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?
'Adding explicit page ID' will be as : Bug 100473
Created attachment 171103 [details] Patch
I updated new patch
I updated new patch that make WebVibrationProxy to be a member of WebPageProxy. This patch includes API changes also.
Created attachment 171131 [details] Patch
(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.
Created attachment 171610 [details] Patch
Rebased with the latest source.
Please unskip the unit test after landing, see bug 100839.
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.
(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.
(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.
(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!
Created attachment 171776 [details] Patch
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.
I made two bug for the EFL port issue : Bug 101132, Bug 101135 It will be clear to handle those in the separate issue.
(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)
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.
(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.
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. }
(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 :)
Created attachment 172788 [details] Patch
Comment on attachment 172788 [details] Patch Clearing flags on attachment: 172788 Committed r133828: <http://trac.webkit.org/changeset/133828>
All reviewed patches have been landed. Closing bug.
*** Bug 100439 has been marked as a duplicate of this bug. ***