Bug 36430 - InsertListCommand needs cleanup
Summary: InsertListCommand needs cleanup
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on: 35281 19539
Blocks: 32422 41403
  Show dependency treegraph
Reported: 2010-03-21 15:53 PDT by Ryosuke Niwa
Modified: 2010-06-30 04:49 PDT (History)
5 users (show)

See Also:

first attempt to separate listifying/unlistifying code (16.75 KB, patch)
2010-03-22 19:09 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Isolates code for listifying/unlistifying paragraphs into seperate functions. (17.60 KB, patch)
2010-06-04 09:49 PDT, Ryosuke Niwa
justin.garcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2010-03-21 15:53:34 PDT
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.
Comment 1 Ryosuke Niwa 2010-03-22 19:09:16 PDT
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.
Comment 2 Tony Chang 2010-03-25 02:20:23 PDT
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.
Comment 3 Ryosuke Niwa 2010-06-04 09:49:43 PDT
Created attachment 57889 [details]
Isolates code for listifying/unlistifying paragraphs into seperate functions.
Comment 4 Justin Garcia 2010-06-08 10:30:03 PDT
Comment on attachment 57889 [details]
Isolates code for listifying/unlistifying paragraphs into seperate functions.

Comment 5 Ryosuke Niwa 2010-06-08 21:00:16 PDT
Landed in http://trac.webkit.org/changeset/60877.