Bug 24951 - Upstream V8DOMMap for v8 bindings.
Summary: Upstream V8DOMMap for v8 bindings.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dmitry Titov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-30 16:27 PDT by Jian Li
Modified: 2009-03-31 12:59 PDT (History)
1 user (show)

See Also:


Attachments
Proposed Patch (28.45 KB, patch)
2009-03-30 16:31 PDT, Jian Li
fishd: review+
Details | Formatted Diff | Diff
Proposed Patch (26.28 KB, patch)
2009-03-31 12:43 PDT, Jian Li
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jian Li 2009-03-30 16:27:20 PDT
Upstream V8DOMMap for v8 bindings.
Comment 1 Jian Li 2009-03-30 16:31:31 PDT
Created attachment 29096 [details]
Proposed Patch
Comment 2 Darin Fisher (:fishd, Google) 2009-03-31 10:57:49 PDT
Comment on attachment 29096 [details]
Proposed Patch

> +++ 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
Comment 3 Jian Li 2009-03-31 12:43:26 PDT
Created attachment 29130 [details]
Proposed Patch

Here is the new patch that fixed all Darin's comment. Please use this to land.
Comment 4 Dmitry Titov 2009-03-31 12:59:45 PDT
Landed: http://trac.webkit.org/changeset/42135