Bug 43227 – Group functions used in createMarkup (range version) into a class so they are easier to understand

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

[Ryosuke Niwa]

In the range version of createMarkup (http://trac.webkit.org/browser/trunk/WebCore/editing/markup.cpp#L797),
we have preMarkup and markup to accumulate text but this should be centralized as in the node version
(http://trac.webkit.org/browser/trunk/WebCore/editing/markup.cpp#L1075).

[Darin Adler]

Why must it be centralized?

[Ryosuke Niwa]

Created attachment 63025 [details]
cleanup proposal

(In reply to comment #1)
> Why must it be centralized?

Because there are many static functions in markup.cpp, and it's hard to understand the relationship between functions and two createMarkup functions.
So I'd like to introduce accumulator (wrapper for now) for the range version first, and gradually increase and organize the shared code between two functions.

See the attachment for what I mean.

[Darin Adler]

My title for the bug would be something more like "Group markup functions into a class so they are easier to understand".

[Ryosuke Niwa]

(In reply to comment #3)
> My title for the bug would be something more like "Group markup functions into a class so they are easier to understand".

Done.

[Ryosuke Niwa]

Created attachment 63167 [details]
Patch

[Ryosuke Niwa]

Created attachment 63168 [details]
Renamed wrapWithStyleSpan to wrapWithStyleNode

[Kent Tamura]

Comment on attachment 63168 [details]
Renamed wrapWithStyleSpan to wrapWithStyleNode

WebCore/editing/markup.cpp:815
 +  private:
nit: We usually have a blank line before an access label not at the beginning of the class.

[Ryosuke Niwa]

Committed r64477: <http://trac.webkit.org/changeset/64477>

[Ojan Vafai]

> +        (WebCore::MarkupAccumulatorWrapper::insertString): Added.
> +        (WebCore::MarkupAccumulatorWrapper::insertOpenTag): Added.
> +        (WebCore::MarkupAccumulatorWrapper::insertEndTag): Added.

How about calling these append, appendOpenTag and appendEndTag?

[Ryosuke Niwa]

(In reply to comment #9)
> > +        (WebCore::MarkupAccumulatorWrapper::insertString): Added.
> > +        (WebCore::MarkupAccumulatorWrapper::insertOpenTag): Added.
> > +        (WebCore::MarkupAccumulatorWrapper::insertEndTag): Added.
> 
> How about calling these append, appendOpenTag and appendEndTag?

I thought about it but the problem is that they don't really "append" the node to the very end.  They're opening or closing the inner most node (possibly following other nodes) wrapped by outer nodes that have already been added by createMarkup.  So "append" might be misleading here.