Bug 41403 - InsertListCommand's modifyRange and doApply should be merged
Summary: InsertListCommand's modifyRange and doApply should be merged
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on: 36430
Blocks: 32422
  Show dependency treegraph
 
Reported: 2010-06-30 04:49 PDT by Ryosuke Niwa
Modified: 2010-09-24 13:58 PDT (History)
7 users (show)

See Also:


Attachments
merges modifyRange and doApply and adds doApplyForSingleParagraph (8.19 KB, patch)
2010-07-03 18:06 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (5.88 KB, text/plain)
2010-09-24 13:57 PDT, Ryosuke Niwa
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2010-06-30 04:49:30 PDT
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.
Comment 1 Ryosuke Niwa 2010-07-03 18:06:09 PDT
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.
Comment 2 Kent Tamura 2010-07-11 22:45:11 PDT
Comment on attachment 60463 [details]
merges modifyRange and doApply and adds doApplyForSingleParagraph

Looks OK.
Please make sure all layout tests pass with this change.
Comment 3 WebKit Commit Bot 2010-07-13 03:12:09 PDT
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>
Comment 4 WebKit Commit Bot 2010-07-13 03:12:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Ryosuke Niwa 2010-09-24 13:57:21 PDT
Created attachment 68740 [details]
Patch
Comment 6 Ryosuke Niwa 2010-09-24 13:58:05 PDT
Comment on attachment 68740 [details]
Patch

Oops, wrong bug.