<rdar://problem/32236827>
Created attachment 313873 [details] Depends on bug #173832
Comment on attachment 313873 [details] Depends on bug #173832 View in context: https://bugs.webkit.org/attachment.cgi?id=313873&action=review r=me assuming it passes tests. > Source/WebCore/page/DragController.cpp:726 > + auto* renderer = image.renderer(); > + if (!is<RenderImage>(renderer)) > + return false; > + > + auto* cachedImage = downcast<RenderImage>(*renderer).cachedImage(); > + return cachedImage && !cachedImage->errorOccurred() && cachedImage->imageForRenderer(renderer); We need to be always careful when we rely on the existence of renderer(). Are we sure we've updated the layout / style before getting here? > Source/WebCore/page/ios/EventHandlerIOS.mm:579 > + IntPoint adjustedClientPosition; We do we need to declare this upfront? > Source/WebCore/page/ios/EventHandlerIOS.mm:584 > + { > + FloatPoint adjustedClientPositionAsFloatPoint(clientPosition); > + protectedFrame->nodeRespondingToClickEvents(clientPosition, adjustedClientPositionAsFloatPoint); > + adjustedClientPosition = roundedIntPoint(adjustedClientPositionAsFloatPoint); > + } Why can't we get rid of the curly brackets and declare adjustedClientPosition right in the third line instead? > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/contenteditable-and-target.html:1 > +<head> Missing DOCTYPE.
Comment on attachment 313873 [details] Depends on bug #173832 View in context: https://bugs.webkit.org/attachment.cgi?id=313873&action=review >> Source/WebCore/page/DragController.cpp:726 >> + return cachedImage && !cachedImage->errorOccurred() && cachedImage->imageForRenderer(renderer); > > We need to be always careful when we rely on the existence of renderer(). Are we sure we've updated the layout / style before getting here? I see. We have always accessed the renderer here (see DragController::draggableElement), but it doesn't seem like we update layout/style from all call sites (even on Mac), which seems problematic! I'll fix it for iOS first by making sure that we update layout/style when starting a drag. >> Source/WebCore/page/ios/EventHandlerIOS.mm:579 >> + IntPoint adjustedClientPosition; > > We do we need to declare this upfront? Right -- I cleaned this up as a part of my second patch. >> Source/WebCore/page/ios/EventHandlerIOS.mm:584 >> + } > > Why can't we get rid of the curly brackets and declare adjustedClientPosition right in the third line instead? 👍 >> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/contenteditable-and-target.html:1 >> +<head> > > Missing DOCTYPE. Added!
Created attachment 313953 [details] Patch v2
Committed r218855: <http://trac.webkit.org/changeset/218855>