Bug 43227 - Group functions used in createMarkup (range version) into a class so they are easier to understand
Summary: Group functions used in createMarkup (range version) into a class so they are...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 43834
  Show dependency tree / graph
 
Reported: 2010-07-29 18:13 PDT by Ryosuke Niwa
Modified: 2010-08-11 00:42 PDT (History)
5 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.