Bug 51006 – Range::processContents needs cleanup

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

Range::processContents is one giant function that implements multiple features for Range.  We should split up this function into smaller pieces.

[Ryosuke Niwa]

I'm seeing some similarities between processContents and StyledMarkupAccumulator::serializeNodes but I'm not sure how we can share code.

[Ryosuke Niwa]

Created attachment 82761 [details]
Patch

[Ryosuke Niwa]

Could someone review my patch?

[Kent Tamura]

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]

(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 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 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]

(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 on attachment 82761 [details]
Patch

Clearing flags on attachment: 82761

Committed r79854: <http://trac.webkit.org/changeset/79854>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.