Bug 132796 - QualifiedName should use RefPtr<QualifiedNameImpl> internally.
Summary: QualifiedName should use RefPtr<QualifiedNameImpl> internally.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-10 22:39 PDT by Andreas Kling
Modified: 2014-07-04 18:31 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.34 KB, patch)
2014-05-10 22:42 PDT, Andreas Kling
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2014-05-10 22:39:33 PDT
QualifiedName should use RefPtr<QualifiedNameImpl> internally. This'll be simpler and we can make ~QualifiedName() inline.
Comment 1 Andreas Kling 2014-05-10 22:42:57 PDT
Created attachment 231250 [details]
Patch
Comment 2 WebKit Commit Bot 2014-05-10 22:45:07 PDT
Attachment 231250 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/QualifiedName.h:74:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/QualifiedName.h:81:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Darin Adler 2014-05-11 10:51:09 PDT
Comment on attachment 231250 [details]
Patch

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.
Comment 4 Andreas Kling 2014-07-04 18:31:20 PDT
Committed r170816: <http://trac.webkit.org/changeset/170816>