Bug 37745 - Move string uniquing tables to (new) WTFThreadData
Summary: Move string uniquing tables to (new) WTFThreadData
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-16 20:28 PDT by Gavin Barraclough
Modified: 2010-04-19 16:07 PDT (History)
6 users (show)

See Also:


Attachments
The patch (39.62 KB, patch)
2010-04-16 20:32 PDT, Gavin Barraclough
no flags 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 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Chromium build fix (43.31 KB, patch)
2010-04-17 17:31 PDT, Gavin Barraclough
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2010-04-16 20:28:43 PDT
Remove AtomicString's dependency on ThreadGlobalData so that we can move WebCore's string classes up to WTF.
Comment 1 Gavin Barraclough 2010-04-16 20:32:56 PDT
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.
Comment 2 Gavin Barraclough 2010-04-16 21:19:07 PDT
Created attachment 53591 [details]
More useful patch, with build file changes – still for the build bot.
Comment 3 WebKit Review Bot 2010-04-16 21:59:32 PDT
Attachment 53590 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1605587
Comment 4 WebKit Review Bot 2010-04-16 22:33:12 PDT
Attachment 53591 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1605589
Comment 5 WebKit Review Bot 2010-04-16 23:11:50 PDT
Attachment 53590 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1676363
Comment 6 Mark Rowe (bdash) 2010-04-17 16:25:25 PDT
The WTF prefix on the file and type name seems redundant, and completely inconsistent with other files in WTF.
Comment 7 Gavin Barraclough 2010-04-17 17:31:55 PDT
Created attachment 53620 [details]
Chromium build fix
Comment 8 Gavin Barraclough 2010-04-17 17:36:47 PDT
(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 Mark Rowe (bdash) 2010-04-17 17:55:54 PDT
(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 Gavin Barraclough 2010-04-17 18:31:42 PDT
(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 Sam Weinig 2010-04-19 12:36:49 PDT
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.
Comment 12 Eric Seidel (no email) 2010-04-19 13:21:11 PDT
This broke windows.
Comment 13 Eric Seidel (no email) 2010-04-19 13:21:40 PDT
Sadly the windows EWS bot is offline otherwise it would have told us this. :(
Comment 14 Adam Barth 2010-04-19 13:59:24 PDT
I got frustrated with windows case insensitive file system.  I can try bringing it back online.
Comment 15 Gavin Barraclough 2010-04-19 15:44:43 PDT
landed in http://trac.webkit.org/changeset/57829