Summary: | FormatBlockCommand's modifyRange and doApply should be merged | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||
Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Enhancement | CC: | darin, enrica, ojan, rniwa, tkent, tony | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 19795 | ||||||
Attachments: |
|
Description
Ryosuke Niwa
2010-09-24 13:24:35 PDT
Created attachment 68741 [details]
Patch
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. (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? Oops, I meant
> if range and startOfParagraph(visibleStart) != startOfLastParagraph
for the first case.
(In reply to comment #3) > > Does this clarify your question? I see, yes that makes sense. LGTM. Committed r68337: <http://trac.webkit.org/changeset/68337> |