[webkit-reviews] review denied: [Bug 67056] Code Refactoring for HTMLElement::deprecatedCreateContextualFragment : [Attachment 106141] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Kaustubh Atrawalkar
<kaustubh at motorola.com>'s request for review:
Bug 67056: Code Refactoring for HTMLElement::deprecatedCreateContextualFragment
https://bugs.webkit.org/show_bug.cgi?id=67056

Attachment 106141: Patch
https://bugs.webkit.org/attachment.cgi?id=106141&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=106141&action=review


> Source/WebCore/ChangeLog:9
> +	   No new tests. (OOPS!)

You should replace this with an explanation of why no tests are needed (just
refactoring) and remove the OOPS.

> Source/WebCore/dom/Range.cpp:7
> + * Portions	(C) 2011 Motorola Mobility. All rights reserved.

I think folks normally just say "Copyright"

> Source/WebCore/dom/Range.cpp:1102
> +    // Exceptions are ignored because none ought to happen here.

That's understood from the ASSERTs below.  You can just remove thsi comment.

> Source/WebCore/dom/Range.cpp:1170
> +    RefPtr<DocumentFragment> fragment =
createDocumentFragmentForElement(markup, static_cast<Element*>(element),
scriptingPermission);

We don't have a toElement cast?  the to* functions will check ->isElement
first, which is useful.

> Source/WebCore/editing/markup.cpp:686
> +    // Still using a fake body element here to trick the HTML parser to
using the

The "still" won't mean anything to anyone reading this comment later.  Should
just be removed.  "Use a fake body element to make the HTMLParser start in
InBody mode."

> Source/WebCore/editing/markup.cpp:688
> +    RefPtr<DocumentFragment> fragment = 
Range::create(document)->createDocumentFragmentForElement(markup,
document->body(), scriptingPermission);

This seems bad.  We're now doing a another malloc.  Why does this need to be on
Range?

> Source/WebKit/qt/Api/qwebelement.cpp:1027
> +    WebCore::Element* e = m_element;
> +    Document* doc = e ? e->document() : 0;
> +    if (!doc)

Why the local variable e?