[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 #18 from Kaustubh Atrawalkar <kaustubh at motorola.com>  2011-09-03 05:33:20 PST ---
Thanks Eric for the review. Please see the reply below.

(In reply to comment #16)
> (From update of attachment 106141 [details])
> 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.
> 

Added "No New Tests. Code Re-factoring" as per the bug.

> > Source/WebCore/dom/Range.cpp:7
> > + * Portions  (C) 2011 Motorola Mobility. All rights reserved.
> 
> I think folks normally just say "Copyright"
> 

OK. I thought if you change very small portion of the whole file u need to add "Portions". But anyway changed it to "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.
> 

Removed unnecessary 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.
> 

Incorporated this change.

> > 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."
> 

Agree with ambiguous "still" word. I have removed that.

> > 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?
> 

The API createDocumentFragmentForElement is non-static API. We can not call it directly without creating object of Range. Previously, it was called on the fakebody element. ( the implementation was in HTMLElement.cpp which is now removed).

> > 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?

Just added one more check. But realized after Ryosuke's comment its not useful. Changed the line to - 

    if(!m_element->document())
        return;

    RefPtr<DocumentFragment> fragment =  Range::create(m_element->document)->createDocumentFragmentForElement(markup, toHTMLElement(m_element));

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