Bug 51006

Summary: Range::processContents needs cleanup
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Enhancement CC: abarth, commit-queue, darin, eae, eric, mitz, ojan, sam, tkent, tony
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 51005, 54282, 54425    
Bug Blocks:    
Attachments:
Description Flags
Patch none

Ryosuke Niwa
Reported 2010-12-13 21:11:10 PST
Range::processContents is one giant function that implements multiple features for Range. We should split up this function into smaller pieces.
Attachments
Patch (12.85 KB, patch)
2011-02-16 23:38 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2011-01-03 20:47:54 PST
I'm seeing some similarities between processContents and StyledMarkupAccumulator::serializeNodes but I'm not sure how we can share code.
Ryosuke Niwa
Comment 2 2011-02-16 23:38:23 PST
Created attachment 82761 [details] Patch
Ryosuke Niwa
Comment 3 2011-02-23 21:24:10 PST
Could someone review my patch?
Kent Tamura
Comment 4 2011-02-28 00:48:54 PST
Comment on attachment 82761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82761&action=review > Source/WebCore/dom/Range.cpp:630 > + // This switch statement must be consistent with that of Range::processContentsBetweenOffsets. So, we had better add a comment to Range::processContentBetweenoffsets(). > Source/WebCore/dom/Range.cpp:760 > + // This switch statement must be consistent with that of lengthOfContentsInNode. ditto.
Kent Tamura
Comment 5 2011-02-28 00:49:50 PST
(In reply to comment #4) > (From update of attachment 82761 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=82761&action=review > > > Source/WebCore/dom/Range.cpp:630 > > + // This switch statement must be consistent with that of Range::processContentsBetweenOffsets. > > So, we had better add a comment to Range::processContentBetweenoffsets(). > > > Source/WebCore/dom/Range.cpp:760 > > + // This switch statement must be consistent with that of lengthOfContentsInNode. > > ditto. Oh, I was confused. Ignore comments :-)
Ryosuke Niwa
Comment 6 2011-02-28 00:50:58 PST
Comment on attachment 82761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82761&action=review >> Source/WebCore/dom/Range.cpp:630 >> + // This switch statement must be consistent with that of Range::processContentsBetweenOffsets. > > So, we had better add a comment to Range::processContentBetweenoffsets(). I have. >> Source/WebCore/dom/Range.cpp:760 >> + // This switch statement must be consistent with that of lengthOfContentsInNode. > > ditto. This comment is added in processContentsBetweenOffsets, and refers back to lengthOfContentsInNode.
Ryosuke Niwa
Comment 7 2011-02-28 00:51:01 PST
Comment on attachment 82761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82761&action=review >> Source/WebCore/dom/Range.cpp:630 >> + // This switch statement must be consistent with that of Range::processContentsBetweenOffsets. > > So, we had better add a comment to Range::processContentBetweenoffsets(). I have. >> Source/WebCore/dom/Range.cpp:760 >> + // This switch statement must be consistent with that of lengthOfContentsInNode. > > ditto. This comment is added in processContentsBetweenOffsets, and refers back to lengthOfContentsInNode.
Ryosuke Niwa
Comment 8 2011-02-28 00:52:52 PST
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 82761 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=82761&action=review > > > > > Source/WebCore/dom/Range.cpp:630 > > > + // This switch statement must be consistent with that of Range::processContentsBetweenOffsets. > > > > So, we had better add a comment to Range::processContentBetweenoffsets(). > > > > > Source/WebCore/dom/Range.cpp:760 > > > + // This switch statement must be consistent with that of lengthOfContentsInNode. > > > > ditto. > > Oh, I was confused. Ignore comments :-) Ah, ok. Thanks for the review anyways :)
WebKit Commit Bot
Comment 9 2011-02-28 05:24:42 PST
Comment on attachment 82761 [details] Patch Clearing flags on attachment: 82761 Committed r79854: <http://trac.webkit.org/changeset/79854>
WebKit Commit Bot
Comment 10 2011-02-28 05:24:48 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.