Bug 41403 – InsertListCommand's modifyRange and doApply should be merged

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

InsertListCommand's doApply mainly implements insertion and removal of lists in single paragraph. To handle a selection with multiple paragraphs, doApply calls modifyRange, which iterates over paragraphs in the selection and calls doApply on each paragraph.  doApply calls modifyRange whenever the selection is a range object and returns except when the starting and the ending of the selection is in the same paragraph in which case modifyRange bails out by returning false and doApply proceed with its list insertion & removal.  However, this peculiar structure makes the code harder to read and it has been an obstacle in debugging the bug 32422.

I propose to merge InsertListCommand's modifyRange with doApply and isolate the logic for single paragraph case into a separate method.

[Ryosuke Niwa]

Created attachment 60463 [details]
merges modifyRange and doApply and adds doApplyForSingleParagraph

I'm not happy about the fact doApplyForSingleParagraph relies on the selection but just passing the starting/ending of paragraph breaks some tests because moveParagraph makes some visible positions invalid by detaching it from the document.  So that should be done in a separate patch by replacing moveParagraph by moveParagraphWithClones.

[Kent Tamura]

Comment on attachment 60463 [details]
merges modifyRange and doApply and adds doApplyForSingleParagraph

Looks OK.
Please make sure all layout tests pass with this change.

[WebKit Commit Bot]

Comment on attachment 60463 [details]
merges modifyRange and doApply and adds doApplyForSingleParagraph

Clearing flags on attachment: 60463

Committed r63189: <http://trac.webkit.org/changeset/63189>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Created attachment 68740 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 68740 [details]
Patch

Oops, wrong bug.