35325 2010-02-23 19:47:56 -0800 [v8] Missing wasCreatedFromMarkup fn in V8LazyEventListener.h 2010-02-26 17:18:04 -0800 1 1 1 Unclassified WebKit SVG 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 rjkroege webkit-unassigned commit-queue dglazkov japhet oldest_to_newest 193220 0 rjkroege 2010-02-23 19:47:56 -0800 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. 193225 1 49355 rjkroege 2010-02-23 20:30:14 -0800 Created attachment 49355 Patch to fix bug 35325 193806 2 49355 japhet 2010-02-25 10:37:44 -0800 Comment on attachment 49355 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. 193812 3 rjkroege 2010-02-25 11:18:00 -0800 (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. 194182 4 49604 rjkroege 2010-02-26 11:32:12 -0800 Created attachment 49604 patch for bug 35325 194184 5 49604 japhet 2010-02-26 11:36:19 -0800 Comment on attachment 49604 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. 194371 6 49604 commit-queue 2010-02-26 17:18:00 -0800 Comment on attachment 49604 patch for bug 35325 Clearing flags on attachment: 49604 Committed r55326: <http://trac.webkit.org/changeset/55326> 194372 7 commit-queue 2010-02-26 17:18:04 -0800 All reviewed patches have been landed. Closing bug. 49355 2010-02-23 20:30:14 -0800 2010-02-26 11:32:12 -0800 Patch to fix bug 35325 bug35325.patch text/plain 1576 rjkroege ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg NDQzZGY0Zi4uOTdkNDRlMCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMiBAQAorMjAxMC0wMi0yMyAgUm9iZXJ0IEtyb2Vn ZXIgIDxyamtyb2VnZUBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZ IChPT1BTISkuCisKKyAgICAgICAgVG8gZmlyZSBlYWNoIGV2ZW50IGhhbmRsZXIgcmVnaXN0ZXJl ZCBvbiBhbiBTVkcgbm9kZSBvbmNlIHBlcgorICAgICAgICBldmVudCwgQ2hyb21pdW0gbmVlZHMg YW4gaW1wbGVtZW50YXRpb24gb2Ygd2FzQ3JlYXRlZEZyb21NYXJrdXAKKyAgICAgICAgYWRkZWQg dG8gVjhMYXp5RXZlbnRMaXN0ZW5lci5oIHRoYXQgbWF0Y2hlcyB0aGUgb25lIGluCisgICAgICAg IFdlYktpdC9XZWJDb3JlL2JpbmRpbmdzL2pzL0pTTGF6eUV2ZW50TGlzdGVuZXIuaC4KKyAgICAg ICAgCisgICAgICAgIFRoaXMgcGF0Y2ggYWRkcyBzdWNoIGEgbWF0Y2hpbmcgaW1wbGVtZW50YXRp b24gb2Ygd2FzQ3JlYXRlZEZyb21NYXJrdXAKKyAgICAgICAgdG8gVjhMYXp5RXZlbnRMaXN0ZW5l ci5oLgorICAgICAgICAKKyAgICAgICAgUGF0Y2ggd2FzIHRlc3RlZCBhbmQgcGFzc2VkIHRoZSBD aHJvbWl1bSB0cnkgYm90cy4KKworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9MzUzMjUKKworICAgICAgICAqIGJpbmRpbmdzL3Y4L1Y4TGF6eUV2ZW50TGlz dGVuZXIuaDoKKyAgICAgICAgKFdlYkNvcmU6OlY4TGF6eUV2ZW50TGlzdGVuZXI6Ondhc0NyZWF0 ZWRGcm9tTWFya3VwKToKKwogMjAxMC0wMi0yMiAgSm9obiBTdWxsaXZhbiAgPHN1bGxpdmFuQGFw cGxlLmNvbT4KICAgICAgICAgCiAgICAgICAgIFJldmlld2VkIGJ5IFRpbSBIYXRjaGVyLgpkaWZm IC0tZ2l0IGEvV2ViQ29yZS9iaW5kaW5ncy92OC9WOExhenlFdmVudExpc3RlbmVyLmggYi9XZWJD b3JlL2JpbmRpbmdzL3Y4L1Y4TGF6eUV2ZW50TGlzdGVuZXIuaAppbmRleCAwNzhkY2YxLi5iY2Y1 MjZlIDEwMDY0NAotLS0gYS9XZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4TGF6eUV2ZW50TGlzdGVuZXIu aAorKysgYi9XZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4TGF6eUV2ZW50TGlzdGVuZXIuaApAQCAtNjAs NiArNjAsOSBAQCBuYW1lc3BhY2UgV2ViQ29yZSB7CiAKICAgICAgICAgdmlydHVhbCB2ODo6TG9j YWw8djg6OlZhbHVlPiBjYWxsTGlzdGVuZXJGdW5jdGlvbihTY3JpcHRFeGVjdXRpb25Db250ZXh0 Kiwgdjg6OkhhbmRsZTx2ODo6VmFsdWU+IGpzRXZlbnQsIEV2ZW50Kik7CiAKKyAgICAgICAgLy8g TmVlZGVkIHRvIG1hdGNoIEpTQyBzZW1hbnRpY3MgaW5zaWRlIHNoYXJlZCBTVkcgY29kZS4KKyAg ICAgICAgdmlydHVhbCBib29sIHdhc0NyZWF0ZWRGcm9tTWFya3VwKCkgY29uc3QgeyByZXR1cm4g dHJ1ZTsgfQorCiAgICAgICAgIFN0cmluZyBtX2Z1bmN0aW9uTmFtZTsKICAgICAgICAgYm9vbCBt X2lzU1ZHRXZlbnQ7CiAgICAgICAgIFN0cmluZyBtX2NvZGU7Cg== 49604 2010-02-26 11:32:12 -0800 2010-02-26 17:18:00 -0800 patch for bug 35325 bug_35325.patch text/plain 1705 rjkroege ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg NzU2ZDMxNi4uNTU2MDYzZCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMCBAQAorMjAxMC0wMi0yMyAgUm9iZXJ0IEtyb2Vn ZXIgIDxyamtyb2VnZUBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTmF0ZSBD aGFwaW4KKworICAgICAgICBUbyBmaXJlIGVhY2ggZXZlbnQgaGFuZGxlciByZWdpc3RlcmVkIG9u IGFuIFNWRyBub2RlIG9uY2UgcGVyCisgICAgICAgIGV2ZW50LCBDaHJvbWl1bSBuZWVkcyBhbiBp bXBsZW1lbnRhdGlvbiBvZiB3YXNDcmVhdGVkRnJvbU1hcmt1cAorICAgICAgICBhZGRlZCB0byBW OExhenlFdmVudExpc3RlbmVyLmggdGhhdCBtYXRjaGVzIHRoZSBvbmUgaW4KKyAgICAgICAgV2Vi S2l0L1dlYkNvcmUvYmluZGluZ3MvanMvSlNMYXp5RXZlbnRMaXN0ZW5lci5oLgorICAgICAgICAK KyAgICAgICAgVGhpcyBwYXRjaCBhZGRzIHN1Y2ggYSBtYXRjaGluZyBpbXBsZW1lbnRhdGlvbiBv ZiB3YXNDcmVhdGVkRnJvbU1hcmt1cAorICAgICAgICB0byBWOExhenlFdmVudExpc3RlbmVyLmgu CisgICAgICAgIAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MzUzMjUKKworICAgICAgICAqIGJpbmRpbmdzL3Y4L1Y4TGF6eUV2ZW50TGlzdGVuZXIuaDoK KyAgICAgICAgKFdlYkNvcmU6OlY4TGF6eUV2ZW50TGlzdGVuZXI6Ondhc0NyZWF0ZWRGcm9tTWFy a3VwKToKKwogMjAxMC0wMi0yNiAgQWRhbSBCYXJ0aCAgPGFiYXJ0aEB3ZWJraXQub3JnPgogCiAg ICAgICAgIFJldmlld2VkIGJ5IERhcmluIEFkbGVyLgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9iaW5k aW5ncy92OC9WOExhenlFdmVudExpc3RlbmVyLmggYi9XZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4TGF6 eUV2ZW50TGlzdGVuZXIuaAppbmRleCAwNzhkY2YxLi5mMTc0ZDIzIDEwMDY0NAotLS0gYS9XZWJD b3JlL2JpbmRpbmdzL3Y4L1Y4TGF6eUV2ZW50TGlzdGVuZXIuaAorKysgYi9XZWJDb3JlL2JpbmRp bmdzL3Y4L1Y4TGF6eUV2ZW50TGlzdGVuZXIuaApAQCAtNjAsNiArNjAsMTIgQEAgbmFtZXNwYWNl IFdlYkNvcmUgewogCiAgICAgICAgIHZpcnR1YWwgdjg6OkxvY2FsPHY4OjpWYWx1ZT4gY2FsbExp c3RlbmVyRnVuY3Rpb24oU2NyaXB0RXhlY3V0aW9uQ29udGV4dCosIHY4OjpIYW5kbGU8djg6OlZh bHVlPiBqc0V2ZW50LCBFdmVudCopOwogCisgICAgICAgIC8vIE5lZWRzIHRvIHJldHVybiB0cnVl IGZvciBhbGwgZXZlbnQgaGFuZGxlcnMgaW1wbGVtZW50ZWQgaW4gSmF2YVNjcmlwdCBzbyB0aGF0 CisgICAgICAgIC8vIHRoZSBTVkcgY29kZSBkb2VzIG5vdCBhZGQgdGhlIGV2ZW50IGhhbmRsZXIg aW4gYm90aAorICAgICAgICAvLyBTVkdVc2VFbGVtZW50OjpidWlsZFNoYWRvd1RyZWUgYW5kIGFn YWluIGluCisgICAgICAgIC8vIFNWR1VzZUVsZW1lbnQ6OnRyYW5zZmVyRXZlbnRMaXN0ZW5lcnNU b1NoYWRvd1RyZWUKKyAgICAgICAgdmlydHVhbCBib29sIHdhc0NyZWF0ZWRGcm9tTWFya3VwKCkg Y29uc3QgeyByZXR1cm4gdHJ1ZTsgfQorCiAgICAgICAgIFN0cmluZyBtX2Z1bmN0aW9uTmFtZTsK ICAgICAgICAgYm9vbCBtX2lzU1ZHRXZlbnQ7CiAgICAgICAgIFN0cmluZyBtX2NvZGU7Cg==