Convert DOMWrapperMap to use PersistentValueMap

Convert DOMWrapperMap to use PersistentValueMap

BUG=276323
R=dcarney@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170974

[dcarney, 2014-03-07 09:38:23]

Hey Daniel,

I had a few minutes to look over where you're doing.   I left lots of comments,
but I don't what state your patch is currently in, so ignore them if they are
out of date.  In general, we need everywhere in this class to avoid the creation
of a new Local.  The only function that should create them is the one called
newLocal, which is so called so that we know the cost upfront.

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
File Source/bindings/v8/DOMWrapperMap.h (right):

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:54: return
v8::Local<v8::Object>::New(isolate, m_map.Get(key));
Get already returns a local, no need to double wrap

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:59: v8::Local<v8::Object> value =
m_map.Get(key);
we don't want to create a local for setting a return value - too expensive. 
you'll need to push setReturnValue down to PersistentValueMap

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:69: v8::Persistent<v8::Object> ref(isolate,
m_map.Get(key));
push to PersistentValueMap

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:75: return !m_map.Get(key).IsEmpty();
too expensive - push to PersistentValueMap

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:80: v8::Local<v8::Object> mapValue =
m_map.Get(key);
this function is stupid and only used in an assert in npobject - move it there,
maybe in another quick cl first

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:87: ASSERT(!wrapper.IsEmpty());
this is asserted in the line above, drop

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:88: m_map.SetWithConfig(key,
v8::Local<v8::Object>::New(m_isolate, wrapper),
no need for local::new

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:108: class WeakCallbackDataType {
WeakCallbackDataType should just be a point to the DOMWrapperMap.  everything
else is available in the callback

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:138: v8::PersistentContainerValue oldValue =
Get(impl, key);
this function seems inefficient - is there a way to do it more direrctly?

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:161: return new WeakCallbackDataType(impl,
key, value);
just return 'this'

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:166: delete callbackData;
should be empty

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:197: NPObject* key)
this needs to go to the file where it's currently used

[vogelheim, 2014-03-07 10:03:57]

On Fri, Mar 7, 2014 at 10:38 AM, <dcarney@chromium.org> wrote:

> I had a few minutes to look over where you're doing.   I left lots of
> comments,
> but I don't what state your patch is currently in, so ignore them if they
> are
> out of date.  In general, we need everywhere in this class to avoid the
> creation
> of a new Local.  The only function that should create them is the one
> called
> newLocal, which is so called so that we know the cost upfront.
>

Cool, thanks. The patch seems to work, test are passing (as much as they
did before), and I was going to send it for review.. :)  There's some
changes to PersistentValueMap, and I somehow didn't manage to make a cl for
that. I'll incorporate your changes, probably by today.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[vogelheim, 2014-03-07 18:01:34]

All done; but some more questions.
The v8.h change is coming shortly.

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
File Source/bindings/v8/DOMWrapperMap.h (right):

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:54: return
v8::Local<v8::Object>::New(isolate, m_map.Get(key));
On 2014/03/07 09:38:24, dcarney wrote:
> Get already returns a local, no need to double wrap

Done.

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:59: v8::Local<v8::Object> value =
m_map.Get(key);
On 2014/03/07 09:38:24, dcarney wrote:
> we don't want to create a local for setting a return value - too expensive. 
> you'll need to push setReturnValue down to PersistentValueMap

Done.

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:69: v8::Persistent<v8::Object> ref(isolate,
m_map.Get(key));
On 2014/03/07 09:38:24, dcarney wrote:
> push to PersistentValueMap

Done.

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:75: return !m_map.Get(key).IsEmpty();
On 2014/03/07 09:38:24, dcarney wrote:
> too expensive - push to PersistentValueMap

Done.

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:80: v8::Local<v8::Object> mapValue =
m_map.Get(key);
On 2014/03/07 09:38:24, dcarney wrote:
> this function is stupid and only used in an assert in npobject - move it
there,
> maybe in another quick cl first

Done.

Actually, it drops out with this change, since that code that calls it no longer
gets to see the values its currently asserting on.

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:87: ASSERT(!wrapper.IsEmpty());
On 2014/03/07 09:38:24, dcarney wrote:
> this is asserted in the line above, drop

Done.

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:88: m_map.SetWithConfig(key,
v8::Local<v8::Object>::New(m_isolate, wrapper),
On 2014/03/07 09:38:24, dcarney wrote:
> no need for local::new

Done.

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:108: class WeakCallbackDataType {
On 2014/03/07 09:38:24, dcarney wrote:
> WeakCallbackDataType should just be a point to the DOMWrapperMap.  everything
> else is available in the callback

Done.

I find this a weird design, though. This client - DOMWrapperMap - can derive
keys from values and vice versa. It mostly pretends that keys and values are
separate things; but the assert in set(..) ensures this behaviour.

The way it's implemented now, this decision is imported into PersitentValueMap:
Traits::KeyFromWeakCallbackData - which contains the isolate, the impl, and the
value, but not the key - is only implementable for maps which have the same
property.

Am I overlooking something?

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:138: v8::PersistentContainerValue oldValue =
Get(impl, key);
On 2014/03/07 09:38:24, dcarney wrote:
> this function seems inefficient - is there a way to do it more direrctly?

Done.

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:161: return new WeakCallbackDataType(impl,
key, value);
On 2014/03/07 09:38:24, dcarney wrote:
> just return 'this'

Done.

(Actually impl, but hat's probably what you meant.)

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:166: delete callbackData;
On 2014/03/07 09:38:24, dcarney wrote:
> should be empty

Done.

https://codereview.chromium.org/180363004/diff/40001/Source/bindings/v8/DOMWr...
Source/bindings/v8/DOMWrapperMap.h:197: NPObject* key)
On 2014/03/07 09:38:24, dcarney wrote:
> this needs to go to the file where it's currently used

Done.

For my understanding: Why? I thought it rather surprising to have a partial
implementation for DOMWrapperMap in a different file; hence my change.

[haraken, 2014-03-10 01:11:33]

It's great we can remove UnsafePersistents from the embedder!

[dcarney, 2014-03-11 15:16:27]

lgtm

https://codereview.chromium.org/180363004/diff/100001/Source/bindings/v8/V8NP...
File Source/bindings/v8/V8NPObject.cpp (left):

https://codereview.chromium.org/180363004/diff/100001/Source/bindings/v8/V8NP...
Source/bindings/v8/V8NPObject.cpp:413:
ASSERT(staticNPObjectMap().containsKeyAndValue(npObject, data.GetValue()));
can we keep the assert?

[vogelheim, 2014-03-11 18:35:30]

https://codereview.chromium.org/180363004/diff/100001/Source/bindings/v8/V8NP...
File Source/bindings/v8/V8NPObject.cpp (left):

https://codereview.chromium.org/180363004/diff/100001/Source/bindings/v8/V8NP...
Source/bindings/v8/V8NPObject.cpp:413:
ASSERT(staticNPObjectMap().containsKeyAndValue(npObject, data.GetValue()));
On 2014/03/11 15:16:28, dcarney wrote:
> can we keep the assert?

Done.

[vogelheim, 2014-03-18 14:31:08]

PTAL. Updated to reflect changes to PersistentValueMap.

[dcarney, 2014-03-18 15:20:25]

lgtm

https://codereview.chromium.org/180363004/diff/140001/Source/bindings/v8/V8NP...
File Source/bindings/v8/V8NPObject.cpp (right):

https://codereview.chromium.org/180363004/diff/140001/Source/bindings/v8/V8NP...
Source/bindings/v8/V8NPObject.cpp:421: 
blink doesn't have the 2 space rule

[vogelheim, 2014-03-20 18:19:05]

https://codereview.chromium.org/180363004/diff/100001/Source/bindings/v8/V8NP...
File Source/bindings/v8/V8NPObject.cpp (left):

https://codereview.chromium.org/180363004/diff/100001/Source/bindings/v8/V8NP...
Source/bindings/v8/V8NPObject.cpp:413:
ASSERT(staticNPObjectMap().containsKeyAndValue(npObject, data.GetValue()));
On 2014/03/11 18:35:31, vogelheim wrote:
> On 2014/03/11 15:16:28, dcarney wrote:
> > can we keep the assert?
> 
> Done.

And gone again. :)

This caused test crashes, since PersistentValueMap will remove the values
/before/ calling the callback, and hence these asserts always failed.

I considered whether to change the call order in PersistentValueMap, but I'd
think remove-before-Dispose seems much more logical.

https://codereview.chromium.org/180363004/diff/140001/Source/bindings/v8/V8NP...
File Source/bindings/v8/V8NPObject.cpp (right):

https://codereview.chromium.org/180363004/diff/140001/Source/bindings/v8/V8NP...
Source/bindings/v8/V8NPObject.cpp:421: 
On 2014/03/18 15:20:25, dcarney wrote:
> blink doesn't have the 2 space rule

Done.

[vogelheim, 2014-04-07 14:04:41]

The CQ bit was checked by vogelheim@chromium.org

[commit-bot: I haz the power, 2014-04-07 14:04:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vogelheim@chromium.org/180363004/240001

[commit-bot: I haz the power, 2014-04-07 14:57:08]

Change committed as 170974