[webkit-reviews] review granted: [Bug 27685] [V8] Split up V8DOMMap.cpp by class : [Attachment 33500] Patch (just copy/paste)

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 27685: [V8] Split up V8DOMMap.cpp by class
https://bugs.webkit.org/show_bug.cgi?id=27685

Attachment 33500: Patch (just copy/paste)
https://bugs.webkit.org/attachment.cgi?id=33500&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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.