[Webkit-unassigned] [Bug 27685] [V8] Split up V8DOMMap.cpp by class

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27685


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #33500|review?                     |review+
               Flag|                            |




--- Comment #3 from David Levin <levin at chromium.org>  2009-07-27 09:16:13 PDT ---
(From update of attachment 33500)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.