Bug 54282

Created attachment 82125 [details] Patch

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.

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.

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

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.

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

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