Bug 206798

Summary: Tighten up some of the drag state machine logic
Product: WebKit Reporter: Darin Adler <darin>
Component: UI EventsAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: ddkilzer, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Darin Adler
Reported 2020-01-25 07:52:26 PST
Tighten up some of the drag state machine logic
Attachments
Patch (5.81 KB, patch)
2020-01-25 07:57 PST, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (9.46 KB, patch)
2020-01-25 16:57 PST, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (9.46 KB, patch)
2020-01-25 17:04 PST, Darin Adler 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Darin Adler
Comment 1 2020-01-25 07:57:08 PST
Created attachment 388774 [details] Patch
Darin Adler
Comment 2 2020-01-25 07:58:44 PST
There’s a chance this fixes a drag-related crash we’ve been seeing, but I haven’t been able to reproduce the crash, fully understand it, or make a test case.
Wenson Hsieh
Comment 3 2020-01-25 12:36:38 PST
Comment on attachment 388774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388774&action=review Thanks for the patch! r=me with the build fix. > Source/WebCore/page/EventHandler.cpp:3651 > + if (dragState().source && dragState().dataTransfer() && dragState().shouldDispatchEvents) { `dragState().dataTransfer()` => `dragState().dataTransfer` I wonder if it might be nice to pull the (source && dataTransfer && shouldDispatchEvents) check into either a helper function or method on DragState, since we have it in two places here.
Darin Adler
Comment 4 2020-01-25 16:57:24 PST Comment hidden (obsolete)
Darin Adler
Comment 5 2020-01-25 16:58:06 PST
Comment on attachment 388774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388774&action=review >> Source/WebCore/page/EventHandler.cpp:3651 >> + if (dragState().source && dragState().dataTransfer() && dragState().shouldDispatchEvents) { > > `dragState().dataTransfer()` => `dragState().dataTransfer` > > I wonder if it might be nice to pull the (source && dataTransfer && shouldDispatchEvents) check into either a helper function or method on DragState, since we have it in two places here. Sure, did that. Uploaded a new patch with the changes.
Darin Adler
Comment 6 2020-01-25 17:04:07 PST
Created attachment 388793 [details] Patch
Darin Adler
Comment 7 2020-01-25 18:43:40 PST
Comment on attachment 388793 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388793&action=review > Source/WebCore/page/EventHandler.h:429 > + static bool shouldDispatchEventsToDragSourceElement(); Just realized this can just be a free function; doesn’t have to be a class member.
Darin Adler
Comment 8 2020-01-25 18:50:23 PST
Comment on attachment 388793 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388793&action=review >> Source/WebCore/page/EventHandler.h:429 >> + static bool shouldDispatchEventsToDragSourceElement(); > > Just realized this can just be a free function; doesn’t have to be a class member. No that's wrong. It does need to be a member function so it can have access to dragState().
Darin Adler
Comment 9 2020-01-25 22:33:48 PST
Committed r255128: <https://trac.webkit.org/changeset/255128>
David Kilzer (:ddkilzer)
Comment 10 2020-01-26 03:50:30 PST
<rdar://problem/58837212>
Note You need to log in before you can comment on or make changes to this bug.