[webkit-reviews] review granted: [Bug 54282] Extract a function to process contents for one node from Range::processContents : [Attachment 82125] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 54282: Extract a function to process contents for one node from
Range::processContents
https://bugs.webkit.org/show_bug.cgi?id=54282

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=82125&action=review

Nice to not repeat so much code twice. What level of test coverage do we have
for this function?

I’ll say r=me, but I am not happy about the new use of UINT_MAX as a magic
number.

> Source/WebCore/dom/Range.cpp:41
> +#include <limits.h>

Normally we’d use numeric_limits<unsigned>::max() rather than UINT_MAX. But
given the way you are using it for a special meaning as an argument to the
function, I suggest a named constant that states that special meaning.

Is the UINT_MAX behavior really needed? Can’t the caller instead compute the
correct value to pass? We could provide a helper function for that.

> Source/WebCore/dom/Range.cpp:604
> +    while (node->parentNode() != commonRoot)
> +	   node = node->parentNode();

Looks to me like this can crash if node isn’t actually inside commonRoot. Maybe
the function should assert that on entry.

> Source/WebCore/dom/Range.cpp:854
> +		   result->appendChild(n, ec); // will remove n from its parent


Could do nodes[i].release() here and avoid a bit of reference count churn.

> Source/WebCore/dom/Range.h:150
> +    PassRefPtr<Node> processContentsBetweenOffsets(ActionType,
ExceptionCode&, PassRefPtr<DocumentFragment>, Node*, unsigned startOffset,
unsigned endOffset);

Normally ExceptionCode& goes last after all the in arguments, and it makes
sense to do that even in a case like this.