[webkit-reviews] review granted: [Bug 206314] Use Visible Position to calculate Positions for highlights : [Attachment 388101] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jan 17 16:29:12 PST 2020
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.
More information about the webkit-reviews
mailing list