Only run the node comparison code in FrameSelection::respondToNodeModification() for range selections
Created attachment 359095 [details] Patch
I measured this as a ~1.5% improvement on Speedometer2.
<rdar://problem/47270794>
Comment on attachment 359095 [details] Patch Nice!
Comment on attachment 359095 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359095&action=review > Source/WebCore/editing/FrameSelection.cpp:545 > + if (compareResult == Range::NODE_BEFORE_AND_AFTER || compareResult == Range::NODE_INSIDE) { Doesn't this mean that we set clearRenderTreeSelection to true if either the selection completely encompasses node, or node completely encompasses the selection? It seems like in the latter case, we can enter this if statement even with a caret selection. We should double check that we either never get here with a caret selection, or that if we get here with a caret selection, we didn't need to clear the render tree selection in the first place.
(In reply to Wenson Hsieh from comment #5) > Comment on attachment 359095 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=359095&action=review > > > Source/WebCore/editing/FrameSelection.cpp:545 > > + if (compareResult == Range::NODE_BEFORE_AND_AFTER || compareResult == Range::NODE_INSIDE) { > > Doesn't this mean that we set clearRenderTreeSelection to true if either the > selection completely encompasses node, or node completely encompasses the > selection? It seems like in the latter case, we can enter this if statement > even with a caret selection. > > We should double check that we either never get here with a caret selection, > or that if we get here with a caret selection, we didn't need to clear the > render tree selection in the first place. From a hallway conversation with Ryosuke, it looks like we don't need to clear the render tree selection here because the node that encompasses the selection is going away anyways, so it doesn't look like this change should break anything.
Comment on attachment 359095 [details] Patch Clearing flags on attachment: 359095 Committed r239971: <https://trac.webkit.org/changeset/239971>
All reviewed patches have been landed. Closing bug.
Looks like a 1% speedometer2 progression!
(In reply to Simon Fraser (smfr) from comment #2) > I measured this as a ~1.5% improvement on Speedometer2. I see you've already measured this :) The bots agree