<rdar://problem/61914738>
Created attachment 404310 [details] Patch
Committed r264386: <https://trac.webkit.org/changeset/264386> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404310 [details].
I wonder if this fixes bug 213300 as well.
*** Bug 213300 has been marked as a duplicate of this bug. ***
(In reply to mitz from comment #3) > I wonder if this fixes bug 213300 as well. It indeed did. Thanks!
Comment on attachment 404310 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404310&action=review I like all the const and upgrading to references, but landing all of those does seem to sort of hide the fix, which I assume was just adding the call to snapRectToDevicePixels? Can we create a regression test? > Source/WebCore/editing/FrameSelection.cpp:1787 > + if (m_selection.isCaret() && m_caretPaint && m_selection.start().deprecatedNode()) What motivated the addition of this null check? The old code just assumed this is non-null. > Source/WebCore/editing/FrameSelection.cpp:2291 > + if (m_position.deepEquivalent().deprecatedNode() && m_position.deepEquivalent().deprecatedNode()->document().frame() == frame) What motivated the addition of this null check?
(In reply to Darin Adler from comment #6) > Comment on attachment 404310 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=404310&action=review > > I like all the const and upgrading to references, but landing all of those > does seem to sort of hide the fix, which I assume was just adding the call > to snapRectToDevicePixels? Yes, the fix is simply going from roundedIntPoint to snapRectToDevicePixels. I tend to go with a pre-cleanup first and land the fix separately, so not sure what happened here. I think the initial const change looked small enough to land it together with the fix and I didn't really pay attention to how much additional changes it required at the end. In retrospect, I should have done my usual pre-cleanup setup here. > > Can we create a regression test? I tried but I couldn't think of a good way of (ref)testing a blinking caret. I can't even use the usual "collect the repaint rects" as the repaint rect are correct here. I am happy to create one if someone has some ideas. > > > Source/WebCore/editing/FrameSelection.cpp:1787 > > + if (m_selection.isCaret() && m_caretPaint && m_selection.start().deprecatedNode()) > > What motivated the addition of this null check? The old code just assumed > this is non-null. > > > Source/WebCore/editing/FrameSelection.cpp:2291 > > + if (m_position.deepEquivalent().deprecatedNode() && m_position.deepEquivalent().deprecatedNode()->document().frame() == frame) > > What motivated the addition of this null check? >The old code just assumed this is non-null. I noticed this pattern in the rendering code. The initial implementation, probably correctly, assumed a never-nullptr value (this was before we started using references extensively) but it may not be true today as the code evolved over time (hence all the drive-by reference changes). This code just looked unsafe to me. (I personally very much dislike random null-checks in the code and prefer references when possible)