[webkit-reviews] review granted: [Bug 206314] Use Visible Position to calculate Positions for highlights : [Attachment 388101] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 206314: Use Visible Position to calculate Positions for highlights
https://bugs.webkit.org/show_bug.cgi?id=206314

Attachment 388101: Patch

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




--- Comment #11 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 388101
  --> https://bugs.webkit.org/attachment.cgi?id=388101
Patch

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

> Source/WebCore/dom/Document.cpp:2751
> +		   if ((!rangeData->startPosition.hasValue() ||
!rangeData->endPosition.hasValue())
> +		       && (&rangeData->range->startContainer()->treeScope() ==
&rangeData->range->endContainer()->treeScope()))
> +		       rangesData.append(makeWeakPtr(rangeData.ptr()));

This is a complicated expression. Can we continue early instead? i.e.
if (!rangeData->startPosition || !rangeData->endPosition)
    continue;
if (rangeData->range->startContainer()->treeScope() !=
&rangeData->range->endContainer()->treeScope())
    continue;
rangesData.append(makeWeakPtr(rangeData.ptr()));

> Source/WebCore/dom/Document.cpp:2765
> +	       if (weakRangeData.get()) {

Continue when !weakRangeData instead?

> Source/WebCore/dom/Document.cpp:2766
> +		   if (!rangeData->startPosition.hasValue())

I don't think we need to check this since it's unlikely that
Page::updateRendering would have ran by updating the layout.
Even if did, there is no harm in re-setting to the new position value.

> Source/WebCore/dom/Document.cpp:2767
> +		       rangeData->startPosition =
visibleSelection.visibleStart().deepEquivalent();

Need to do this instead: rangeData->startPosition = startPosition

> Source/WebCore/dom/Document.cpp:2768
> +		   if (!rangeData->endPosition.hasValue())

Ditto.

> Source/WebCore/dom/Document.cpp:2769
> +		       rangeData->endPosition =
visibleSelection.visibleEnd().deepEquivalent();

Ditto.

> Source/WebCore/rendering/InlineTextBox.cpp:1047
> +	       if (rangeData->startPosition.hasValue() &&
rangeData->endPosition.hasValue()) {

Nit: It's more succinct to say: rangeData->startPosition &&
rangeData->endPosition.