Bug 37745 - Move string uniquing tables to (new) WTFThreadData
: Move string uniquing tables to (new) WTFThreadData
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-04-16 20:28 PST by
Modified: 2010-04-19 16:07 PST (History)


Attachments
The patch (39.62 KB, patch)
2010-04-16 20:32 PST, Gavin Barraclough
no flags Review Patch | Details | Formatted Diff | Diff
More useful patch, with build file changes – still for the build bot. (43.30 KB, patch)
2010-04-16 21:19 PST, Gavin Barraclough
no flags Review Patch | Details | Formatted Diff | Diff
Chromium build fix (43.31 KB, patch)
2010-04-17 17:31 PST, Gavin Barraclough
sam: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-04-16 20:28:43 PST
Remove AtomicString's dependency on ThreadGlobalData so that we can move WebCore's string classes up to WTF.
------- Comment #1 From 2010-04-16 20:32:56 PST -------
Created an attachment (id=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.
------- Comment #2 From 2010-04-16 21:19:07 PST -------
Created an attachment (id=53591) [details]
More useful patch, with build file changes – still for the build bot.
------- Comment #3 From 2010-04-16 21:59:32 PST -------
Attachment 53590 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1605587
------- Comment #4 From 2010-04-16 22:33:12 PST -------
Attachment 53591 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1605589
------- Comment #5 From 2010-04-16 23:11:50 PST -------
Attachment 53590 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1676363
------- Comment #6 From 2010-04-17 16:25:25 PST -------
The WTF prefix on the file and type name seems redundant, and completely inconsistent with other files in WTF.
------- Comment #7 From 2010-04-17 17:31:55 PST -------
Created an attachment (id=53620) [details]
Chromium build fix
------- Comment #8 From 2010-04-17 17:36:47 PST -------
(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?
------- Comment #9 From 2010-04-17 17:55:54 PST -------
(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.
------- Comment #10 From 2010-04-17 18:31:42 PST -------
(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).
------- Comment #11 From 2010-04-19 12:36:49 PST -------
(From update of attachment 53620 [details])
> +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.
------- Comment #12 From 2010-04-19 13:21:11 PST -------
This broke windows.
------- Comment #13 From 2010-04-19 13:21:40 PST -------
Sadly the windows EWS bot is offline otherwise it would have told us this. :(
------- Comment #14 From 2010-04-19 13:59:24 PST -------
I got frustrated with windows case insensitive file system.  I can try bringing it back online.
------- Comment #15 From 2010-04-19 15:44:43 PST -------
landed in http://trac.webkit.org/changeset/57829