Bug 54282

Summary: Extract a function to process contents for one node from Range::processContents
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Enhancement CC: ap, darin, koivisto, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 51006    
Attachments:
Description Flags
Patch
none
Fixed per Darin's comments darin: review+

Description Ryosuke Niwa 2011-02-11 05:22:13 PST
This is a cleanup of Range::processContents.
Comment 1 Ryosuke Niwa 2011-02-11 05:40:42 PST
Created attachment 82125 [details]
Patch
Comment 2 Darin Adler 2011-02-11 13:03:39 PST
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.
Comment 3 Ryosuke Niwa 2011-02-11 14:15:12 PST
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.
Comment 4 Ryosuke Niwa 2011-02-11 14:19:42 PST
Created attachment 82176 [details]
Fixed per Darin's comments
Comment 5 Darin Adler 2011-02-11 14:45:51 PST
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.
Comment 6 WebKit Review Bot 2011-02-11 22:04:43 PST
Attachment 82176 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7868815
Comment 7 Ryosuke Niwa 2011-02-12 01:10:53 PST
Committed r78409: <http://trac.webkit.org/changeset/78409>