[webkit-reviews] review denied: [Bug 78361] Selectively retrieve text content around a given position : [Attachment 131616] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Leandro Graciá Gil
<leandrogracia at chromium.org>'s request for review:
Bug 78361: Selectively retrieve text content around a given position
https://bugs.webkit.org/show_bug.cgi?id=78361

Attachment 131616: Patch
https://bugs.webkit.org/attachment.cgi?id=131616&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=131616&action=review


> Source/WebCore/ChangeLog:10
> +	   Reviewed by NOBODY (OOPS!).

This line needs to appear before the description.

> Source/WebCore/editing/SurroundingText.cpp:22
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1.  Redistributions of source code must retain the above copyright
> + *	  notice, this list of conditions and the following disclaimer.
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *	  notice, this list of conditions and the following disclaimer in the
> + *	  documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED

> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS BE LIABLE
FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
AND ON
> + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
THIS
> + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

Please use the correct license. e.g. EditingStyle.h

> Source/WebCore/editing/SurroundingText.cpp:37
> +SurroundingText::SurroundingText(const VisiblePosition& position, unsigned
maxLength)

It's probably better to call this one visiblePosition.

> Source/WebCore/editing/SurroundingText.cpp:44
> +    // No forward contents, started inside form control.

I don't think this comment is useful (it only says what, not why; also the code
is self-evident as is). Please remove.

> Source/WebCore/editing/SurroundingText.cpp:45
> +    Position deepPosition = position.deepEquivalent();

And this one position.

> Source/WebCore/editing/SurroundingText.cpp:47
> +    if (!Range::create(document, deepPosition,
forwardIterator.range()->startPosition())->text().length())

You can't pass a position to Range unless it's range compliant. r- because of
this.
So you need to do deepPosition.parentAnchoredEquivalent() here.
You probably want to do this when you declare deepPosition.

> Source/WebCore/editing/SurroundingText.cpp:57
> +PassRefPtr<Range> SurroundingText::contentOffsetsToRange(unsigned
startOffsetInContent, unsigned endOffsetInContent)

It's probably better to call this function rangeFromContentOffsets.

> Source/WebCore/editing/TextIterator.cpp:245
> +static bool enclosedByFormControlElement(Node* startNode)
> +{
> +    Node* node = startNode;

You could just name startNode node, then you wouldn't have to declare a local
variable.

> Source/WebCore/editing/TextIterator.cpp:395
> +	   if (!m_shouldStop && m_stopsOnFormControls &&
enclosedByFormControlElement(m_node))
> +	       m_shouldStop = true;

Why do we want to proceed if we're already inside a form control element?
Shouldn't be just bail out here?
If you do that, we wouldn't need m_shouldStop. We can just m_node = 0 and
everything else should just work.