Bug 24459 – Add v8 bindings for event.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[David Levin]

See summary.

[David Levin]

Created attachment 28410 [details]
Proposed fix.

[Eric Seidel (no email)]

Comment on attachment 28410 [details]
Proposed fix.

I don't think we really want/need this logging code:
 41 #define IF_DEVEL(stmt) ((void) 0)

These should be set using C++ constructor syntax:
52     // Get the position in the source if any.
 53     m_lineNumber = 0;
 54     m_columnNumber = 0;

Lots of variables with teh wrong naming style: event_symbol should be eventSymbol.

Unfortunatly I have to run catch a train atm.  Dimitri definitely knows the style quite well tehse days and could help you pick out many of the style issues in this patch.

[Dimitri Glazkov (Google)]

Comment on attachment 28410 [details]
Proposed fix.

I made a first pass, need Eric's blessing. Overall, it looks good, aside from style issues.

The only concern is the use of empty V8 return value and figuring out what that actually means. In my experience, this is V8 interceptor-related and I added notHandledByInterceptor() helper to make the value more descriptive. So that's a great "learn V8 bindings" assignment for ya :)

> +        * bindings/v8/custom/V8AbstractEventListener.cpp: Added.
> +        (WebCore::V8AbstractEventListener::V8AbstractEventListener):
> +        (WebCore::V8AbstractEventListener::HandleEventHelper):
> +        (WebCore::V8AbstractEventListener::handleEvent):
> +        (WebCore::V8AbstractEventListener::DisposeListenerObject):
> +        (WebCore::V8AbstractEventListener::GetReceiverObject):

Here and below: no need to keep method names for newly added files unless you have a valuable comment that goes with them.

> diff --git a/WebCore/bindings/v8/custom/V8AbstractEventListener.cpp b/WebCore/bindings/v8/custom/V8AbstractEventListener.cpp
> +
> +#include "config.h"
> +

No line break needed.

> +#include "V8AbstractEventListener.h"

> +#define IF_DEVEL(stmt) ((void) 0)
I don't think we need this macro. It looks like a carryover from some debugging exercise.

> +
> +void V8AbstractEventListener::HandleEventHelper(v8::Handle<v8::Context> context, Event* event, bool isWindowEvent)

*Helper is a bad name. Perhaps invokeEventHandler?

> +    // Enter the V8 context in which to perform the event handling.
> +    v8::Context::Scope scope(context);
> +
> +    // Get the V8 wrapper for the event object.
> +    v8::Handle<v8::Value> jsevent = V8Proxy::EventToV8Object(event);

jsEvent

> +    // For compatibility, we store the event object as a property on the window called "event".  Because this is the global namespace, we save away any
> +    // existing "event" property, and then restore it after executing the javascript handler.
> +    v8::Local<v8::String> event_symbol = v8::String::NewSymbol("event");

eventSymbil

> +    v8::Local<v8::Value> saved_evt;

savedEvent

> +    v8::Local<v8::Value> ret;

returnValue

> +
> +    {
> +        // Catch exceptions thrown in the event handler so they do not propagate to javascript code that caused the event to fire.
> +        // Setting and getting the 'event' property on the global object can throw exceptions as well (for instance if accessors that
> +        // throw exceptions are defined for 'event' using __defineGetter__ and __defineSetter__ on the global object).
> +        v8::TryCatch try_catch;

tryCatch

> +        try_catch.SetVerbose(true);
> +
> +        // Save the old 'event' property so we can restore it later.
> +        saved_evt = context->Global()->Get(event_symbol);
> +        try_catch.Reset();
> +
> +        // Make the event available in the window object.
> +        //
> +        // TODO: This does not work as in safari if the window.event property is already set. We need to make sure that property

TODO->FIXME. In Safari? That seems like a strange comment. Perhaps the author meant "with JSC bindings?"

> +        // access is intercepted correctly.
> +        context->Global()->Set(event_symbol, jsevent);
> +        try_catch.Reset();
> +
> +        // Call the event handler.
> +        ret = CallListenerFunction(jsevent, event, isWindowEvent);

callListenerFunction

> +        try_catch.Reset();
> +
> +        // Restore the old event. This must be done for all exit paths through this method.
> +        if (saved_evt.IsEmpty())
> +            context->Global()->Set(event_symbol, v8::Undefined());
> +        else
> +            context->Global()->Set(event_symbol, saved_evt);
> +        try_catch.Reset();
> +    }
> +
> +    ASSERT(!V8Proxy::HandleOutOfMemory() || ret.IsEmpty());
> +
> +    if (ret.IsEmpty())
> +        return;

Is the check below needed?

> +    if (!ret.IsEmpty()) {
> +        if (!ret->IsNull() && !ret->IsUndefined() && event->storesResultAsString())
> +            event->storeResult(ToWebCoreString(ret));

toWebCoreString

> +    v8::HandleScope handle_scope;

handleScope

> +    v8::Handle<v8::Context> context = V8Proxy::GetContext(m_frame);
> +    if (context.IsEmpty())
> +        return;
> +
> +    // m_frame can removed by the callback function, protect it until the callback function returns.
> +    RefPtr<Frame> protector(m_frame);
> +
> +    IF_DEVEL(log_info(frame, "Handling DOM event", m_frame->document()->URL()));
> +
> +    HandleEventHelper(context, event, isWindowEvent);
> +
> +    Document::updateDocumentsRendering();
> +}
> +
> +void V8AbstractEventListener::DisposeListenerObject()

disposeListenerObject

> +{
> +    if (!m_listener.IsEmpty()) {
> +#ifndef NDEBUG
> +        V8Proxy::UnregisterGlobalHandle(this, m_listener);
> +#endif
> +        m_listener.Dispose();
> +        m_listener.Clear();
> +    }
> +}
> +
> +v8::Local<v8::Object> V8AbstractEventListener::GetReceiverObject(Event* event, bool isWindowEvent)
> +{

getReceiverObject

> +    if (!m_listener.IsEmpty() && !m_listener->IsFunction())
> +        return v8::Local<v8::Object>::New(m_listener);
> +
> +    if (isWindowEvent)
> +        return v8::Context::GetCurrent()->Global();
> +
> +    EventTarget* target = event->currentTarget();
> +    v8::Handle<v8::Value> value = V8Proxy::EventTargetToV8Object(target);
> +    if (value.IsEmpty())
> +        return v8::Local<v8::Object>();

Is this interceptor-related? If so, we should use notHandledByInterceptor();

> +    return v8::Local<v8::Object>::New(v8::Handle<v8::Object>::Cast(value));
> +}
> +

> +        // Call listener function.
> +        virtual v8::Local<v8::Value> CallListenerFunction(v8::Handle<v8::Value> jsevent, Event*, bool isWindowEvent) = 0;

callListenerFunction

> diff --git a/WebCore/bindings/v8/custom/V8AttrCustom.cpp b/WebCore/bindings/v8/custom/V8AttrCustom.cpp
>   *
>   * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> - * "AS IS" AND ANY EXPRESS OR iframeLIED WARRANTIES, INCLUDING, BUT NOT
> - * LIMITED TO, THE iframeLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>   * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT

Whoopsie! :)

> diff --git a/WebCore/bindings/v8/custom/V8CustomEventListener.cpp b/WebCore/bindings/v8/custom/V8CustomEventListener.cpp

> +{
> +    // It could be disposed already.
> +    if (m_listener.IsEmpty())
> +        return v8::Local<v8::Function>();

Same interceptor concern,

> +v8::Local<v8::Value> V8EventListener::CallListenerFunction(v8::Handle<v8::Value> jsevent, Event* event, bool isWindowEvent) {
> +    v8::Local<v8::Function> handler_func = GetListenerFunction();

handlerFunction

> diff --git a/WebCore/bindings/v8/custom/V8LazyEventListener.cpp b/WebCore/bindings/v8/custom/V8LazyEventListener.cpp
> +#define IF_DEVEL(stmt) ((void) 0)

Not needed.

> +
> +    {
> +        // Switch to the contex of m_frame
> +        v8::HandleScope handle_scope;

handleScope

> +        if (context.IsEmpty()) return v8::Local<v8::Function>();

return indented on next line.

> +                v8::Local<v8::Function> listener_func = v8::Local<v8::Function>::Cast(value);

listenerFunction

> +v8::Local<v8::Value> V8LazyEventListener::CallListenerFunction(v8::Handle<v8::Value> jsevent, Event* event, bool isWindowEvent)

jsEvent

> +        // Switch to the contex of m_frame
> +        v8::HandleScope handle_scope;

handleScope

> +                // Set the function name.
> +                m_wrappedFunction->SetName(v8::String::New(FromWebCoreString(m_functionName), m_functionName.length()));

Let's define a fromWebCoreString wrapper in V8Binding.h, so that we could temporarily use both upper and lowercase, just like toWebCoreString

> diff --git a/WebCore/bindings/v8/custom/V8LazyEventListener.h b/WebCore/bindings/v8/custom/V8LazyEventListener.h
> +static void WeakObjectEventListenerCallback(v8::Persistent<v8::Value> obj, void* para)

parameter?

> +    v8::HandleScope handle_scope;

handleScope

> +    v8::Handle<v8::Context> context = m_proxy->GetContext();
> +v8::Local<v8::Value> V8WorkerContextEventListener::CallListenerFunction(v8::Handle<v8::Value> jsevent, Event* event, bool isWindowEvent)
> +{
> +    v8::Local<v8::Function> handler_func = GetListenerFunction();

handlerFunction.

> diff --git a/WebCore/bindings/v8/custom/V8WorkerContextEventListener.h b/WebCore/bindings/v8/custom/V8WorkerContextEventListener.h

[David Levin]

Created attachment 28453 [details]
Proposed fix.

I think I've addressed all style issues.

> The only concern is the use of empty V8 return value and figuring out what that actually means. 
As we found out for these cases, it is how V8 returns 'null'.  Usually it means not-found.

[David Levin]

Committed r41600.