| Summary: | Tighten up some of the drag state machine logic | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||
| Component: | UI Events | Assignee: | 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
Darin Adler
2020-01-25 07:52:26 PST
Created attachment 388774 [details]
Patch
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. 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. Created attachment 388792 [details]
Patch
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. Created attachment 388793 [details]
Patch
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. 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(). Committed r255128: <https://trac.webkit.org/changeset/255128> |