Summary: | [EFL] Fix crash caused by invalid cursor image. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | KwangHyuk <hyuki.kim> | ||||||||||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | buildbot, bunhere, cdumez, commit-queue, gyuyoung.kim, lucas.de.marchi, rniwa, ryuan.choi, sergio | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Linux | ||||||||||||||||
Attachments: |
|
Description
KwangHyuk
2014-07-06 08:19:37 PDT
Created attachment 234458 [details]
Patch
Add code to prevent referring of unavailable cursor image on EwkView since the custom cursor image is invalid once a mouse is out of the webview.
Comment on attachment 234458 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=234458&action=review > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:223 > + smartData->mouse_in = 1; Use true instead of 1. > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:233 > + smartData->mouse_in = 0; Use true instead of 0. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:224 > + Eina_Bool mouse_in; I'm not sure whether we should maintain this flag as a field of Ewk_View_Smart_Data since this flag is only used internally. Any reason we should open this flag as public ? If not, it looks we maintain it as one of EwkView member internally. Comment on attachment 234458 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=234458&action=review >> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:233 >> + smartData->mouse_in = 0; > > Use true instead of 0. Typo: true -> false. >> Source/WebKit2/UIProcess/API/efl/ewk_view.h:224 >> + Eina_Bool mouse_in; > I'm not sure whether we should maintain this flag as a field of Ewk_View_Smart_Data since this flag is only used internally. Any reason we should open this flag as public ? If not, it looks we maintain it as one of EwkView member internally. Thank you and agree with you. I found better approach. it will be updated soon. :) Created attachment 234461 [details]
Patch
Remove calling of updateCursor since the custom cursor image is invalid once a mouse is out of the webview. in addition, EVAS_CALLBACK_MOUSE_IN callback subscribtion is also removed because it's handling updateCursor only.
Comment on attachment 234461 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=234461&action=review > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:-225 > - self->updateCursor(); This callback was added by r165730 in order to restore previous cursor before going out of webview. http://trac.webkit.org/changeset/165730 Can we restore previous cursor when coming back to inside WebView without this cursor update ? (In reply to comment #6) > (From update of attachment 234461 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=234461&action=review > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:-225 > > - self->updateCursor(); > > This callback was added by r165730 in order to restore previous cursor before going out of webview. > http://trac.webkit.org/changeset/165730 > > Can we restore previous cursor when coming back to inside WebView without this cursor update ? Re-entrance point can be always changed randomly according to user's intention. In addition, cursor image is newly updated whenever mouse re-enter the webview again. So, I can' sure whether restoration is good idea. However, currently custom cursor seems to be destroyed as soon as it was rendered. So, at least, updateCursor on mouse in callback seems to be removed. Comment on attachment 234461 [details] Patch Attachment 234461 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5361787385937920 New failing tests: media/W3C/video/networkState/networkState_during_loadstart.html Created attachment 234463 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 234461 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=234461&action=review > > > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:-225 > > > - self->updateCursor(); > > > > This callback was added by r165730 in order to restore previous cursor before going out of webview. > > http://trac.webkit.org/changeset/165730 > > > > Can we restore previous cursor when coming back to inside WebView without this cursor update ? > > Re-entrance point can be always changed randomly according to user's intention. > In addition, cursor image is newly updated whenever mouse re-enter the webview again. > So, I can' sure whether restoration is good idea. > > However, currently custom cursor seems to be destroyed as soon as it was rendered. > So, at least, updateCursor on mouse in callback seems to be removed. I got your point, but it just mean that we move back to previous state. IMO, One other solution is removing Evas Object based cursor. Recently, I realized that evas object cursor is not useful to collaborate with multiple windows. After removed evas object cursor, I think that we can implement custom cursor as xpixmap which EwkView maintains. Although it tightly depends on X, ewebkit now only support X backend and we can make general function like applyFallbackCursor having several backend until EFL implement common APIs for the cursor. Comment on attachment 234461 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=234461&action=review >>>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:-225 >>>> - self->updateCursor(); >>> >>> This callback was added by r165730 in order to restore previous cursor before going out of webview. >>> http://trac.webkit.org/changeset/165730 >>> >>> Can we restore previous cursor when coming back to inside WebView without this cursor update ? >> >> Re-entrance point can be always changed randomly according to user's intention. >> In addition, cursor image is newly updated whenever mouse re-enter the webview again. >> So, I can' sure whether restoration is good idea. >> >> However, currently custom cursor seems to be destroyed as soon as it was rendered. >> So, at least, updateCursor on mouse in callback seems to be removed. > > I got your point, but it just mean that we move back to previous state. > > IMO, One other solution is removing Evas Object based cursor. > Recently, I realized that evas object cursor is not useful to collaborate with multiple windows. > > After removed evas object cursor, I think that we can implement custom cursor as xpixmap which EwkView maintains. > Although it tightly depends on X, ewebkit now only support X backend and we can make general function like applyFallbackCursor having several backend until EFL implement common APIs for the cursor. How about keeping void EwkViewEventHandler<EVAS_CALLBACK_MOUSE_IN>::handleEvent(void* data, Evas*, Evas_Object*, void*) with "notImplemented", then add "FIXME: bla bla" ? (In reply to comment #11) > (From update of attachment 234461 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=234461&action=review > > >>>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:-225 > >>>> - self->updateCursor(); > >>> > >>> This callback was added by r165730 in order to restore previous cursor before going out of webview. > >>> http://trac.webkit.org/changeset/165730 > >>> > >>> Can we restore previous cursor when coming back to inside WebView without this cursor update ? > >> > >> Re-entrance point can be always changed randomly according to user's intention. > >> In addition, cursor image is newly updated whenever mouse re-enter the webview again. > >> So, I can' sure whether restoration is good idea. > >> > >> However, currently custom cursor seems to be destroyed as soon as it was rendered. > >> So, at least, updateCursor on mouse in callback seems to be removed. > > > > I got your point, but it just mean that we move back to previous state. > > > > IMO, One other solution is removing Evas Object based cursor. > > Recently, I realized that evas object cursor is not useful to collaborate with multiple windows. > > > > After removed evas object cursor, I think that we can implement custom cursor as xpixmap which EwkView maintains. > > Although it tightly depends on X, ewebkit now only support X backend and we can make general function like applyFallbackCursor having several backend until EFL implement common APIs for the cursor. > > How about keeping void EwkViewEventHandler<EVAS_CALLBACK_MOUSE_IN>::handleEvent(void* data, Evas*, Evas_Object*, void*) with "notImplemented", then add "FIXME: bla bla" ? Thank you for your idea. :) Created attachment 234497 [details]
Patch
Apply reviewer's comment.
Comment on attachment 234497 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=234497&action=review > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:222 > + // FIXME : self->updateCursor() call is removed because crash occurs when invalid cursor image is referred. nit: FIXME : -> FIXME: Created attachment 234559 [details]
Patch
(In reply to comment #15) > Created an attachment (id=234559) [details] > Patch Once you got r+ed, you don't need to request r? again. Just request cq?, then committer can land the patch. (In reply to comment #16) > (In reply to comment #15) > > Created an attachment (id=234559) [details] [details] > > Patch > > Once you got r+ed, you don't need to request r? again. Just request cq?, then committer can land the patch. Ok, good tip. Created attachment 234617 [details]
patch
Comment on attachment 234617 [details] patch Clearing flags on attachment: 234617 Committed r170914: <http://trac.webkit.org/changeset/170914> All reviewed patches have been landed. Closing bug. |