https://codereview.chromium.org/189463019/diff/20001/include/v8.h
File include/v8.h (right):
https://codereview.chromium.org/189463019/diff/20001/include/v8.h#newcode853
include/v8.h:853: UniquePersistent<V> Set(const K& key,
UniquePersistent<V>& value) {
On 2014/03/11 07:44:46, dcarney wrote:
UniquePersistent should always be passed by value. not ref
Done.
https://codereview.chromium.org/189463019/diff/20001/include/v8.h#newcode853
include/v8.h:853: UniquePersistent<V> Set(const K& key,
UniquePersistent<V>& value) {
On 2014/03/11 07:44:46, dcarney wrote:
On 2014/03/10 19:00:46, vogelheim wrote:
> I'd like to ASSERT(!value.IsWeak()), since the code makes the
assumption it
can
> SetWeak(..) on this value. How?
>
> (Most parts of v8 seem to use ASSERT from src/checks.h, but I assume
one
> shouldn't include stuff from src/... here and thus export that
header to all
v8
> clients?)
it's a bit annoying, but we are setting weak immediately, which means
we can do
the assert internally. it may already be there
What do you mean with "we can do the assert internally"?
https://codereview.chromium.org/189463019/diff/20001/include/v8.h#newcode5872
include/v8.h:5872: if (hasValue) {
On 2014/03/11 07:44:46, dcarney wrote:
setting an empty returnvalue means settings the returnvalue's default
value
This emulates the current behaviour of
DOMWrapperMap::SetReturnValueFrom, which will leave the returnValue
unmodified whenever it returns false. I checked the callers of that code
and couldn't tell whether they (deliberately) rely on this property or
not. Hence I opted for the safe choice and preserved the current
behaviour.
The code path is certainly in use, but I suspect in most cases they
receive a 'fresh' v8::ReturnValue that is set to the default coming in,
so that it wouldn't make a difference.
How can I determine whether always setting the returnValue modifies the
behaviour or not?
Reference:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/bindings/v8/DOMWrapperMap.h&q=DOMWrapperMap&sq=package:chromium&type=cs&l=58-62
https://codereview.chromium.org/189463019/diff/20001/include/v8.h#newcode5890
include/v8.h:5890: }
On 2014/03/11 07:57:18, dcarney wrote:
we need to dispose the callback data for weak maps
This calls PersistentValueMap::Release(), which disposes of the callback
data if Traits::kIsWeak.
https://codereview.chromium.org/189463019/diff/20001/include/v8.h#newcode5901
include/v8.h:5901: if (!data.GetValue().IsEmpty()) {
On 2014/03/11 07:57:18, dcarney wrote:
this cannot be empty
Removed.
https://codereview.chromium.org/189463019/diff/20001/include/v8.h#newcode5902
include/v8.h:5902: Traits::DisposeByKey(impl, key);
On 2014/03/11 07:57:18, dcarney wrote:
we should be able to do all this with one disposeCallback, not 3
DisposeCallbackData is called in different contexts from DisposeBy*,
e.g. in Release, when a map value gets deleted or overwritten before it
gets garbage collected, so I guess we need two?
I originally separated out the other two, since seem to be doing two
different things: One does additional cleanup on the value, while the
other does additional cleanup on the map implementation. Those are
easily combinable, so I did that.
If you want to, I can combine all three into one, where I'd pass in all
args in the general case and null-args in the Release-case and then have
the traits classes do an if(..). Not sure that makes the code easier to
read; but I'll be happy to follow your opinion.
https://codereview.chromium.org/189463019/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
