Bug 46504 – FormatBlockCommand's modifyRange and doApply should be merged

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

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 68741 [details]
Patch

[Tony Chang]

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.

[Ryosuke Niwa]

(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?

[Ryosuke Niwa]

Oops, I meant
> if range and startOfParagraph(visibleStart) != startOfLastParagraph 
for the first case.

[Tony Chang]

(In reply to comment #3)
> 
> Does this clarify your question?

I see, yes that makes sense.  LGTM.

[Ryosuke Niwa]

Committed r68337: <http://trac.webkit.org/changeset/68337>