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+

Ryosuke Niwa

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

Attachments
cleanup proposal (11.92 KB, patch) 2010-07-29 21:05 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Patch (13.66 KB, patch) 2010-07-31 19:23 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Renamed wrapWithStyleSpan to wrapWithStyleNode (13.66 KB, patch) 2010-07-31 19:39 PDT, Ryosuke Niwa	tkent: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Darin Adler

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

Why must it be centralized?

Ryosuke Niwa

Comment 2 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.

Darin Adler

Comment 3 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".

Ryosuke Niwa

Comment 4 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.

Ryosuke Niwa

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

Created attachment 63167 [details] Patch

Ryosuke Niwa

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

Created attachment 63168 [details] Renamed wrapWithStyleSpan to wrapWithStyleNode

Kent Tamura

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

Ryosuke Niwa

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

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

Ojan Vafai

Comment 9 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?

Ryosuke Niwa

Comment 10 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.

Note You need to log in before you can comment on or make changes to this bug.