Summary: | Extract a function to process contents for one node from Range::processContents | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
Component: | DOM | Assignee: | 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
Ryosuke Niwa
2011-02-11 05:22:13 PST
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> |