[webkit-reviews] review granted: [Bug 91703] Use the original token to create an element in "reconstruct the active formatting elements" and "call the adoption agency" : [Attachment 153783] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Kwang Yul Seo
<skyul at company100.net>'s request for review:
Bug 91703: Use the original token to create an element in "reconstruct the
active formatting elements" and "call the adoption agency"
https://bugs.webkit.org/show_bug.cgi?id=91703

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=153783&action=review


Thanks.  This is so much easier to read.  A couple minor points below, but this
looks great.

> Source/WebCore/html/parser/HTMLFormattingElementList.cpp:92
> +    RefPtr<HTMLStackItem> newItem = prpNewItem;

Ideally we would call newItem.release() at some point in this function to save
a ref()/deref() pair.  Can we call release on line 103?

> Source/WebCore/html/parser/HTMLFormattingElementList.h:76
> -	   bool operator==(Element* element) const { return m_element ==
element; }
> -	   bool operator!=(Element* element) const { return m_element !=
element; }
> +	   bool operator==(Element* element) const { return !m_item ? !element
: m_item->element() == element; }
> +	   bool operator!=(Element* element) const { return !m_item ? !!element
: m_item->element() != element; }

This looks slightly complicated, but it seems to make sense.

> Source/WebCore/html/parser/HTMLFormattingElementList.h:115
> -    void swapTo(Element* oldElement, Element* newElement, const Bookmark&);
> +    void swapTo(Element* oldElement, PassRefPtr<HTMLStackItem> prpNewItem,
const Bookmark&);

Normally we skip the "prp" prefix in the header.  It's useful in the
implementation because the PassRefPtr variables can become null easily.

> Source/WebCore/html/parser/HTMLStackItem.h:50
> +    static PassRefPtr<HTMLStackItem> create(PassRefPtr<ContainerNode> node,
AtomicHTMLToken* token, const AtomicString& namespaceURI =
HTMLNames::xhtmlNamespaceURI)

This should be PassRefPtr<AtomicHTMLToken> because we're going to end up taking
a reference to token.  The point of using PassRefPtr in these cases is so that
our caller can donate a reference (using release()) if they don't need their
reference anymore.  That saves us a ref()/deref() pair.

> Source/WebCore/html/parser/HTMLStackItem.h:56
> +    static PassRefPtr<HTMLStackItem> create(Element* element,
AtomicHTMLToken* token, const AtomicString& namespaceURI =
HTMLNames::xhtmlNamespaceURI)

|element| and |AtomicHTMLToken| should use PassRefPtr.

> Source/WebCore/html/parser/HTMLStackItem.h:61
> +    Element* element() const { return toElement(m_node.get()); }

Does toElement have any cost?  It it just an ASSERT plus a static_cast?