[webkit-reviews] review denied: [Bug 78361] Selectively retrieve text content around a given position : [Attachment 126522] 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 126522: Patch
https://bugs.webkit.org/attachment.cgi?id=126522&action=review

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


> Source/WebCore/dom/DOMTextContentWalker.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
AND ANY

Wrong copyright statement. Please use the "correct" one.

> Source/WebCore/dom/DOMTextContentWalker.cpp:32
> +#include "TextIterator.h"
> +#include "VisiblePosition.h"
> +#include "VisibleSelection.h"
> +#include "visible_units.h"

This class heavily depends on editing. I don't think it should be in DOM. It
belongs in editing.

> Source/WebCore/dom/DOMTextContentWalker.cpp:36
> +static PassRefPtr<Range> getRange(const Position& start, const Position&
end)

In WebKit, we only use "get"~ if the value is returned through
pass-by-reference or pass-by-pointer in the argument list.
Also, "getRange" is an extremely generic name.

> Source/WebCore/dom/DOMTextContentWalker.cpp:38
> +    return VisibleSelection(start.parentAnchoredEquivalent(),
end.parentAnchoredEquivalent(), DOWNSTREAM).firstRange();

Why not Range::create(start.parentAnchoredEquivalent(),
end.parentAnchoredEquivalent()) ?

> Source/WebCore/dom/DOMTextContentWalker.cpp:45
> +    CharacterIterator forwardChar(makeRange(position,
endOfDocument(position)).get(), TextIteratorStopsOnFormControls);

This isn't "char", it's an iterator.

> Source/WebCore/dom/DOMTextContentWalker.h:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
AND ANY

Ditto about "Apple Inc."

> Source/WebCore/dom/DOMTextContentWalker.h:36
> +// Explore the DOM tree to find the text contents up to a limit
> +// around a position in a given text node.

We don't normally add a comment like this to explain what a class does.
Instead, we give a descriptive name to the class such that it's self-evident.

> Source/WebCore/dom/DOMTextContentWalker.h:37
> +class DOMTextContentWalker {

This isn't really a "walker" class. It obtains text around a given visible
position. It's certainly not "DOM" because the prefix "DOM" indicates that it's
exposed to DOM, which isn't case at all here.
How about SurroundingText?

> Source/WebCore/dom/DOMTextContentWalker.h:45
> +    // Convert start/end positions in the content text string into a text
range.

This comment repeats the code, please remove and perhaps rename arguments to
startOffsetInContent and endOffsetInContent.

> Source/WebCore/editing/TextIterator.cpp:243
> +static bool checkFormControlElement(Node* startNode)

What are you "checking" here? It should renamed to
enclosedByFormControlElement.

> Source/WebCore/editing/TextIterator.cpp:247
> +	   if (node->isElementNode() &&
static_cast<Element*>(node)->isFormControlElement())

Don't we have toElement?

> Source/WebCore/editing/TextIterator.cpp:399
> +	   if (!m_shouldStop && m_stopsOnFormControls &&
checkFormControlElement(m_node))

r- because TextIterator is in extremely hot path and we shouldn't be walking up
the tree on every node like this.