Add documentation for platform/bindings

Add documentation for platform/bindings

This CL adds a README.md explaining the contents of platform/bindings, as well as adds some class level comments to files that were missing them.

BUG=682322
Review-Url: https://codereview.chromium.org/2851563004
Cr-Commit-Position: refs/heads/master@{#468648}
Committed: https://chromium.googlesource.com/chromium/src/+/d996c52de4c95c56df3fef9ec32d48ca6b4e004d

[adithyas, 2017-04-28 18:51:00]

adithyas@chromium.org changed reviewers:
+ jbroman@chromium.org

[jbroman, 2017-04-28 19:07:03]

\o/ hurray documentation

lgtm with nits

https://codereview.chromium.org/2851563004/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/TraceWrapperReference.md (left):

https://codereview.chromium.org/2851563004/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperReference.md:1: # Wrapper
Tracing Reference
Fix the link in BlinkGCAPIReference.md?

https://codereview.chromium.org/2851563004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/bindings/DOMWrapperMap.h (right):

https://codereview.chromium.org/2851563004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/bindings/DOMWrapperMap.h:46: // wrappers
created for those objects.
nit: "between" isn't clear about the direction (the other direction is of course
the wrapped value field). Maybe something along the lines of "Maps from C++
objects to their corresponding JavaScript wrappers. See also DOMDataStore."

https://codereview.chromium.org/2851563004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/bindings/ScopedPersistent.h (right):

https://codereview.chromium.org/2851563004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/bindings/ScopedPersistent.h:43: // A wrapper
class around v8::Persistent, use this class to hold a persistent
super-nit: comma splice. Maybe just "Holds a persistent handle to a V8 object."

https://codereview.chromium.org/2851563004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/bindings/V8ValueCache.h (right):

https://codereview.chromium.org/2851563004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/bindings/V8ValueCache.h:77: // This class
creates and caches v8::String objects. StringCache is held and can
nit: Maybe expand slightly to clarify that it caches v8::String objects
corresponding to WTF::StringImpls (i.e. String/AtomicString)?

[adithyas, 2017-04-28 20:19:43]

https://codereview.chromium.org/2851563004/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/TraceWrapperReference.md (left):

https://codereview.chromium.org/2851563004/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/TraceWrapperReference.md:1: # Wrapper
Tracing Reference
On 2017/04/28 at 19:07:03, jbroman wrote:
> Fix the link in BlinkGCAPIReference.md?

Nice catch

https://codereview.chromium.org/2851563004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/bindings/DOMWrapperMap.h (right):

https://codereview.chromium.org/2851563004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/bindings/DOMWrapperMap.h:46: // wrappers
created for those objects.
On 2017/04/28 at 19:07:03, jbroman wrote:
> nit: "between" isn't clear about the direction (the other direction is of
course the wrapped value field). Maybe something along the lines of "Maps from
C++ objects to their corresponding JavaScript wrappers. See also DOMDataStore."

Oh right! This made sense in my head .. fixed.

https://codereview.chromium.org/2851563004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/bindings/ScopedPersistent.h (right):

https://codereview.chromium.org/2851563004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/bindings/ScopedPersistent.h:43: // A wrapper
class around v8::Persistent, use this class to hold a persistent
On 2017/04/28 at 19:07:03, jbroman wrote:
> super-nit: comma splice. Maybe just "Holds a persistent handle to a V8
object."

Much better :)

https://codereview.chromium.org/2851563004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/bindings/V8ValueCache.h (right):

https://codereview.chromium.org/2851563004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/bindings/V8ValueCache.h:77: // This class
creates and caches v8::String objects. StringCache is held and can
On 2017/04/28 at 19:07:03, jbroman wrote:
> nit: Maybe expand slightly to clarify that it caches v8::String objects
corresponding to WTF::StringImpls (i.e. String/AtomicString)?

Ok updated, WDYT?

[haraken, 2017-04-29 02:39:10]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2017-04-29 02:39:11]

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/bindings/DOMDataStore.h (right):

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/bindings/DOMDataStore.h:51: // and manage
wrappers in a single world.

Clarify that DOMDataStore is a per-world thing and wrappers from different
worlds should not mix.

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/bindings/README.md (right):

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/bindings/README.md:3: This directory contains
classes and functionality used to implement the V8 bindings layer in Blink. Some
of the things you can find here are:

Clarify what should be in platform/bindings/ and what should be in
bindings/core/.

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/bindings/ScopedPersistent.h (right):

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/bindings/ScopedPersistent.h:43: // Holds a
persistent handle to a V8 object.

Add that:

- developers should use ScopedPersistent instead of using v8::Persistent
directly
- developers must be super careful when introducing a (non-weak)
ScopedPersistent since it has a high risk of producing memory leaks. Ask
blink-reviews-bindings@ for review.

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/bindings/SharedPersistent.h (right):

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/bindings/SharedPersistent.h:41: // A ref
counted version of ScopedPersistent.

This class is intended to be used for ScriptValue and not for normal use.
Consider using ScopedPersistent directly.

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/bindings/V8ValueCache.h (right):

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/bindings/V8ValueCache.h:79: // for future
use. It is held by and can be retrieved from V8PerIsolateData.

Add a comment about how the cache is maintained (when the entry is evicted, when
the cache is cleared).

[jbroman, 2017-04-29 04:11:30]

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/bindings/ScopedPersistent.h (right):

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/bindings/ScopedPersistent.h:43: // Holds a
persistent handle to a V8 object.
On 2017/04/29 at 02:39:11, haraken wrote:
> Add that:
> 
> - developers should use ScopedPersistent instead of using v8::Persistent
directly
> - developers must be super careful when introducing a (non-weak)
ScopedPersistent since it has a high risk of producing memory leaks. Ask
blink-reviews-bindings@ for review.

Aside: What does ScopedPersistent do that v8::Global doesn't? Should we
deprecate ScopedPersistent at some point?

[haraken, 2017-05-01 00:30:43]

On 2017/04/29 04:11:30, jbroman wrote:
>
https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/bindings/ScopedPersistent.h (right):
> 
>
https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/bindings/ScopedPersistent.h:43: // Holds a
> persistent handle to a V8 object.
> On 2017/04/29 at 02:39:11, haraken wrote:
> > Add that:
> > 
> > - developers should use ScopedPersistent instead of using v8::Persistent
> directly
> > - developers must be super careful when introducing a (non-weak)
> ScopedPersistent since it has a high risk of producing memory leaks. Ask
> blink-reviews-bindings@ for review.
> 
> Aside: What does ScopedPersistent do that v8::Global doesn't? Should we
> deprecate ScopedPersistent at some point?

Good point. I agree that we should deprecate ScopedPersistent in favor of
v8::Global.

[adithyas, 2017-05-01 17:41:16]

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/bindings/DOMDataStore.h (right):

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/bindings/DOMDataStore.h:51: // and manage
wrappers in a single world.
On 2017/04/29 at 02:39:11, haraken wrote:
> Clarify that DOMDataStore is a per-world thing and wrappers from different
worlds should not mix.

Done.

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/bindings/README.md (right):

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/bindings/README.md:3: This directory contains
classes and functionality used to implement the V8 bindings layer in Blink. Some
of the things you can find here are:
On 2017/04/29 at 02:39:11, haraken wrote:
> Clarify what should be in platform/bindings/ and what should be in
bindings/core/.

Done.

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/bindings/ScopedPersistent.h (right):

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/bindings/ScopedPersistent.h:43: // Holds a
persistent handle to a V8 object.
On 2017/04/29 at 02:39:11, haraken wrote:
> Add that:
> 
> - developers should use ScopedPersistent instead of using v8::Persistent
directly
> - developers must be super careful when introducing a (non-weak)
ScopedPersistent since it has a high risk of producing memory leaks. Ask
blink-reviews-bindings@ for review.

Done.

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/bindings/SharedPersistent.h (right):

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/bindings/SharedPersistent.h:41: // A ref
counted version of ScopedPersistent.
On 2017/04/29 at 02:39:11, haraken wrote:
> This class is intended to be used for ScriptValue and not for normal use.
Consider using ScopedPersistent directly.

Added.

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/bindings/V8ValueCache.h (right):

https://codereview.chromium.org/2851563004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/bindings/V8ValueCache.h:79: // for future
use. It is held by and can be retrieved from V8PerIsolateData.
On 2017/04/29 at 02:39:11, haraken wrote:
> Add a comment about how the cache is maintained (when the entry is evicted,
when the cache is cleared).

Done.

[haraken, 2017-05-02 04:01:43]

LGTM

[adithyas, 2017-05-02 13:58:18]

The CQ bit was checked by adithyas@chromium.org

[adithyas, 2017-05-02 13:58:19]

The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2851563004/#ps40001
(title: "Address code review")

[commit-bot: I haz the power, 2017-05-02 13:58:29]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 15:46:41]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493733498624980,
"parent_rev": "67a0d617995679c71b8b97e8666752931e3f557c", "commit_rev":
"d996c52de4c95c56df3fef9ec32d48ca6b4e004d"}

[commit-bot: I haz the power, 2017-05-02 15:46:49]

Description was changed from

==========
Add documentation for platform/bindings

This CL adds a README.md explaining the contents of platform/bindings, as well
as adds some class level comments to files that were missing them.

BUG=682322
==========

to

==========
Add documentation for platform/bindings

This CL adds a README.md explaining the contents of platform/bindings, as well
as adds some class level comments to files that were missing them.

BUG=682322

Review-Url: https://codereview.chromium.org/2851563004
Cr-Commit-Position: refs/heads/master@{#468648}
Committed:
https://chromium.googlesource.com/chromium/src/+/d996c52de4c95c56df3fef9ec32d...
==========

[commit-bot: I haz the power, 2017-05-02 15:46:53]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d996c52de4c95c56df3fef9ec32d...