Summary: | jsDocumentPrototypeFunctionGetElementById should not create an AtomicString for the function argument | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||
Component: | New Bugs | Assignee: | 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
Benjamin Poulain
2014-02-17 00:56:39 PST
Created attachment 224340 [details]
Patch
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. (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). My bad, I somehow overlooked that this function is static, and thought that this was about finding a substring! 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 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. Committed r164505: <http://trac.webkit.org/changeset/164505> |