Bug 193416

Summary: Only run the node comparison code in FrameSelection::respondToNodeModification() for range selections
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: HTML EditingAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, rniwa, sbarati, simon.fraser, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Simon Fraser (smfr) 2019-01-14 16:19:35 PST
Only run the node comparison code in FrameSelection::respondToNodeModification() for range selections
Comment 1 Simon Fraser (smfr) 2019-01-14 16:22:42 PST
Created attachment 359095 [details]
Patch
Comment 2 Simon Fraser (smfr) 2019-01-14 16:23:59 PST
I measured this as a ~1.5% improvement on Speedometer2.
Comment 3 Radar WebKit Bug Importer 2019-01-14 16:52:30 PST
<rdar://problem/47270794>
Comment 4 Wenson Hsieh 2019-01-14 17:16:22 PST
Comment on attachment 359095 [details]
Patch

Nice!
Comment 5 Wenson Hsieh 2019-01-14 17:25:03 PST
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.
Comment 6 Wenson Hsieh 2019-01-14 19:09:02 PST
(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 7 WebKit Commit Bot 2019-01-14 19:31:47 PST
Comment on attachment 359095 [details]
Patch

Clearing flags on attachment: 359095

Committed r239971: <https://trac.webkit.org/changeset/239971>
Comment 8 WebKit Commit Bot 2019-01-14 19:31:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Saam Barati 2019-01-15 14:48:52 PST
Looks like a 1% speedometer2 progression!
Comment 10 Saam Barati 2019-01-15 14:49:20 PST
(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