Bug 43405 - Extract a function that serializes nodes from the range version of createMarkup
Summary: Extract a function that serializes nodes from the range version of createMarkup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Enhancement
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-03 01:06 PDT by Ryosuke Niwa
Modified: 2010-08-03 18:09 PDT (History)
4 users (show)

See Also:


Attachments
extracted serializeNodes (9.58 KB, patch)
2010-08-03 01:21 PDT, Ryosuke Niwa
ojan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2010-08-03 01:06:56 PDT
The range version of createMarkup currently does three things:
1. Disable "delete button" and adjust the starting node
2. Serialize nodes
3. Add style spans / divs to the serialized nodes to preserve styles.

As a result, createMarkup is large is a large function with plenty of local variables.
Because step 2 is straight forward operation, I propose to extract it as a function.
Comment 1 Ryosuke Niwa 2010-08-03 01:21:43 PDT
Created attachment 63308 [details]
extracted serializeNodes
Comment 2 Tony Chang 2010-08-03 11:09:07 PDT
Comment on attachment 63308 [details]
extracted serializeNodes

> Index: WebCore/editing/markup.cpp
> ===================================================================
> +static Node* serializeNodes(MarkupAccumulatorWrapper& accumulator, Node* startNode, Node* pastEnd, Vector<Node*>* nodes, const Range* range, EAnnotateForInterchange annotate, EAbsoluteURLs absoluteURLs)

Nit: Can we make |startNode| or |pastEnd| const?
Comment 3 Ryosuke Niwa 2010-08-03 13:22:22 PDT
Thanks for the review ojan!

(In reply to comment #2)
> (From update of attachment 63308 [details])
> > Index: WebCore/editing/markup.cpp
> > ===================================================================
> > +static Node* serializeNodes(MarkupAccumulatorWrapper& accumulator, Node* startNode, Node* pastEnd, Vector<Node*>* nodes, const Range* range, EAnnotateForInterchange annotate, EAbsoluteURLs absoluteURLs)
> 
> Nit: Can we make |startNode| or |pastEnd| const?

Will fix and commit.
Comment 4 Ryosuke Niwa 2010-08-03 13:38:21 PDT
(In reply to comment #3)
> > Nit: Can we make |startNode| or |pastEnd| const?
> 
> Will fix and commit.

As it turns out, this is not possible because startNode is assigned to n, which is passed to various functions which requires n to be non-const.  pastEnd is also assigned to next so we can't make pastEnd const either.
Comment 5 Darin Adler 2010-08-03 14:30:52 PDT
Generally speaking I don't think we should ever use const Node* -- it doesn't make sense to say that a node in a tree is "const". What about the entire rest of the tree? Why is it OK for me to get a non-const pointer to my parent given a const pointer to myself. I think const has limited usefulness on DOM nodes and render tree nodes.
Comment 6 Ryosuke Niwa 2010-08-03 18:09:36 PDT
Committed r64609: <http://trac.webkit.org/changeset/64609>