Bug 116791 – Layering violations in ThreadGlobalData

• Status: NEW
• Component: New Bugs

[Zan Dobersek]

Layering violations in ThreadGlobalData

[Zan Dobersek]

Created attachment 202926 [details]
Patch

[WebKit Commit Bot]

Attachment 202926 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/DOMImplementation.cpp', u'Source/WebCore/dom/DOMImplementation.h', u'Source/WebCore/dom/EventNames.h', u'Source/WebCore/inspector/InspectorCounters.cpp', u'Source/WebCore/inspector/InspectorCounters.h', u'Source/WebCore/loader/cache/CachedResourceRequestInitiators.h', u'Source/WebCore/platform/ThreadGlobalData.cpp', u'Source/WebCore/platform/ThreadGlobalData.h', u'Source/WebCore/platform/ThreadTimers.cpp', u'Source/WebCore/platform/ThreadTimers.h', u'Source/WebCore/platform/Timer.cpp', u'Source/WebCore/platform/text/TextCodecICU.cpp', u'Source/WebCore/platform/text/TextCodecICU.h', u'Source/WebCore/platform/text/mac/TextCodecMac.cpp', u'Source/WebCore/platform/text/mac/TextCodecMac.h', u'Source/WebCore/workers/WorkerRunLoop.cpp']" exit_code: 1
Source/WebCore/platform/ThreadGlobalData.h:45:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 202926 [details]
Patch

Attachment 202926 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/739354

[EFL EWS Bot]

Comment on attachment 202926 [details]
Patch

Attachment 202926 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/739356

[Early Warning System Bot]

Comment on attachment 202926 [details]
Patch

Attachment 202926 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/686529

[EFL EWS Bot]

Comment on attachment 202926 [details]
Patch

Attachment 202926 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/718216

[Build Bot]

Comment on attachment 202926 [details]
Patch

Attachment 202926 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/686532

[Build Bot]

Comment on attachment 202926 [details]
Patch

Attachment 202926 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/665221

[Alexey Proskuryakov]

Comment on attachment 202926 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=202926&action=review

We need to find a way to solve this without introducing a new hash map lookup for every eventNames() access.

> Source/WebCore/platform/ThreadGlobalData.h:-60
> -        const CachedResourceRequestInitiators& cachedResourceRequestInitiators() { return *m_cachedResourceRequestInitiators; }

Why was this ever needed? This looks like nearly dead code (only used by RESOURCE_TIMING), and never needed to use per-thread AtomicStrings anyway - it could use a simple plain enum.

> Source/WebCore/platform/ThreadGlobalData.h:-61
> -        EventNames& eventNames() { return *m_eventNames; }

This one we probably need, to support event dispatch in workers. But if we can make AtomicStrings thread safe, then there will be no need to have them in thread local storage.

> Source/WebCore/platform/ThreadGlobalData.h:-62
> -        ThreadTimers& threadTimers() { return *m_threadTimers; }

Timers are a platforms concept, and should be fine to keep.

> Source/WebCore/platform/ThreadGlobalData.h:-63
> -        XMLMIMETypeRegExp& xmlTypeRegExp() { return *m_xmlTypeRegExp; }

This just seems completely useless. We shouldn't use regular expressions for parsing, and it's certainly not some kind of super hot code AFAICT.

> Source/WebCore/platform/ThreadGlobalData.h:-66
> -        ICUConverterWrapper& cachedConverterICU() { return *m_cachedConverterICU; }

This is also part of platform.

> Source/WebCore/platform/ThreadGlobalData.h:-70
> -        TECConverterWrapper& cachedConverterTEC() { return *m_cachedConverterTEC; }

Ditto.

> Source/WebCore/platform/ThreadGlobalData.h:-74
> -        ThreadLocalInspectorCounters& inspectorCounters() { return *m_inspectorCounters; }

Unsure what this is. Does Inspector actually hook up to multiple threads?

[Zan Dobersek]

Removing XMLMIMETypeRegExp in bug #116861.