[webkit-reviews] review granted: [Bug 132796] QualifiedName should use RefPtr<QualifiedNameImpl> internally. : [Attachment 231250] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andreas Kling <akling at apple.com>'s
request for review:
Bug 132796: QualifiedName should use RefPtr<QualifiedNameImpl> internally.
https://bugs.webkit.org/show_bug.cgi?id=132796

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=231250&action=review


These are the Windows build errors:

     1>..\dom\QualifiedName.cpp(100): error C2734: 'WebCore::anyName' : const
object must be initialized if not extern
     1>..\dom\QualifiedName.cpp(100): warning C4211: nonstandard extension used
: redefined extern to static
     1>..\dom\QualifiedName.cpp(100): error C2512: 'WebCore::QualifiedName' :
no appropriate default constructor available

Looks good as long as you resolve that before landing. I think
QNAME_DEFAULT_CONSTRUCTOR was doing some good on Windows.

> Source/WebCore/dom/QualifiedName.cpp:81
>      QualifiedNameComponents components = { p.impl(), l.impl(), n.isEmpty() ?
nullAtom.impl() : n.impl() };

Isn’t nullAtom.impl() the same thing as nullptr?

> Source/WebCore/dom/QualifiedName.cpp:83
> +    m_impl = addResult.isNewEntry ? adoptRef(*addResult.iterator) :
*addResult.iterator;

Can we make an inline helper function to do this work? Then we can use
construction instead of assignment for m_impl, and possibly even save a branch.


I also think we could go nuts and use words for the three arguments instead of
letters.

> Source/WebCore/dom/QualifiedName.h:74
> +    explicit QualifiedName(WTF::HashTableDeletedValueType) :
m_impl(WTF::HashTableDeletedValue) { }

Are the explicit WTF prefixes needed here? Fine to leave them in if they are
indeed needed.

> Source/WebCore/dom/QualifiedName.h:76
> +    ~QualifiedName() { }

I believe the best way to get this effect, a public empty destructor that gets
inlined, is to omit explicit declaration or definition of the destructor
entirely.

> Source/WebCore/dom/QualifiedName.h:78
>      QualifiedName() : m_impl(0) { }

Maybe nullptr this while you are in the neighborhood.