[Webkit-unassigned] [Bug 67056] Code Refactoring for HTMLElement::deprecatedCreateContextualFragment

[bugzilla-daemon at webkit.org]

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


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #106141|review?                     |review-
               Flag|                            |




--- Comment #16 from Eric Seidel <eric at webkit.org>  2011-09-02 12:13:44 PST ---
(From update of attachment 106141)
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?

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