Bug 54282 – Extract a function to process contents for one node from Range::processContents

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

This is a cleanup of Range::processContents.

[Ryosuke Niwa]

Created attachment 82125 [details]
Patch

[Darin Adler]

Comment on attachment 82125 [details]
Patch

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.

[Ryosuke Niwa]

Thanks for the review.  I'll resubmit new patch for a review because I'm making quite few changes.

(In reply to comment #2)
> Nice to not repeat so much code twice. What level of test coverage do we have for this function?

We have quite few now in fast/dom/Range.  I had encountered several crashes and fails while I was refactoring this code so I'd trust that they have a decent coverage.

> 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.

I'm not happy about that either.  However, I was more concerned about having two identical switch statement's cases that need to be maintained to be same.  I think that's unnecessarily error-prone.  I also considered using template to generate 3 versions of functions but that seemed to bloat the code unnecessary.  I added LengthOfContentsInNode using numeric_limits<unsigned>::max()

> > 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.

Thanks for pointing that out.  I was intending to add an assertion there and forgot.

> > 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.

Good idea.  Got rid of local variable n altogether.

> > 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.

Fixed.

[Ryosuke Niwa]

Created attachment 82176 [details]
Fixed per Darin's comments

[Darin Adler]

Comment on attachment 82176 [details]
Fixed per Darin's comments

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

> Source/WebCore/dom/Range.cpp:610
> +static unsigned LengthOfContentsInNode = numeric_limits<unsigned>::max();

Should be const.

[WebKit Review Bot]

Attachment 82176 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7868815

[Ryosuke Niwa]

Committed r78409: <http://trac.webkit.org/changeset/78409>