| Summary: | Range::processContents needs cleanup | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||
| Component: | DOM | Assignee: | 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
Ryosuke Niwa
2010-12-13 21:11:10 PST
I'm seeing some similarities between processContents and StyledMarkupAccumulator::serializeNodes but I'm not sure how we can share code. Created attachment 82761 [details]
Patch
Could someone review my patch? 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. (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 :-) 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. 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. (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 :) Comment on attachment 82761 [details] Patch Clearing flags on attachment: 82761 Committed r79854: <http://trac.webkit.org/changeset/79854> All reviewed patches have been landed. Closing bug. |