Bug (CPPEventListeners) – [Windows, WinCairo] Implement ability to add C++ event listeners to html dom elements and dom window

• Component: WebKit API
• Status: RESOLVED FIXED

[Anthony Johnson]

Created attachment 92420 [details]
Patch for adding event listeners

The included patch adds the ability to attach C++ event listeners to an html dom element and to a dom window for the wincairo port.

To give an idea of what I mean, here's some pseudo-code:
// for dom elements
domElement = webFrame->DOMDocument()->getElementById("myelement");
domElement->addEventListener("click", myCPPElementEventListener, false);

// for dom windows
domWindow = webFrame->DOMWindow();
domWindow->addEventListener("focus", myCPPWindowEventListener, false);


Of course, the actual c++ code is a little more involved, but I'll include that as well (minus all the error checking).
// for dom elements
CComPtr<IDOMElement> domDocument;
webFrame->DOMDocument(&domDocument);

CComPtr<IDOMElement> domElement;
domDocument->getElementById("myelement", &domElement);
CComPtr<IDOMEventTarget> target;
domElement->QueryInterface(IID_IDOMEventTarget, &target);
// myCPPElementEventListener is an instantiation of a class I (the end user) define that implements IDOMEventListener
target->addEventListener(TEXT("click"), static_cast<IDOMEventListener*>(myCPPElementEventListener), false); 

// for dom windows
CComPtr<IDOMWindow> domWindow;
webFrame->DOMWindow(&domWindow);
CComPtr<IDOMEventTarget> target;
domWindow->QueryInterface(IID_IDOMEventTarget, &target);
// myCPPWindowEventListener is an instantiation of a class I (the end user) define that implements IDOMEventListener
target->addEventListener(TEXT("focus"), static_cast<IDOMEventListener*>(myCPPWindowEventListener), false);

Now, an explanation of my design.
1. I added a class called CPPEventListener, which inherits WebCore::EventListener and is a wrapper for an IDOMEventListener*, passing events on to that user-defined interface. As far as location of the class definition, I defined the class in DOMEventsClasses.h, and defined the methods in DOMEventsClasses.cpp. Not sure if that's the best spot to put it.
2. I filled in the DOMNode::addEventListener() and DOMNode::removeEventListener() methods, which instantiates the CPPEventListener class and adds it to the webcore node's event listeners.
3. I defined a DOMWindow class that implements the IDOMWindow interface as well as the IDOMEventTarget interface, and is a wrapper around WebCore::DOMWindow.
4. I added a DOMWindow() method to the IWebFrame interface and WebFrame class, which instantiates a DOMWindow object and returns its IDOMWindow interface.

Additional points:
- You may want to look at my use of RefPtr in DOMNode::addEventListener(), DOMNode::removeEventListener(), DOMWindow::addEventListener(), and DOMWindow::removeEventListener(). I'm not yet an expert on RefPtr, so I may not have done it in the best-practices method.
- My creation of the CPPEventListener sub-class WebCore::EventListener seems to me to be the most precarious, because I don't know if it's proper to sub-class a WebCore class in a platform-specific port. But that seemed to be the only way I could think of to get it to dispatch events to my own code.

Thanks,
Anthony Johnson

[Brent Fulgham]

Comment on attachment 92420 [details]
Patch for adding event listeners

View in context: https://bugs.webkit.org/attachment.cgi?id=92420&action=review

> Source/WebKit/win/DOMCoreClasses.cpp:409
> +    RefPtr<CPPEventListener> cpplistener = adoptRef(new CPPEventListener(listener));

I don't think it's necessary to prefix the classname with CPP.  Maybe call it something like "CustomEventListener" or "UserEventListener", or even "EventListenerAdapter"?

> Source/WebKit/win/DOMCoreClasses.cpp:423
> +    m_node->removeEventListener(type, (WebCore::EventListener*)cpplistener.get(), useCapture);

You should use a C++ cast here.

> Source/WebKit/win/DOMCoreClasses.cpp:894
> +    RefPtr<CPPEventListener> cppListener = adoptRef(new CPPEventListener(listener));

Ditto re: naming.

> Source/WebKit/win/DOMCoreClasses.cpp:907
> +    m_window->removeEventListener(type, (WebCore::EventListener*)cpplistener.get(), useCapture);

Ditto re: Casting

> Source/WebKit/win/DOMCoreClasses.cpp:916
> +        return E_FAIL;

Shouldn't you be checking result as well?  I'd check evt & result for null and return E_POINTER, and return E_FAIL if m_window is null.

> Source/WebKit/win/DOMCoreClasses.cpp:919
> +    if (![self _node]->isEventTargetNode())

Do we have a C++ analog for this call?  Which DOM object are we expecting to get here?

> Source/WebKit/win/DOMCoreClasses.cpp:931
> +    WebCore::raiseOnDOMError(ec);

Is this not implemented for the C++ backend? I'm not sure why it's commented out, since we should be getting an exception code in the previous statement.

> Source/WebKit/win/DOMCoreClasses.cpp:957
> +    hr = newWindow->QueryInterface(IID_IDOMWindow, (void**)&domWindow);

This should be a C++ cast, even though others in the file are still using C-style casts.

> Source/WebKit/win/DOMEventsClasses.cpp:74
> +    DOMUIEvent * domUIEvent = new DOMUIEvent(ePtr);

This would be better as a COMPtr (see #include <WebCore/COMPtr.h>).

> Source/WebKit/win/DOMEventsClasses.cpp:76
> +    IDOMEvent* iDOMUIEvent = 0;

Ditto regarding COMPtr.

> Source/WebKit/win/DOMEventsClasses.cpp:77
> +    domUIEvent->QueryInterface(IID_IDOMEvent, (void**)&iDOMUIEvent);

This should be a C++-style cast.

> Source/WebKit/win/Interfaces/IWebFrame.idl:104
> +

Note: You need to add new methods at the bottom of the IDL file to avoid breaking existing Windows clients.

> Source/WebKit/win/WebFrame.cpp:483
> +HRESULT STDMETHODCALLTYPE WebFrame::DOMWindow(

You don't need to declare the STDMETHODCALLTYPE here (it's in the header), even though other methods in this file use this redundant declaration.

[Brent Fulgham]

This patch is relevant to both the standard Windows port and the WinCairo port.

I added some style comments, but I'm not really qualified to discuss the architectural merits of this approach, though at a glance it looks like a good idea.

I'm wondering if there might be a way to get rid of the helper class you created, and have this functionality be part of the IDOMEventListener* directly?

[Alexey Proskuryakov]

Obviously, the most difficult part here is getting object lifetime issues right. I think that DOM JS garbage collection relies on listeners being JavaScript objects.

[Geoffrey Garen]

(In reply to comment #3)
> Obviously, the most difficult part here is getting object lifetime issues right. 

This biggest potential lifetime problem is that the listener may create a reference cycle, if the listener object directly or indirectly references the event target.

This problem is unique to reference counted environments -- GC can break cycles; reference counting cannot.

> I think that DOM JS garbage collection relies on listeners being JavaScript objects.

This shouldn't be a problem. We already support a bunch of non-JS listener types. Search for "public EventListener" to see them all.

[Geoffrey Garen]

By the way, I think "CPPEventListener" is a bad name. The listener type should be named after the environment in which it will be used, not the programming language. We have lots of event listeners that are implemented in C++.

I would suggest COMEventListener, or something similar.

[Anthony Johnson]

If I make these changes mentioned, do I just submit a new patch file?

[Anthony Johnson]

> I'm wondering if there might be a way to get rid of the helper class you created, and have this functionality be part of the IDOMEventListener* directly?

I don't see a possibility right now. Since it seems to me like the design is such that the user accessing the API would only go through the COM interface, they would not have access to the definition of WebCore::EventListener, and in order to override WebCore::EventListener::handleEvent(), they'd need access to the class definition.

[Brent Fulgham]

(In reply to comment #6)
> If I make these changes mentioned, do I just submit a new patch file?

Yes.  Just mark the old version as obsolete.  You should also mark the patch as "?" to get it in the review queue.

[Anthony Johnson]

(In reply to comment #4)

> This biggest potential lifetime problem is that the listener may create a reference cycle, if the listener object directly or indirectly references the event target.
> 
> This problem is unique to reference counted environments -- GC can break cycles; reference counting cannot.
> 

Is there a way to prevent or discourage reference cycles through the implementation?

[Brent Fulgham]

(In reply to comment #7)
> > I'm wondering if there might be a way to get rid of the helper class you created, and have this functionality be part of the IDOMEventListener* directly?
> 
> I don't see a possibility right now. Since it seems to me like the design is such that the user accessing the API would only go through the COM interface, they would not have access to the definition of WebCore::EventListener, and in order to override WebCore::EventListener::handleEvent(), they'd need access to the class definition.

The more I look at this, the less I understand the design of the listener handling.  What is the point of an IDOMEventListener, if it can't be round-tripped through WebCore such that its 'handleEvent' method ends up getting called?

Are we just not understanding how this is meant to be connected to WebCore?

[Anthony Johnson]

(In reply to comment #10)
> (In reply to comment #7)
> > > I'm wondering if there might be a way to get rid of the helper class you created, and have this functionality be part of the IDOMEventListener* directly?
> > 
> > I don't see a possibility right now. Since it seems to me like the design is such that the user accessing the API would only go through the COM interface, they would not have access to the definition of WebCore::EventListener, and in order to override WebCore::EventListener::handleEvent(), they'd need access to the class definition.
> 
> The more I look at this, the less I understand the design of the listener handling.  What is the point of an IDOMEventListener, if it can't be round-tripped through WebCore such that its 'handleEvent' method ends up getting called?
> 
> Are we just not understanding how this is meant to be connected to WebCore?

First, please realize that a lot of this was already implemented with empty stubs when I got to it. I'm just trying to fill it out like I imagine the original guy would have. I think the idea is that an end user would only see the COM interfaces, i.e. IDOMEventTarget, IDOMEventListener, presumably so that the interfaces can be accessed via non-c++ functionality as well as c++ functionality. So, if I as an end user just see the interface definitions, then in order to get the stuff to call my handleEvent() method, I essentially have to define my own class that implements the IDOMEventListener interface:

class AnthonyEventListener : public IDOMEventListener {
  handleEvent(IDOMEvent* e);
  ...
};

Then when I need to attach to an element event, I create an instance of AnthonyEventListener, then pass my instantiation to IDOMEventTarget::addEventListener(). At that point I assume that when the event is fired, my handleEvent() method will be fired.

Now, in order for the implementation to do a direct pass-through from WebCore to my class handleEvent() method, my class definition would also need to inherit from WebCore::EventListener.

class AnthonyEventListener : public WebCore::EventListener, public IDOMEventListener{
  handleEvent(WebCore::ScriptExecutionContext* s, WebCore::Event* e);// EventListener override
  handleEvent(IDOMEvent* e); // IDOMEventListener override
  ...
}

There are a few issues with this. First, the end-user would need to sub-class WebCore::EventListener, which by design they should not have a definition of (they just see the COM interfaces). Second, IDOMEventTarget::addEventListener() would have to assume that the IDOMEventListener passed would also be a sub-class of WebCore::EventListener, because it needs to translate it into that in order to call WebCore::Node::addEventListener(), which isn't very good design and would break for non-c++ COM bindings.

So, it seemed to me that the best way to do it would be to create a go-between class within the COM implementation, that handles the translation from IDOMEventListener to WebCore::EventListener. The class inherits from WebCore::EventListener, so it can be added using WebCore::Node::addEventListener() and its handleEvent() method will be properly called, and it also holds a pointer to an IDOMEventListener (the user's implementation), so that it can pass the handleEvent to the IDOMEventListener. In this way, the end-user can just worry about implementing his own IDOMEventListener interface, and the go-between will automatically handle the COM-to-WebCore translation and back.

[Brent Fulgham]

(In reply to comment #11)
> > Are we just not understanding how this is meant to be connected to WebCore?
> 
> First, please realize that a lot of this was already implemented with empty stubs when I got to it. I'm just trying to fill it out like I imagine the original guy would have. I think the idea is that an end user would only see the COM interfaces, i.e. IDOMEventTarget, IDOMEventListener, presumably so that the interfaces can be accessed via non-c++ functionality as well as c++ functionality. 

Right -- I understand you are working with the existing stubs.  I just meant that I don't get why you were forced to go through an extra layer to connect up to WebCore.  This should work along the lines of WebHistory or one of the other API calls, where the COM object is converted internally to the correct WebCore type.

I guess you are basically creating this layer, though I would have thought that WebCore would already have had an analogous object that could be filled in with the contents of the IDOMEventListener such that it would get triggered appropriately.

[Anthony Johnson]

> Right -- I understand you are working with the existing stubs.  I just meant that I don't get why you were forced to go through an extra layer to connect up to WebCore.  This should work along the lines of WebHistory or one of the other API calls, where the COM object is converted internally to the correct WebCore type.
> 
> I guess you are basically creating this layer, though I would have thought that WebCore would already have had an analogous object that could be filled in with the contents of the IDOMEventListener such that it would get triggered appropriately.

Sure, if there's a class or translation mechanism that already does it, let me know. I'm still very new and untrained in all this.

[Adam Roben (:aroben)]

(In reply to comment #5)
> By the way, I think "CPPEventListener" is a bad name. The listener type should be named after the environment in which it will be used, not the programming language. We have lots of event listeners that are implemented in C++.
> 
> I would suggest COMEventListener, or something similar.

A more typical name for a class in WebKit would be "WebEventListener".

(In reply to comment #12)
> (In reply to comment #11)
> > > Are we just not understanding how this is meant to be connected to WebCore?
> > 
> > First, please realize that a lot of this was already implemented with empty stubs when I got to it. I'm just trying to fill it out like I imagine the original guy would have. I think the idea is that an end user would only see the COM interfaces, i.e. IDOMEventTarget, IDOMEventListener, presumably so that the interfaces can be accessed via non-c++ functionality as well as c++ functionality. 
> 
> Right -- I understand you are working with the existing stubs.  I just meant that I don't get why you were forced to go through an extra layer to connect up to WebCore.  This should work along the lines of WebHistory or one of the other API calls, where the COM object is converted internally to the correct WebCore type.
> 
> I guess you are basically creating this layer, though I would have thought that WebCore would already have had an analogous object that could be filled in with the contents of the IDOMEventListener such that it would get triggered appropriately.

I think the approach taken in this patch makes sense. We do similar things in other parts of the WebKit API.

[Anthony Johnson]

Comment on attachment 92420 [details]
Patch for adding event listeners

I'm setting this to rejected. I'll post a new patch once I've got it done.

[Anthony Johnson]

Created attachment 92613 [details]
Patch for adding event listeners v2

[Brent Fulgham]

Comment on attachment 92613 [details]
Patch for adding event listeners v2

View in context: https://bugs.webkit.org/attachment.cgi?id=92613&action=review

This looks great.  I hope we can land this soon, as this would be useful in a variety of contexts!

> DOMCoreClasses.cpp:409
> +	RefPtr<WebEventListener> cpplistener = WebEventListener::create(listener);

I'd change 'cpplistener' to 'webListener'

> DOMCoreClasses.cpp:421
> +		return E_POINTER;

Extra indention

> DOMCoreClasses.cpp:424
> +    RefPtr<WebEventListener> cpplistener = WebEventListener::create(listener);

Ditto: 'cpplistener' -> 'webListener'

> DOMCoreClasses.cpp:434
> +		return E_POINTER;

Ditto: indent

> DOMCoreClasses.cpp:897
> +		return E_POINTER;

Ditto: Indent

> DOMCoreClasses.cpp:900
> +    RefPtr<WebEventListener> cppListener = adoptRef(new WebEventListener(listener));

cppListener -> webListener

> DOMCoreClasses.cpp:911
> +		return E_POINTER;

Ditto: Indent

> DOMCoreClasses.cpp:914
> +	RefPtr<WebEventListener> cpplistener = WebEventListener::create(listener);

Ditto: cppListener -> webListener

> DOMCoreClasses.cpp:929
> +    if (![self _node]->isEventTargetNode())

What is your plan for this logic?  I assume this is copied from the Cocoa implementation.  Is there an analogous method we can be calling in our C++ code?

> DOMCoreClasses.cpp:941
> +    WebCore::raiseOnDOMError(ec);

Is this not implemented in C++?  Why is it commented out?

> DOMEventsClasses.cpp:63
> +}

Style guide requires an empty line between these implementations.

> DOMEventsClasses.cpp:67
> +}

Ditto

> DOMEventsClasses.cpp:72
> +}

Ditto

[Brent Fulgham]

I haven't seen anyone address the question of object lifetime raised by ggaren and ap. Do we need to be doing anything in the COM layer to make sure we don't create problems?

[Anthony Johnson]

Created attachment 92636 [details]
Patch for adding event listeners v3

Implemented Brent's styling suggestions

[Anthony Johnson]

> What is your plan for this logic?  I assume this is copied from the Cocoa implementation.  Is there an analogous method we can be calling in our C++ code?
> 
> > DOMCoreClasses.cpp:941
> > +    WebCore::raiseOnDOMError(ec);
> 
> Is this not implemented in C++?  Why is it commented out?
> 

It seems like my annotation comments don't generate a new entry in the main comment stream. Anyway, in case you didn't see my reply on that, the code was a straight copy from DOMNode::dispatchEvent() at line 429. I only changed m_node to m_window, and left everything else the same. Being a newbie, I'm going for least impact.

[Brent Fulgham]

Comment on attachment 92636 [details]
Patch for adding event listeners v3

View in context: https://bugs.webkit.org/attachment.cgi?id=92636&action=review

This looks great!  Let's get ggaren or Adam to review object lifecycle, and we need to figure out what to do about the "isEventTargetNode" test you have commented out.

> DOMCoreClasses.cpp:929
> +    if (![self _node]->isEventTargetNode())

I think this is the only remaining question -- what goes here?

[Brent Fulgham]

It looks like the patch doesn't apply cleanly against ToT (see http://queues.webkit.org/results/8611068).  Someone must have added IDL changes in the last day or so.  :)

[Brent Fulgham]

(In reply to comment #20)
> > What is your plan for this logic?  I assume this is copied from the Cocoa implementation.  Is there an analogous method we can be calling in our C++ code?
> > 
> > > DOMCoreClasses.cpp:941
> > > +    WebCore::raiseOnDOMError(ec);
> > 
> > Is this not implemented in C++?  Why is it commented out?
> > 
> 
> It seems like my annotation comments don't generate a new entry in the main comment stream. Anyway, in case you didn't see my reply on that, the code was a straight copy from DOMNode::dispatchEvent() at line 429. I only changed m_node to m_window, and left everything else the same. Being a newbie, I'm going for least impact.

Ah!  Makes sense.  Let's see if we can find ggaren/ap/aroben to finish this review.

[Anthony Johnson]

> Ah!  Makes sense.  Let's see if we can find ggaren/ap/aroben to finish this review.

Reviewers, please don't forget to look at my WebEventListener::== operator, and advise whether it's OK for WebEventListener to take exclusivity on the enum type WebCore::EventListener::CPPEventListenerType, or whether I should create a new WebCore::EventListener::WebEventListenerType for my class.

[Alexey Proskuryakov]

See also: <http://trac.webkit.org/changeset/48884>, which removed  dysfunctional code for COM event listeners on Windows. Not sure if there is anything to learn from that, but perhaps someone will remember something that's important for this patch :)

[Geoffrey Garen]

Comment on attachment 92636 [details]
Patch for adding event listeners v3

View in context: https://bugs.webkit.org/attachment.cgi?id=92636&action=review

The WebCore code here looks fine to me. I'm not sure who should review the COM code.

> DOMEventsClasses.cpp:59
> +    :EventListener(CPPEventListenerType)

Needs a space after ":".

[Brent Fulgham]

Created attachment 94014 [details]
Patch

[Brent Fulgham]

I updated Anthony's patch with one that passes code review, and that should apply cleanly on the EWS bots.

After doing some research, I tracked down the answer about the Cocoa "isEventNodeTarget" accessor (no longer used), and added logic for handling any exception conditions in the dispatch.

[Brent Fulgham]

I double-checked the COM changes with bweinstein and confirmed that they are okay.

[Brent Fulgham]

Comment on attachment 94014 [details]
Patch

Double-checked with bweinstein to confirm COM implementation is correct.

[Brent Fulgham]

Landed patch in http://trac.webkit.org/changeset/87000.

[Brent Fulgham]

Comment on attachment 94014 [details]
Patch

Clearing review flag so I can leave this bug open while a new test is added.

[Brent Fulgham]

Created attachment 95029 [details]
Patch

[Brent Fulgham]

Closing as the feature is in place. A new bug will be used for a test case, and an example program.