[webkit-reviews] review granted: [Bug 40484] Add beforeScript (or equivalent) event to fire on inline scripts and inline stylesheets : [Attachment 59958] Rework createAttributeEventListener

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Brady Eidson
<beidson at apple.com>'s request for review:
Bug 40484: Add beforeScript (or equivalent) event to fire on inline scripts and
inline stylesheets
https://bugs.webkit.org/show_bug.cgi?id=40484

Attachment 59958: Rework createAttributeEventListener
https://bugs.webkit.org/attachment.cgi?id=59958&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> +PassRefPtr<JSLazyEventListener> createWindowAttributeEventListener(Node*
windowEquivalentNode, Attribute* attr)
>  {
> +    Frame* frame = windowEquivalentNode->document()->frame();

I would reinforce this by asserting that the node is actually one of those
elements. Oh, and maybe pass Element*, not Node*.

> -    return JSLazyEventListener::create(attr->localName().string(),
eventParameterName(frame->document()->isSVGDocument()), attr->value(), 0,
sourceURL, lineNumber, wrapper, mainThreadNormalWorld());
> +    return JSLazyEventListener::create(attr->localName().string(),
eventParameterName(frame->document()->isSVGDocument()), attr->value(),
windowEquivalentNode, sourceURL, lineNumber, wrapper, mainThreadNormalWorld());


Please add a comment in JSLazyEventListener.h explaining that m_wrapper isn't
necessarily a wrapper for m_originalNode, but is created from it if the wrapper
is null. You may want to remove "if (m_originalNode)" condition in
JSLazyEventListener::initializeJSFunction(), as it's seemingly always true now.


It will be a fairly confusing detail that wrapper doesn't always match
m_originalNode.

r=me with comments addressed.