Bug 128893 – jsDocumentPrototypeFunctionGetElementById should not create an AtomicString for the function argument

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

jsDocumentPrototypeFunctionGetElementById should not create an AtomicString for the function argument

[Benjamin Poulain]

Created attachment 224340 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 224340 [details]
Patch

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

> Source/WTF/wtf/text/AtomicString.h:92
> +    static AtomicStringImpl* find(StringImpl* string)
> +    {
> +        if (!string || string->isAtomic())
> +            return static_cast<AtomicStringImpl*>(string);

Is this correct? We don't perform any searching whenever the argument is an atomic string.

[Benjamin Poulain]

(In reply to comment #2)
> (From update of attachment 224340 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224340&action=review
> 
> > Source/WTF/wtf/text/AtomicString.h:92
> > +    static AtomicStringImpl* find(StringImpl* string)
> > +    {
> > +        if (!string || string->isAtomic())
> > +            return static_cast<AtomicStringImpl*>(string);
> 
> Is this correct? We don't perform any searching whenever the argument is an atomic string.

The flag "isAtomic" marks strings that are in the atomic string table. If the flag is set on the current StringImpl, that string is what would be returned by find().

You remind me of something. I am supposed to assert that the string is in the right string table (in case of multithread).

[Alexey Proskuryakov]

My bad, I somehow overlooked that this function is static, and thought that this was about finding a substring!

[Darin Adler]

Comment on attachment 224340 [details]
Patch

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

Seems to me that in the future we might want to have functions like this take a StringView rather than a const String&.

> Source/WTF/wtf/text/AtomicString.cpp:407
> +AtomicStringImpl* AtomicString::findStringWithHash(const StringImpl* stringImpl)

Since you’re renaming this, I suggest also changing it to take a reference instead of a pointer.

> Source/WTF/wtf/text/AtomicString.cpp:452
> +AtomicStringImpl* AtomicString::findSlowCase(StringImpl* string)

Seems like this should take a reference rather than a pointer.

> Source/WTF/wtf/text/AtomicString.cpp:454
> +    ASSERT_WITH_MESSAGE(!string->isAtomic(), "We should not hit the slow case if the string is already atomic.");

Message seems to just repeat what the assertion says, not sure it’s helpful.

> Source/WTF/wtf/text/AtomicString.cpp:461
> +    HashSet<StringImpl*>& atomicStringTable = stringTable();
> +    auto iterator = atomicStringTable.find(string);
> +    if (iterator != atomicStringTable.end())
> +        return static_cast<AtomicStringImpl*>(*iterator);
> +    return nullptr;

We really should add HashSet::get for cases like this one. If that existed, then this would be:

    return static_cast<AtomicStringImpl*>(stringTable().get(string));

I think that reads much better than the 5 lines of code above.

> Source/WebCore/ChangeLog:31
> +        When getting and AtomicString, the case of a empty string is not important, use isNull() instead.

typo: "and" instead of "an"

[Darin Adler]

Comment on attachment 224340 [details]
Patch

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

> Source/WebCore/ChangeLog:26
> +        Now that there are two overloads for TreeScope::getElementById(), the conversion from NSString*
> +        is ambiguous. I add the keyword ObjCExplicitAtomicString to force an explicit conversion to AtomicString.

I think this is really awkward. Lets try to find a better solution in the future.

[Benjamin Poulain]

Committed r164505: <http://trac.webkit.org/changeset/164505>