Bug 176639 – QualifiedName::init should assume AtomicStrings::init was already called

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joseph Pecoraro]

QualifiedName::init should assume AtomicStrings::init was already called

All clients of QualifiedName::init() already call AtomicString::init(), QualifiedName doesn't need to do it itself.

    $ ack 'QualifiedName::init' -B 2
    WebKit/UIProcess/API/APIContentRuleListStore.cpp
    429-{
    430-    AtomicString::init();
    431:    WebCore::QualifiedName::init();

    WebCore/page/Frame.cpp
    174-    AtomicString::init();
    175-    HTMLNames::init();
    176:    QualifiedName::init();

    WebCore/contentextensions/ContentExtensionParser.cpp
    235-{
    236-    AtomicString::init();
    237:    QualifiedName::init();

[Joseph Pecoraro]

Created attachment 320317 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 320317 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=320317&action=review

> Source/WebCore/ChangeLog:10
> +        All callers of QualifiedName::init preceed it with their own call to

Typo: preceed => precede

[Joseph Pecoraro]

Created attachment 320318 [details]
[PATCH] Proposed Fix

[Sam Weinig]

Comment on attachment 320318 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=320318&action=review

> Source/WebCore/dom/QualifiedName.cpp:-48
> -    AtomicString::init();

Can we assert this instead?

[Joseph Pecoraro]

Comment on attachment 320318 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=320318&action=review

>> Source/WebCore/dom/QualifiedName.cpp:-48
>> -    AtomicString::init();
> 
> Can we assert this instead?

The next line uses nullAtom / starAtom which will assert if AtomicString::init has not happened. I figured that was enough, albeit a little obtuse.

[Sam Weinig]

(In reply to Joseph Pecoraro from comment #5)
> Comment on attachment 320318 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320318&action=review
> 
> >> Source/WebCore/dom/QualifiedName.cpp:-48
> >> -    AtomicString::init();
> > 
> > Can we assert this instead?
> 
> The next line uses nullAtom / starAtom which will assert if
> AtomicString::init has not happened. I figured that was enough, albeit a
> little obtuse.

Seems like an explicit assert would document intent much better.

[Joseph Pecoraro]

Created attachment 320489 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 320489 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=320489&action=review

> Source/WTF/wtf/NeverDestroyed.h:114
> +    bool isConstructed() const { return m_isConstructed; }

Needs !ASSERT_DISABLED like the member.

[Joseph Pecoraro]

Created attachment 320508 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 320508 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 320508

Committed r221951: <http://trac.webkit.org/changeset/221951>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693488>