Bug 193416 – Only run the node comparison code in FrameSelection::respondToNodeModification() for range selections

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Simon Fraser (smfr)]

Only run the node comparison code in FrameSelection::respondToNodeModification() for range selections

[Simon Fraser (smfr)]

Created attachment 359095 [details]
Patch

[Simon Fraser (smfr)]

I measured this as a ~1.5% improvement on Speedometer2.

[Radar WebKit Bug Importer]

<rdar://problem/47270794>

[Wenson Hsieh]

Comment on attachment 359095 [details]
Patch

Nice!

[Wenson Hsieh]

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.

[Wenson Hsieh]

(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.

[WebKit Commit Bot]

Comment on attachment 359095 [details]
Patch

Clearing flags on attachment: 359095

Committed r239971: <https://trac.webkit.org/changeset/239971>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

Looks like a 1% speedometer2 progression!

[Saam Barati]

(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