Bug 37745 – Move string uniquing tables to (new) WTFThreadData

• Component: Platform
• Status: RESOLVED FIXED

[Gavin Barraclough]

Remove AtomicString's dependency on ThreadGlobalData so that we can move WebCore's string classes up to WTF.

[Gavin Barraclough]

Created attachment 53590 [details]
The patch

Mainly uploading this patch to get the build bot to take a sniff at it, probably not worth reviewing right now.

[Gavin Barraclough]

Created attachment 53591 [details]
More useful patch, with build file changes – still for the build bot.

[WebKit Review Bot]

Attachment 53590 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1605587

[WebKit Review Bot]

Attachment 53591 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1605589

[WebKit Review Bot]

Attachment 53590 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1676363

[Mark Rowe (bdash)]

The WTF prefix on the file and type name seems redundant, and completely inconsistent with other files in WTF.

[Gavin Barraclough]

Created attachment 53620 [details]
Chromium build fix

[Gavin Barraclough]

(In reply to comment #6)
> The WTF prefix on the file and type name seems redundant, and completely
> inconsistent with other files in WTF.

Most classes in the WTF directories are classes to be used by clients of WTF, not parts of WTF's internal implementation (for example ThreadSpecific, which is used instantiated by clients of WTF), and as such the name ThreadData seemed too generic - it did not seem clear that this was not something to be used by clients of WTF but instead something that WTF would use internally.  I added the prefix to try to make this distinction a little more clear.

What do you suggest for a class name, do you like just "ThreadData"? or any other suggestions?

[Mark Rowe (bdash)]

(In reply to comment #8)
> (In reply to comment #6)
> > The WTF prefix on the file and type name seems redundant, and completely
> > inconsistent with other files in WTF.
> 
> Most classes in the WTF directories are classes to be used by clients of WTF,
> not parts of WTF's internal implementation (for example ThreadSpecific, which
> is used instantiated by clients of WTF), and as such the name ThreadData seemed
> too generic - it did not seem clear that this was not something to be used by
> clients of WTF but instead something that WTF would use internally.  I added
> the prefix to try to make this distinction a little more clear.

A word like “private” or “internal” would bring with it the connotation that this is not supposed to be used outside of WTF.  Using WTF as a prefix just makes me wonder WTF this file happens to start with WTF.

[Gavin Barraclough]

(In reply to comment #9)
> A word like “private” or “internal” would bring with it the connotation that
> this is not supposed to be used outside of WTF.  Using WTF as a prefix just
> makes me wonder WTF this file happens to start with WTF.

While it is a part of WTF, it is also accessed from outside of WTF where certain thread-specific pieces of information need to be accessed (currently just the string uniquing tables), so the word 'private' seems inappropriate.  While it is a part of WTF's implementation rather than a class vended to clients, it is a public piece of implementation.  We also have a ThreadGlobalData object in WebCore, and I wanted to be nice and clear and unambiguous about which thread specific data object was being accessed, for example consider the following line from the patch from within WebCore:

    WTFThreadData& data = wtfThreadData();

It seems nice and clear that this is accessing the thread data from and pertaining to WTF, rather than WebCore's own thread data.  Would you suggest:

    InternalThreadData& data = wtfThreadData(); ?
    InternalThreadData& data = internalThreadData(); ?

I'd suggest that having wtf in the name does make sense for this class and function, since it makes it completely clear whose thread data is being accessed (WTF's as opposed than WebCore's).

[Sam Weinig]

Comment on attachment 53620 [details]
Chromium build fix

> +namespace JSC {
> +class IdentifierTable;
> +}
> +
>  namespace WebCore {
> +class AtomicStringTable;
> +}

How weird. Please put a comment explaining how this is a temporary measure while we finish the great string merger.

Otherwise, r=me.

[Eric Seidel (no email)]

This broke windows.

[Eric Seidel (no email)]

Sadly the windows EWS bot is offline otherwise it would have told us this. :(

[Adam Barth]

I got frustrated with windows case insensitive file system.  I can try bringing it back online.

[Gavin Barraclough]

landed in http://trac.webkit.org/changeset/57829

[WebKit Review Bot]

http://trac.webkit.org/changeset/57829 might have broken Windows Debug (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/57829
http://trac.webkit.org/changeset/57830
http://trac.webkit.org/changeset/57831
http://trac.webkit.org/changeset/57832
http://trac.webkit.org/changeset/57833
http://trac.webkit.org/changeset/57834
http://trac.webkit.org/changeset/57835