[webkit-reviews] review denied: [Bug 116791] Layering violations in ThreadGlobalData : [Attachment 202926] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 116791: Layering violations in ThreadGlobalData
https://bugs.webkit.org/show_bug.cgi?id=116791

Attachment 202926: Patch
https://bugs.webkit.org/attachment.cgi?id=202926&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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?