[webkit-reviews] review denied: [Bug 216325] Make Selection throw same errors as other browsers and return same Range object from getRangeAt like other browsers : [Attachment 408368] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 216325: Make Selection throw same errors as other browsers and return same
Range object from getRangeAt like other browsers
https://bugs.webkit.org/show_bug.cgi?id=216325

Attachment 408368: Patch

https://bugs.webkit.org/attachment.cgi?id=408368&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 408368
  --> https://bugs.webkit.org/attachment.cgi?id=408368
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408368&action=review

> Source/WebCore/page/DOMSelection.cpp:323
> +	   if (auto shadowAncestor = selectionShadowAncestor(frame))
> +	       m_cachedRange =
createLiveRange(makeSimpleRange(*makeBoundaryPointBeforeNode(*shadowAncestor)))
;
> +	   else
> +	       m_cachedRange =
createLiveRange(*frame.selection().selection().firstRange());

This is not right. What invalidates this when the selection changes? The
DOMSelection class’s functions aren’t the only way to change the selection.

If we need to cache a Range and re-use it we will have to check that it’s still
the same and create a new one if it’s not. Or add notification from the actual
selection mechanism to tell the DOMSelection when to generate a new Range.

Easiest is probably to always recalculate the range and then only reuse
m_cachedRange if it’s identical to what’s computed.

But since a Range is a mutable object, I’d like to understand what the
semantics are if the caller changes the value of the Range after receiving it.
Will we modify the selection if that is done (I doubt it)? Do we need to keep
returning the same Range even though its value is now incorrect?