Replaced HTMLIdentifier with an atomized string factory function

Replaced HTMLIdentifier usage with a factory function which will try to match the string with a map of all known static strings.

As the HTMLNames strings are now static and hence threadsafe, the HTMLIdentifier wrapper used for thread safe string usage by the parser thread is no longer needed.

Sourced from discussions in https://codereview.chromium.org/74513003/

R=abarth@chromium.org,eseidel@chromium.org
BUG=277886
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163806

[oystein (OOO til 10th of July), 2013-12-10 00:56:56]

Something like this?

[abarth-chromium, 2013-12-10 01:22:51]

LGTM.  This CL is a step in the right direction.  I've noted how we can go a bit
further, which we can do either in this CL or in the next CL.  Thanks!!

https://codereview.chromium.org/110843004/diff/1/Source/core/html/parser/CSSP...
File Source/core/html/parser/CSSPreloadScanner.cpp (right):

https://codereview.chromium.org/110843004/diff/1/Source/core/html/parser/CSSP...
Source/core/html/parser/CSSPreloadScanner.cpp:68: void
CSSPreloadScanner::scan(const String& identifier,  const SegmentedString&
source, PreloadRequestStream& requests)
identifier -> tagName?

https://codereview.chromium.org/110843004/diff/1/Source/core/html/parser/HTML...
File Source/core/html/parser/HTMLIdentifier.cpp (right):

https://codereview.chromium.org/110843004/diff/1/Source/core/html/parser/HTML...
Source/core/html/parser/HTMLIdentifier.cpp:100: addNames(getHTMLAttrs(),
HTMLAttrsCount);
This CL is fine as a stepping stone, but we don't actually need to do this work
now.  In StringImpl.cpp, we have StringImpl::allStaticStrings(), which is a
vector of all the static strings.  Rather than storing them in a vector, we can
store them in a HashSet, which makes it faster to query.

https://codereview.chromium.org/110843004/diff/1/Source/core/html/parser/HTML...
File Source/core/html/parser/HTMLIdentifier.h (right):

https://codereview.chromium.org/110843004/diff/1/Source/core/html/parser/HTML...
Source/core/html/parser/HTMLIdentifier.h:58: }
It seems like we don't need this class anymore.  We can just make this a free
function in HTMLParserIdioms.

https://codereview.chromium.org/110843004/diff/1/Source/core/html/parser/HTML...
Source/core/html/parser/HTMLIdentifier.h:68: String m_string;
Is this member used anywhere?

[oystein (OOO til 10th of July), 2013-12-10 02:28:48]

On 2013/12/10 01:22:51, abarth wrote:
> LGTM.  This CL is a step in the right direction.  I've noted how we can go a
bit
> further, which we can do either in this CL or in the next CL.  Thanks!!

Awesome :). Made a stab at the suggestions; still OK?

> 
>
https://codereview.chromium.org/110843004/diff/1/Source/core/html/parser/CSSP...
> File Source/core/html/parser/CSSPreloadScanner.cpp (right):
> 
>
https://codereview.chromium.org/110843004/diff/1/Source/core/html/parser/CSSP...
> Source/core/html/parser/CSSPreloadScanner.cpp:68: void
> CSSPreloadScanner::scan(const String& identifier,  const SegmentedString&
> source, PreloadRequestStream& requests)
> identifier -> tagName?

Done.

>
https://codereview.chromium.org/110843004/diff/1/Source/core/html/parser/HTML...
> File Source/core/html/parser/HTMLIdentifier.cpp (right):
> 
>
https://codereview.chromium.org/110843004/diff/1/Source/core/html/parser/HTML...
> Source/core/html/parser/HTMLIdentifier.cpp:100: addNames(getHTMLAttrs(),
> HTMLAttrsCount);
> This CL is fine as a stepping stone, but we don't actually need to do this
work
> now.  In StringImpl.cpp, we have StringImpl::allStaticStrings(), which is a
> vector of all the static strings.  Rather than storing them in a vector, we
can
> store them in a HashSet, which makes it faster to query.

Done.

>
https://codereview.chromium.org/110843004/diff/1/Source/core/html/parser/HTML...
> File Source/core/html/parser/HTMLIdentifier.h (right):
> 
>
https://codereview.chromium.org/110843004/diff/1/Source/core/html/parser/HTML...
> Source/core/html/parser/HTMLIdentifier.h:58: }
> It seems like we don't need this class anymore.  We can just make this a free
> function in HTMLParserIdioms.

Done.

[abarth-chromium, 2013-12-10 04:48:23]

https://codereview.chromium.org/110843004/diff/40001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserIdioms.cpp (right):

https://codereview.chromium.org/110843004/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserIdioms.cpp:302: // However ASSERTs in
addNames() guard against there ever being collisions
addNames?  Does that function exist anymore?

https://codereview.chromium.org/110843004/diff/40001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserIdioms.h (right):

https://codereview.chromium.org/110843004/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserIdioms.h:102: StringImpl*
findAtomizedStringIfKnown(const UChar* characters, unsigned length);
findAtomizedStringIfKnown -> findStringIfStatic

https://codereview.chromium.org/110843004/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserIdioms.h:111: static String
createAtomizedString(const Vector<UChar, inlineCapacity>& vector, CharacterWidth
width)
createAtomizedString isn't a great name because it doesn't always atomize the
string.  Maybe createPotentiallyStaticString?  That's not a great name either...

https://codereview.chromium.org/110843004/diff/40001/Source/wtf/text/StringIm...
File Source/wtf/text/StringImpl.cpp (right):

https://codereview.chromium.org/110843004/diff/40001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:410: staticStrings().add(hash, impl);
Can you ASSERT that this hash isn't already in the table?  We used to have that
assert and it's important for correctness.

[oystein (OOO til 10th of July), 2013-12-10 18:42:26]

https://codereview.chromium.org/110843004/diff/40001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserIdioms.cpp (right):

https://codereview.chromium.org/110843004/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserIdioms.cpp:302: // However ASSERTs in
addNames() guard against there ever being collisions
On 2013/12/10 04:48:23, abarth wrote:
> addNames?  Does that function exist anymore?

Updated comment.

https://codereview.chromium.org/110843004/diff/40001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserIdioms.h (right):

https://codereview.chromium.org/110843004/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserIdioms.h:102: StringImpl*
findAtomizedStringIfKnown(const UChar* characters, unsigned length);
On 2013/12/10 04:48:23, abarth wrote:
> findAtomizedStringIfKnown -> findStringIfStatic

Done.

https://codereview.chromium.org/110843004/diff/40001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserIdioms.h:111: static String
createAtomizedString(const Vector<UChar, inlineCapacity>& vector, CharacterWidth
width)
On 2013/12/10 04:48:23, abarth wrote:
> createAtomizedString isn't a great name because it doesn't always atomize the
> string.  Maybe createPotentiallyStaticString?  That's not a great name
either...

attemptStaticStringCreation?

https://codereview.chromium.org/110843004/diff/40001/Source/wtf/text/StringIm...
File Source/wtf/text/StringImpl.cpp (right):

https://codereview.chromium.org/110843004/diff/40001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.cpp:410: staticStrings().add(hash, impl);
On 2013/12/10 04:48:23, abarth wrote:
> Can you ASSERT that this hash isn't already in the table?  We used to have
that
> assert and it's important for correctness.

There were actually some duplicates here (SVGNames added "a", I'm guessing
HTMLNames did the same). I added a lookup so they'll be always unique (and an
assert to ensure the actual string also matches).

[oystein (OOO til 10th of July), 2013-12-12 18:29:11]

ping :)

[abarth-chromium, 2013-12-12 18:36:09]

lgtm

Sorry for the delay.

[commit-bot: I haz the power, 2013-12-12 18:36:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oysteine@chromium.org/110843004/60001

[commit-bot: I haz the power, 2013-12-12 19:43:44]

Change committed as 163806