rubyjs / therubyracer

Embed the V8 Javascript Interpreter into Ruby
1.67k stars 190 forks source link

Implement methods for Template, FunctionTemplate & ObjectTemplate #385

Open georgyangelov opened 8 years ago

georgyangelov commented 8 years ago

A ton of callbacks here :) Not sure if we will need them all though.

georgyangelov commented 8 years ago

@cowboyd, do you have an idea of a reason why Template::SetNativeDataProperty callbacks would not be called when doing template.NewInstance.Get(@ctx, key)?

cowboyd commented 8 years ago

Awesome to see this! I'm travelling today, but will have a look at these tomorrow.

I know it seems like a lot of callbacks, but if I recall correctly, there are at least four that are necessary to implement complete "method_missing" behavior for Ruby objects.

I can't say off the top of my head why the SetNativeDataProperty would not be invoked when doing a NewInstance().Get(). It certainly seems like the docs suggest that it should. I can investigate further on this tomorrow, but it might be a good question for the v8-users list.

ignisf commented 8 years ago

I kind of liked having you in our time zone :)

cowboyd commented 8 years ago

I know it was nice, right? Welp. Next summer :)

cowboyd commented 8 years ago

Looking good! That is a lot of callbacks, but this really is the last piece in the puzzle before we can start putting it all together.

cowboyd commented 8 years ago

I actually agree with you about the readability aspect here, and I want to make sure that this codebase is as readable as possible. I wonder if there is some common ground here that preserves readability and also safety. Maybe something like:

PropertyCallbackData data(isolate, rb_data);
 t->SetAccessor(
        *Name(r_name),
        AccessorGetter(getter, data),
        AccessorSetter(setter, data),
        data,
        Enum<v8::AccessControl>(r_settings, v8::DEFAULT),
        Enum<v8::PropertyAttribute>(r_attribute, v8::None),
        AccessorSignature(r_signature)
      );

Hate to thrash on this, but this is one of the trickier bits of the codebase, and so it's important to get right. If you don't think it's worth it, then we'll go ahead and merge, but seeing it in action, it is quite "WTF" to see callback repeated 5 times.

georgyangelov commented 8 years ago

I completely agree we should get this right. I'm thinking something like this:

PropertyCallback callback(isolate, getter, setter, rb_data);
 t->SetAccessor(
        *Name(r_name),
        callback.getter(),
        callback.setter(),
        data,
        Enum<v8::AccessControl>(r_settings, v8::DEFAULT),
        Enum<v8::PropertyAttribute>(r_attribute, v8::None),
        AccessorSignature(r_signature)
      );

where getter and setter return a function pointer.

The example you gave wouldn't work since the data should already contain the Ruby proc objects so that we can pass it to the data parameter of SetAccessor.

cowboyd commented 8 years ago

Sounds good!

georgyangelov commented 8 years ago

Done, sorry for taking so long..

Do you want me to squash this?

cowboyd commented 8 years ago

No worries, and no need to squash unless you want to. I've been crazy busy at work, but hoping to merge this, this week.