Summary: | [V8] Split up V8DOMMap.cpp by class | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||||
Component: | WebCore JavaScript | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | dglazkov, eric, fishd | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Adam Barth
2009-07-25 16:56:17 PDT
Created attachment 33499 [details]
Patch (just copy/paste)
Created attachment 33500 [details]
Patch (just copy/paste)
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. Created attachment 33558 [details]
Patch with style feedback addressed
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 |