Bug 46504 - FormatBlockCommand's modifyRange and doApply should be merged
Summary: FormatBlockCommand'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: All All
: P2 Enhancement
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 19795
  Show dependency treegraph
 
Reported: 2010-09-24 13:24 PDT by Ryosuke Niwa
Modified: 2010-09-25 13:42 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.88 KB, patch)
2010-09-24 13:59 PDT, Ryosuke Niwa
tony: 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-09-24 13:24:35 PDT
FormatBlockCommand'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 range 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.  This bug is similar to https://bugs.webkit.org/show_bug.cgi?id=41403.

I propose to merge FormatBlockCommand's modifyRange with doApply and isolate the logic for single paragraph case into a separate function.
Comment 1 Ryosuke Niwa 2010-09-24 13:59:07 PDT
Created attachment 68741 [details]
Patch
Comment 2 Tony Chang 2010-09-24 17:20:48 PDT
Comment on attachment 68741 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68741&action=review

In general, this seems fine.  It looks like it reduces the amount of recursion used, which is also good.

> WebCore/editing/FormatBlockCommand.cpp:71
> +    if (endingSelection().isCaret() || startOfParagraph(visibleStart) == startOfLastParagraph) {
> +        doApplyForSingleParagraph();
>          return;
> +    }

Can you comment on why you added the isCaret() check?  It seems like it's a new condition.
Comment 3 Ryosuke Niwa 2010-09-24 17:32:57 PDT
(In reply to comment #2)
> (From update of attachment 68741 [details])
> > WebCore/editing/FormatBlockCommand.cpp:71
> > +    if (endingSelection().isCaret() || startOfParagraph(visibleStart) == startOfLastParagraph) {
> > +        doApplyForSingleParagraph();
> >          return;
> > +    }
> 
> Can you comment on why you added the isCaret() check?  It seems like it's a new condition.

So we used to call doModify when endingSelection().isRange() returned true, and proceeded with our implementation of format block for single paragraph otherwise.  In my patch, I'm exchanging the order so that we early exit when we have a caret and handle the multiple paragraphs case otherwise.

i.e. we used to have

if range and startOfParagraph(visibleStart) == startOfLastParagraph (this condition is checked inside modifyRange):
   call doApply on each paragraph in the selection
else:
  formatBlock on single paragraph

I'm changing it to:

if caret or startOfParagraph(visibleStart) == startOfLastParagraph:
  do formatBlock on single paragraph
else:
  do formatBlock on each paragraph in the selection.

Does this clarify your question?
Comment 4 Ryosuke Niwa 2010-09-24 17:34:02 PDT
Oops, I meant
> if range and startOfParagraph(visibleStart) != startOfLastParagraph 
for the first case.
Comment 5 Tony Chang 2010-09-24 17:44:51 PDT
(In reply to comment #3)
> 
> Does this clarify your question?

I see, yes that makes sense.  LGTM.
Comment 6 Ryosuke Niwa 2010-09-25 13:42:59 PDT
Committed r68337: <http://trac.webkit.org/changeset/68337>