Bug 27685 – [V8] Split up V8DOMMap.cpp by class

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Adam Barth]

This file has too many classes.  Let's break them up into separate files.  I don't plan to change the interface at all.  Patch forthcoming.

[Adam Barth]

Created attachment 33499 [details]
Patch (just copy/paste)

[Adam Barth]

Created attachment 33500 [details]
Patch (just copy/paste)

[David Levin]

Comment on attachment 33500 [details]
Patch (just copy/paste)

Just a few things that can be fixed up on landing.  

If the changes are substantial or you'd like me to review the "thread safe" comments or fixes, feel free to put up for review again.


> Index: WebCore/ChangeLog
> +        * bindings/v8/DOMDataStore.cpp: Added.
> +        (WebCore::DOMDataStore::DOMDataStore):
> +        (WebCore::DOMDataStore::~DOMDataStore):
> +        (WebCore::DOMDataStore::allStores):
> +        (WebCore::DOMDataStore::allStoresMutex):
> +        (WebCore::DOMDataStore::getDOMWrapperMap):
> +        (WebCore::::forget):

Would be nice to fix this function definition: "WebCore::::forget"


> Index: WebCore/WebCore.gypi
>              'bindings/js/StringSourceProvider.h',
>              'bindings/js/WorkerScriptController.cpp',
>              'bindings/js/WorkerScriptController.h',
> +            'bindings/v8/ChildThreadDOMData.h',
> +            'bindings/v8/ChildThreadDOMData.cpp',

For all changes in gypi, put foo.h after foo.cpp (sorting).

> Index: WebCore/bindings/v8/DOMData.cpp

> +
> +DOMData* DOMData::getCurrent()
> +{
> +    if (WTF::isMainThread())
> +        return getCurrentMainThread();
> +
> +    DEFINE_STATIC_LOCAL(WTF::ThreadSpecific<ChildThreadDOMData>, childThreadDOMData, ());

This looks like it has race conditions.  Please add a comment about why it isn't or consider switching to using AtomicallyInitializedStatic (from wtf/Threading.h).

>
> +void DOMData::ensureDeref(V8ClassIndex::V8WrapperType type, void* domObject)
> +{
> +    if (m_owningThread == WTF::currentThread()) {
> +        // No need to delay the work.  We can deref right now.

Nice to change to single space after .


> Index: WebCore/bindings/v8/DOMData.h
> ===================================================================
>
> +    class DOMData: public Noncopyable {
Add space before ":"

> +    public:
> +        DOMData();
> +
> +        static DOMData* getCurrent();
> +        static DOMData* getCurrentMainThread(); // Caller must be on the main thread.
> +        virtual DOMDataStore& getStore() = 0;
> +
> +        template<typename T>
> +        static void handleWeakObject(DOMDataStore::DOMWrapperMapType mapType, v8::Handle<v8::Object> v8Object, T* domObject);
Remove redundant parameter names "mapType" and possibly "v8Object".


> +
> +        void forgetDelayedObject(void* object) { m_delayedObjectMap.take(object); }
> +
> +        // This is to ensure that we will deref DOM objects from the owning thread,
> +        // not the GC thread.  The helper function will be scheduled by the GC

Nice to change to single space after .

> Index: WebCore/bindings/v8/DOMDataStore.cpp


> +WTF::Mutex& DOMDataStore::allStoresMutex()
> +{
> +    DEFINE_STATIC_LOCAL(WTF::Mutex, staticDOMDataListMutex, ());

This looks like it has race conditions.  Please add a comment about why it isn't or consider switching to using AtomicallyInitializedStatic (from wtf/Threading.h).



> Index: WebCore/bindings/v8/DOMDataStore.h
> ===================================================================

> +
> +#include <wtf/HashMap.h>
> +#include <wtf/MainThread.h>
> +#include <wtf/Noncopyable.h>
> +#include <wtf/OwnPtr.h>
> +#include <wtf/StdLibExtras.h>
> +#include <wtf/Threading.h>
> +#include <wtf/ThreadSpecific.h>
> +#include <wtf/Vector.h>
> +#include <v8.h>

I would put v8.h before the wtf based on sorting.


> +
> +        template <class KeyType>
> +        class InternalDOMWrapperMap : public DOMWrapperMap<KeyType> {
> +        public:
> +            InternalDOMWrapperMap(DOMData* domData, v8::WeakReferenceCallback callback)
> +                : DOMWrapperMap<KeyType>(callback) , m_domData(domData) { }

Bad spacing before the ,

Actually, I would just format the code like this:
            InternalDOMWrapperMap(DOMData* domData, v8::WeakReferenceCallback callback)
                : DOMWrapperMap<KeyType>(callback)
                , m_domData(domData)
            {
            }


> +        DOMDataStore(DOMData* domData);

Remove parameter name.



> Index: WebCore/bindings/v8/ScopedDOMDataStore.cpp
> +ScopedDOMDataStore::ScopedDOMDataStore(DOMData* domData) : DOMDataStore(domData)

Put ": DOMDataStore(domData)" on the next line.


> Index: WebCore/bindings/v8/ScopedDOMDataStore.h
> +    class ScopedDOMDataStore : public DOMDataStore {
> +    public:
> +        ScopedDOMDataStore(DOMData* domData);

Remove parameter name.

[Adam Barth]

Created attachment 33558 [details]
Patch with style feedback addressed

[Adam Barth]

Sending        WebCore/ChangeLog
Sending        WebCore/WebCore.gypi
Adding         WebCore/bindings/v8/ChildThreadDOMData.cpp
Adding         WebCore/bindings/v8/ChildThreadDOMData.h
Adding         WebCore/bindings/v8/DOMData.cpp
Adding         WebCore/bindings/v8/DOMData.h
Adding         WebCore/bindings/v8/DOMDataStore.cpp
Adding         WebCore/bindings/v8/DOMDataStore.h
Adding         WebCore/bindings/v8/MainThreadDOMData.cpp
Adding         WebCore/bindings/v8/MainThreadDOMData.h
Adding         WebCore/bindings/v8/ScopedDOMDataStore.cpp
Adding         WebCore/bindings/v8/ScopedDOMDataStore.h
Adding         WebCore/bindings/v8/StaticDOMDataStore.cpp
Adding         WebCore/bindings/v8/StaticDOMDataStore.h
Sending        WebCore/bindings/v8/V8DOMMap.cpp
Sending        WebCore/bindings/v8/V8DOMMap.h
Sending        WebCore/bindings/v8/V8GCController.cpp
Transmitting file data .................
Committed revision 46459.
http://trac.webkit.org/changeset/46459