Bug 35325 – [v8] Missing wasCreatedFromMarkup fn in V8LazyEventListener.h

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Robert Kroeger]

To fire each event handler registered on an SVG node once per event, Chromium needs an implementation of wasCreatedFromMarkup added to V8LazyEventListener.h that matches the one in WebKit/WebCore/bindings/js/JSLazyEventListener.h.

[Robert Kroeger]

Created attachment 49355 [details]
Patch to fix bug 35325

[Nate Chapin]

Comment on attachment 49355 [details]
Patch to fix bug 35325

> +        Patch was tested and passed the Chromium try bots.

Try bot info shouldn't go in the ChangeLog. If you're going to include it, leave it in the bug.

> +        // Needed to match JSC semantics inside shared SVG code.
> +        virtual bool wasCreatedFromMarkup() const { return true; }
> +

This comment doesn't really help me understand why this change is necessary.  It would probably be better off explaining why either set of javascript bindings needs to override EventListener::wasCreatedFromMarkup(), rather than just saying that we're matching JSC. Alternatively, you could present a case why a comment isn't necessary here :)


r- on nitpicks.

[Robert Kroeger]

(In reply to comment #2)
> (From update of attachment 49355 [details])
> > +        Patch was tested and passed the Chromium try bots.
> 
> Try bot info shouldn't go in the ChangeLog. If you're going to include it,
> leave it in the bug.

This is easily removed. I'll upload a new patch without it after dealing with the below.

> 
> > +        // Needed to match JSC semantics inside shared SVG code.
> > +        virtual bool wasCreatedFromMarkup() const { return true; }
> > +
> 
> This comment doesn't really help me understand why this change is necessary. 
> It would probably be better off explaining why either set of javascript
> bindings needs to override EventListener::wasCreatedFromMarkup(), rather than
> just saying that we're matching JSC. Alternatively, you could present a case
> why a comment isn't necessary here :)

I'm not sure how to address this comment. 

I'd be happy to remove the comment. Deleting is easy. :-) Argument in favour of removing the comment: it would match the brevity of the un-commented  JSLazyEventListener::wasCreatedFromMarkup()

I know how the presence of JSLazyEventListener::wasCreatedFromMarkup changes the behaviour of the SVG event handling code: this method returns true if the clone of a SVG Node (into the shadow tree) includes event handlers already or if they need to be added in a second pass over the DOM.

I think that this patch (without the comment) is the smallest amount of code that corrects Chromium's SVG event dispatch behaviour to match with WebKit. 

Please advise me further on what I need to do to make the patch acceptable.

> 
> 
> r- on nitpicks.

[Robert Kroeger]

Created attachment 49604 [details]
patch for bug 35325

[Nate Chapin]

Comment on attachment 49604 [details]
patch for bug 35325

Great, thanks!

> +        Reviewed by Nate Chapin

For future reference, typically we just leave the "Reviewed by NOBODY (OOPS!)." in place until a patch has received an r+.  The commit queue scripts will take care of replace it with the name of the person who actually approved the patch.

[WebKit Commit Bot]

Comment on attachment 49604 [details]
patch for bug 35325

Clearing flags on attachment: 49604

Committed r55326: <http://trac.webkit.org/changeset/55326>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.