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

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied 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 387845: Patch

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




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

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

> Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:85
> +	   auto visibleSelection =
VisibleSelection(Range::create(*rangeGroup->m_document.get(), data->range));

It makes no sense to create a Range just so that we can create
VisibleSelection. r- because of this.
Once again, Range is a very expensive object to create so we should avoid
making one.
Just add a new constructor of VisibleSelection which takes StaticRange instead.
Also, what guarantees that boundary points of StaticRange still point to nodes
in the same document at this point?

> Source/WebCore/Modules/highlight/HighlightRangeGroup.h:49
> +    HighlightRangeData(StaticRange&& range)

Nit: Please define this before all data members and place a blank line
between member functions & member variables to visually separate them.

> Source/WebCore/Modules/highlight/HighlightRangeGroup.h:71
> +    

Nit: whitespace.

> Source/WebCore/editing/TextManipulationController.cpp:-209
> -

This seems like an unrelated change. Please revert.

> Source/WebCore/rendering/InlineTextBox.cpp:1055
> +		   

Ditto. Revert.

> Source/WebCore/rendering/InlineTextBox.cpp:1060
> +		       highlightData.setContext({startRenderer, endRenderer,
static_cast<unsigned>(startOffset), static_cast<unsigned>(endOffset)});

Ditto.

> Source/WebCore/rendering/SelectionRangeData.cpp:170
> -    if (&renderer == m_selectionContext.start() ||
renderer.isDescendantOf(m_selectionContext.start())) {
> +    if (&renderer == m_selectionContext.start() /*||
renderer.isDescendantOf(m_selectionContext.start())*/) {

What's happening with this? We don't commit commented out code like this.

> Source/WebCore/rendering/SelectionRangeData.cpp:176
> -    if (&renderer == m_selectionContext.end() ||
renderer.isDescendantOf(m_selectionContext.end()))
> +    if (&renderer == m_selectionContext.end() /*||
renderer.isDescendantOf(m_selectionContext.end())*/)

Ditto.