Summary: | [v8] Missing wasCreatedFromMarkup fn in V8LazyEventListener.h | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Robert Kroeger <rjkroege> | ||||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, dglazkov, japhet | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Robert Kroeger
2010-02-23 19:47:56 PST
Created attachment 49355 [details] Patch to fix bug 35325 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. (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. Created attachment 49604 [details] patch for bug 35325 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. Comment on attachment 49604 [details] patch for bug 35325 Clearing flags on attachment: 49604 Committed r55326: <http://trac.webkit.org/changeset/55326> All reviewed patches have been landed. Closing bug. |