Bug 36430 – InsertListCommand needs cleanup

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Currently, InsertListCommand has three major problems:

1. If the selection contains more than one paragraphs, modifyRange() loops over each paragraph and calls doApply.  This is very misleading because doApply() and modifyRange() aren't really mutually recursive functions (doApply() calls modifyRange() at most once when doApply() is called for the first time).

2. doApply() has two parts; the first part removes the child node and the second part adds list. Although there are cases in which we remove a list element and add a different type of list element later, two parts are sufficiently isolated and they should be separate functions.

3. IndentOutdentCommand and InsertListCommand implements very similar features but do not share much code.  IndentOutdentCommand adds and removes list just like InsertListCommand, and the only difference is that InsertListCommand sometimes needs to wrap a paragraph with LI whereas IndentOutdentCommand wraps with a blockquote in such cases.  Two classes should share more code.

Proposal: we first divide doApply into two separate functions/methods.  We then move the code inside of modifyRange into doApply.  We can then combine code in IndentOutdentCommand and InsertListCommand.

[Ryosuke Niwa]

Created attachment 51386 [details]
first attempt to separate listifying/unlistifying code

This is my first attempt to separate two parts of doApply.  Maybe it makes sense to file another but just to land this patch first.  Svn diff is messy because it contains the changes to 19539 as well.

[Tony Chang]

Seems like a good refactoring to me.  The original method was long and the new function names help to clarify what the code is trying to do.

[Ryosuke Niwa]

Created attachment 57889 [details]
Isolates code for listifying/unlistifying paragraphs into seperate functions.

[Justin Garcia]

Comment on attachment 57889 [details]
Isolates code for listifying/unlistifying paragraphs into seperate functions.

r=me

[Ryosuke Niwa]

Landed in http://trac.webkit.org/changeset/60877.