[Webkit-unassigned] [Bug 67056] Logic from HTMLElement::deprecatedCreateContextualFragment moved into Range::createContextualFragment function

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=67056





--- Comment #24 from Ryosuke Niwa <rniwa at webkit.org>  2011-09-04 11:04:40 PST ---
(From update of attachment 106288)
View in context: https://bugs.webkit.org/attachment.cgi?id=106288&action=review

> Source/WebCore/dom/Range.cpp:1100
> +static inline void insertIntoFragment(PassRefPtr<DocumentFragment> fragment, HTMLElement* element)

I don't think this function name is appropriate because element and its children are already in the fragment at this point. What this function is doing is to remove element preserving children.  I'd say removeElementPreservingChildren will be a better name (I'm kind of sad about the fact we can't share code with RemoveNodePreservingChildrenCommand).

> Source/WebCore/dom/Range.cpp:1104
> +    Node* firstChild = element->firstChild();

I don't see any benefit in declaring in this variable.  You should just do:
for (RefPtr<Node> child = element->firstChild(); child; child = nextChild)

> Source/WebCore/dom/Range.cpp:1133
> +    else {
> +        if (!fragment->parseXML(markup, element, scriptingPermission))

This should be a single else-if

> Source/WebCore/dom/Range.cpp:1135
> +            // FIXME: We should propagate a syntax error exception out here.
> +            return 0;

This comment should either be on the same line as return 0; or be wrapped by curly brackets (because the comment & return 0 occupy 2 lines).

> Source/WebCore/dom/Range.h:93
> +    PassRefPtr<DocumentFragment> createDocumentFragmentForElement(const String& markup, Element*,  FragmentScriptingPermission = FragmentScriptingAllowed);

This function can be static, no?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.