Bug 43227

Summary:

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

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

HTML Editing

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Enhancement

CC:

darin, enrica, justin.garcia, ojan, tony

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

43834

Attachments:

Description	Flags
cleanup proposal	none
Patch	none
Renamed wrapWithStyleSpan to wrapWithStyleNode	tkent: review+

Description Ryosuke Niwa 2010-07-29 18:13:43 PDT

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).

Comment 1 Darin Adler 2010-07-29 20:46:25 PDT

Why must it be centralized?

Comment 2 Ryosuke Niwa 2010-07-29 21:05:11 PDT

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.

Comment 3 Darin Adler 2010-07-29 23:25:39 PDT

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

Comment 4 Ryosuke Niwa 2010-07-29 23:51:10 PDT

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

Comment 5 Ryosuke Niwa 2010-07-31 19:23:32 PDT

Created attachment 63167 [details]
Patch

Comment 6 Ryosuke Niwa 2010-07-31 19:39:56 PDT

Created attachment 63168 [details]
Renamed wrapWithStyleSpan to wrapWithStyleNode

Comment 7 Kent Tamura 2010-08-01 18:53:01 PDT

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.

Comment 8 Ryosuke Niwa 2010-08-02 11:14:12 PDT

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

Comment 9 Ojan Vafai 2010-08-02 11:15:08 PDT

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

How about calling these append, appendOpenTag and appendEndTag?

Comment 10 Ryosuke Niwa 2010-08-02 13:48:15 PDT

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