[webkit-reviews] review granted: [Bug 24951] Upstream V8DOMMap for v8 bindings. : [Attachment 29096] Proposed Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has granted Jian Li
<jianli at chromium.org>'s request for review:
Bug 24951: Upstream V8DOMMap for v8 bindings.
https://bugs.webkit.org/show_bug.cgi?id=24951

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> +++ b/WebCore/ChangeLog
...
> +	   * bindings/v8/V8DOMMap.cpp: Added.
> +	   (WebCore::domThreadMap):
> +	   (WebCore::domThreadMapMutex):
> +	   (WebCore::ThreadSpecificDOMData::):
> +	  
(WebCore::ThreadSpecificDOMData::InternalDOMWrapperMap::InternalDOMWrapperMap):


nit: when adding a new file to the repository, please delete these lines
that prepare-ChangeLog adds for each added method.


> +++ b/WebCore/bindings/v8/V8DOMMap.cpp

> +// DOM binding algorithm:
> +//
> +// There are two kinds of DOM objects:
> +// 1. DOM tree nodes, such as Document, HTMLElement, ...
> +//	 there classes implements TreeShared<T> interface;

nit: perhaps this would be better written as: "these classes implement the
TreeShared<T> interface" ?


> +//	Handles from DOM objects to JS wrappers are always weak,
> +// so JS wrappers of non-node object cannot create a cycle.

nit: I think this comment is referring to Peerable, which no longer exists.  It
might
be good to check with the V8 guys.


> +// This encapsulates thread-specific DOM data for the main thread. All the
maps in it are static.
> +// This is because we are unable to rely on WTF::ThreadSpecificThreadExit to
do the cleanup since the place that tears down the main thread can not call any
WTF functions.

nit: it might be nice to add a line break to the second line of the comment so
that each line of
the comment block is approx the same width.


otherwise LGTM