Unify phantom and internal fields weak handle callbacks

Unify phantom and internal fields weak handle callbacks

R=dcarney@chromium.org
BUG=

Committed: https://crrev.com/7db1759b656ecdc017236165c756ceb4488f5711
Cr-Commit-Position: refs/heads/master@{#26022}

[dcarney, 2015-01-09 12:44:38]

this fixes it - no idea why

https://codereview.chromium.org/842153004/diff/1/src/global-handles.cc
File src/global-handles.cc (right):

https://codereview.chromium.org/842153004/diff/1/src/global-handles.cc#newcod...
src/global-handles.cc:273: void* internal_field0 = 0;
      Object* internal_field0 = nullptr;
      Object* internal_field1 = nullptr;

https://codereview.chromium.org/842153004/diff/1/src/global-handles.cc#newcod...
src/global-handles.cc:299: }
      if (!internal_field0->IsSmi()) {
        internal_field0 = nullptr;
      }
      if (!internal_field1->IsSmi()) {
        internal_field1 = nullptr;
      }

[Erik Corry Chromium.org, 2015-01-09 14:13:41]

PTAL. This passes tests.  A followup change will clean up the way handles are
reset so they can't be reused before we have asserted that they were reset.

[dcarney, 2015-01-09 14:22:53]

lgtm, one comment for this cl and a bunch of followup remarks

https://codereview.chromium.org/842153004/diff/20002/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/842153004/diff/20002/include/v8.h#newcode537
include/v8.h:537: V8_INLINE void SetWeak(
another follow up:

make two new SetWeak(P*, WeakCallbackData<T, P>::Callback, WeakCallbackType)
functions where WeakCallbackType contains the four relevant types and mark these
functions as needing deprecation

https://codereview.chromium.org/842153004/diff/20002/include/v8.h#newcode558
include/v8.h:558: P* parameter, int internal_field_index1,
i'm not sure i see the point in setting these fields, but since the api is in
flux, that can wait for a follow up

https://codereview.chromium.org/842153004/diff/20002/src/global-handles.cc
File src/global-handles.cc (right):

https://codereview.chromium.org/842153004/diff/20002/src/global-handles.cc#ne...
src/global-handles.cc:242: set_weakness_type(PHANTOM_WEAK_0_INTERNAL_FIELDS);
i think verifying here that the jsobject has enough fields is better than below
if, for instance, you want 2 fields but there is only 1, it's probably okay to
just trim the callback type down to 1
the number of fields is immutable and the object pointed to by the cell can't
change

[dcarney, 2015-01-09 14:31:11]

https://codereview.chromium.org/842153004/diff/20002/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/842153004/diff/20002/include/v8.h#newcode141
include/v8.h:141: class CallbackData {
one comment disappeared from my previous mail - there's no reason not to have
one callbackdata - WeakCallbackData - and an enum describing the 4 types of weak
callbacks

[Erik Corry Chromium.org, 2015-01-12 11:37:53]

https://codereview.chromium.org/842153004/diff/20002/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/842153004/diff/20002/include/v8.h#newcode141
include/v8.h:141: class CallbackData {
On 2015/01/09 14:31:10, dcarney wrote:
> one comment disappeared from my previous mail - there's no reason not to have
> one callbackdata - WeakCallbackData - and an enum describing the 4 types of
weak
> callbacks

The reason is static type checking.  If your function takes a structure that has
a handle then that's an indication that it may want to use that handle, but if
you set it up with MakePhantom, there isn't going to be one.

https://codereview.chromium.org/842153004/diff/20002/include/v8.h#newcode537
include/v8.h:537: V8_INLINE void SetWeak(
On 2015/01/09 14:22:52, dcarney wrote:
> another follow up:
> 
> make two new SetWeak(P*, WeakCallbackData<T, P>::Callback, WeakCallbackType)
> functions where WeakCallbackType contains the four relevant types and mark
these
> functions as needing deprecation

Again, I'd rather not move to this more dynamic version where it is harder to
write asserts.

https://codereview.chromium.org/842153004/diff/20002/include/v8.h#newcode558
include/v8.h:558: P* parameter, int internal_field_index1,
On 2015/01/09 14:22:52, dcarney wrote:
> i'm not sure i see the point in setting these fields, but since the api is in
> flux, that can wait for a follow up

The code is not setting any fields from these parameters; the index numbers are
passed so we can assert on them in V8.

https://codereview.chromium.org/842153004/diff/20002/src/global-handles.cc
File src/global-handles.cc (right):

https://codereview.chromium.org/842153004/diff/20002/src/global-handles.cc#ne...
src/global-handles.cc:242: set_weakness_type(PHANTOM_WEAK_0_INTERNAL_FIELDS);
On 2015/01/09 14:22:52, dcarney wrote:
> i think verifying here that the jsobject has enough fields is better than
below
> if, for instance, you want 2 fields but there is only 1, it's probably okay to
> just trim the callback type down to 1
> the number of fields is immutable and the object pointed to by the cell can't
> change

I'm not very happy with this suggestion as it stands. I can't see how to check
it statically, but I at least want an assert to trigger if the API user asks for
internal field numbers that are not 0 or 1, and I want an assert to trigger if
there are fewer fields than the API user has asked for.

[Erik Corry Chromium.org, 2015-01-12 11:44:11]

The CQ bit was checked by erikcorry@chromium.org

[commit-bot: I haz the power, 2015-01-12 11:44:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/842153004/20002

[commit-bot: I haz the power, 2015-01-12 12:11:56]

Committed patchset #3 (id:20002)

[commit-bot: I haz the power, 2015-01-12 12:12:18]

Patchset 3 (id:??) landed as
https://crrev.com/7db1759b656ecdc017236165c756ceb4488f5711
Cr-Commit-Position: refs/heads/master@{#26022}