Bug 128893

Summary: jsDocumentPrototypeFunctionGetElementById should not create an AtomicString for the function argument
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cdumez, cgarcia, cmarcelo, commit-queue, darin, d-r, esprehn+autocc, fmalita, gyuyoung.kim, kangil.han, kondapallykalyan, pdr, schenney, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch darin: review+

Description Benjamin Poulain 2014-02-17 00:56:39 PST
jsDocumentPrototypeFunctionGetElementById should not create an AtomicString for the function argument
Comment 1 Benjamin Poulain 2014-02-17 01:11:21 PST
Created attachment 224340 [details]
Patch
Comment 2 Alexey Proskuryakov 2014-02-17 10:33:10 PST
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.
Comment 3 Benjamin Poulain 2014-02-17 11:21:50 PST
(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).
Comment 4 Alexey Proskuryakov 2014-02-17 11:34:12 PST
My bad, I somehow overlooked that this function is static, and thought that this was about finding a substring!
Comment 5 Darin Adler 2014-02-19 10:46:40 PST
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"
Comment 6 Darin Adler 2014-02-19 10:47:12 PST
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.
Comment 7 Benjamin Poulain 2014-02-21 14:42:30 PST
Committed r164505: <http://trac.webkit.org/changeset/164505>