80795 2012-03-11 13:15:19 -0700 Remove vestigal abortEvent from image attribute. 2012-03-13 14:59:28 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 gavinp gavinp abarth ap darin webkit.review.bot oldest_to_newest 575826 0 gavinp 2012-03-11 13:15:19 -0700 Remove vestigal abortEvent from image attribute. 575827 1 131256 gavinp 2012-03-11 13:22:56 -0700 Created attachment 131256 Patch 575828 2 131256 abarth 2012-03-11 14:20:25 -0700 Comment on attachment 131256 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131256&action=review > Source/WebCore/ChangeLog:11 > + It seems we installed a listener for the abort event on images, > + but never ever raised them. So this patch removes it. This could > + cause different behaviour if the user sends abort events directly > + at an image element, although addEventListener will still work. Do we need to remove any onabort attributes from the HTMLImageElement.idl ? Do we understand why this code was added to WebKit? Do other browsers let you install "abort" handlers using the onabort attribute (e.g., to receive custom abort events)? 575842 3 gavinp 2012-03-11 15:26:22 -0700 (In reply to comment #2) > (From update of attachment 131256 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131256&action=review > > > Source/WebCore/ChangeLog:11 > > + It seems we installed a listener for the abort event on images, > > + but never ever raised them. So this patch removes it. This could > > + cause different behaviour if the user sends abort events directly > > + at an image element, although addEventListener will still work. > > Do we need to remove any onabort attributes from the HTMLImageElement.idl ? No. The onabort event handler comes from HTMLElement.idl; lots of other events don't set up an abortEvent listener without problem. See HTMLScriptElement. > Do we understand why this code was added to WebKit? No. The line was last editted in 2009 by eseidel when reorganising the source tree, and carried this over. I believe it to be completely vestigal, but I haven't rooted back farther than that. > Do other browsers let you install "abort" handlers using the onabort attribute (e.g., to receive custom abort events)? I believe my comment above was premature above. I'm having a lot of trouble even constructing an event exception, and throwing it at an element. So now I'm not so sure I've changed behaviour. 575854 4 abarth 2012-03-11 17:12:41 -0700 > > Do we understand why this code was added to WebKit? > > No. The line was last editted in 2009 by eseidel when reorganising the source tree, and carried this over. I believe it to be completely vestigal, but I haven't rooted back farther than that. It would probably be worth digging back further in the history to understand why it was added in the first place. > > Do other browsers let you install "abort" handlers using the onabort attribute (e.g., to receive custom abort events)? > > I believe my comment above was premature above. I'm having a lot of trouble even constructing an event exception, and throwing it at an element. So now I'm not so sure I've changed behaviour. You should be able to test this by creating an event, initializing with an eventType of "abort" and calling dispatchEvent at the appropriate element. 577628 5 gavinp 2012-03-13 11:59:33 -0700 Indeed, I was able to create and dispatch events in Firefox and Chrome for comparison. Firefox seems to install handlers on all elements. I successfully sent an abort event to a <script> in Firefox, and it ran the onabort code. Then I got silly, and tried it with a <b>, and that worked too. The onabort attr was installed as an handler. We only install onabort handlers from the attribute in HTMLImageElement, HTMLMediaElement and SVGSVGElement. In Media & SVG elements, we actually raise abort events sometimes. In Image, we do not. So either way, we're being inconsistant. Either we should install them on everything ala Firefox, or only install them where we fire the event ourselves. I'm not sure which is better. 577798 6 gavinp 2012-03-13 14:09:27 -0700 As for the history, this handler goes back to the initial revision. Behold WebCore/khtml/html/html_imageimpl.cpp as of 2001-08-24. 166 case ATTR_ONABORT: // ### add support for this 167 setHTMLEventListener(EventImpl::ABORT_EVENT, 168 ownerDocument()->createHTMLEventListener(attr->value().string())); 169 break; 170 case ATTR_ONERROR: // ### add support for this 171 setHTMLEventListener(EventImpl::ERROR_EVENT, 172 ownerDocument()->createHTMLEventListener(attr->value().string())); 173 break; 174 case ATTR_ONLOAD: 175 setHTMLEventListener(EventImpl::LOAD_EVENT, 176 ownerDocument()->createHTMLEventListener(attr->value().string())); 177 break; In revision @2855 (2002-11-24), onerror was implemented, and that comment was removed, as that changeset sent out the error event from the resource on notifyFinished(). In 9824 in 2005-07-18, a refactoring changed events from being an enumeration to having an attribute class, and the comment was removed: @@ -227,51 +213,40 @@ } m_isLink = !attr->isNull(); - case ATTR_ISMAP: + } else if (attr->name() == HTMLAttributes::ismap()) { ismap = true; - break; - case ATTR_ONABORT: // ### add support for this + } else if (attr->name() == HTMLAttributes::onabort()) { setHTMLEventListener(EventImpl::ABORT_EVENT, - getDocument()->createHTMLEventListener(attr->value().string(), this)); - break; - case ATTR_ONERROR: + getDocument()->createHTMLEventListener(attr->value().string(), this)); + } else if (attr->name() == HTMLAttributes::onerror()) { setHTMLEventListener(EventImpl::ERROR_EVENT, - getDocument()->createHTMLEventListener(attr->value().string(), this)); - break; 9824 didn't add any causes of abort, and it was a pretty big change (patch is ~35,000 lines). 577805 7 131256 abarth 2012-03-13 14:15:38 -0700 Comment on attachment 131256 Patch Sounds like this is just vestigial. In principle, we could add a test showing that it's not there, but I don't should add that test so we don't bias future developers against adding the event (if they want to use it for something). 577844 8 131256 webkit.review.bot 2012-03-13 14:59:23 -0700 Comment on attachment 131256 Patch Clearing flags on attachment: 131256 Committed r110618: <http://trac.webkit.org/changeset/110618> 577845 9 webkit.review.bot 2012-03-13 14:59:28 -0700 All reviewed patches have been landed. Closing bug. 131256 2012-03-11 13:22:56 -0700 2012-03-13 14:59:23 -0700 Patch bug-80795-20120311162255.patch text/plain 1793 gavinp U3VidmVyc2lvbiBSZXZpc2lvbjogMTEwMzc4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNWVmNGM1ZTZmYTM2ZGE1 ZGQ0NGQwZTEyNjAwNzk1N2Y4YWJiNzgzMC4uZDE2NmI4ZDk5MTJkMzQ1OWYzMDJkNDlhMGRjOWVm NDJiYjE4MTdlNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDEyLTAzLTExICBHYXZp biBQZXRlcnMgIDxnYXZpbnBAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJlbW92ZSB2ZXN0aWdh bCBhYm9ydEV2ZW50IGZyb20gaW1hZ2UgYXR0cmlidXRlLgorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODA3OTUKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBJdCBzZWVtcyB3ZSBpbnN0YWxsZWQgYSBsaXN0ZW5l ciBmb3IgdGhlIGFib3J0IGV2ZW50IG9uIGltYWdlcywKKyAgICAgICAgYnV0IG5ldmVyIGV2ZXIg cmFpc2VkIHRoZW0uICBTbyB0aGlzIHBhdGNoIHJlbW92ZXMgaXQuICBUaGlzIGNvdWxkCisgICAg ICAgIGNhdXNlIGRpZmZlcmVudCBiZWhhdmlvdXIgaWYgdGhlIHVzZXIgc2VuZHMgYWJvcnQgZXZl bnRzIGRpcmVjdGx5CisgICAgICAgIGF0IGFuIGltYWdlIGVsZW1lbnQsIGFsdGhvdWdoIGFkZEV2 ZW50TGlzdGVuZXIgd2lsbCBzdGlsbCB3b3JrLgorCisgICAgICAgICogaHRtbC9IVE1MSW1hZ2VF bGVtZW50LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkhUTUxJbWFnZUVsZW1lbnQ6OnBhcnNlQXR0 cmlidXRlKToKKwogMjAxMi0wMy0xMCAgU3RlcGhlbiBXaGl0ZSAgPHNlbm9yYmxhbmNvQGNocm9t aXVtLm9yZz4KIAogICAgICAgICBVbnJldmlld2VkLCByb2xsaW5nIG91dCByMTEwMzU4LgpkaWZm IC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MSW1hZ2VFbGVtZW50LmNwcCBiL1NvdXJj ZS9XZWJDb3JlL2h0bWwvSFRNTEltYWdlRWxlbWVudC5jcHAKaW5kZXggZmY4OWM2ZTRmOWQxOGYy MGNmOWIwY2JiYWU3NDM3NzFiZjExMzEzYi4uYmUxMjJjY2Y1ZDRjNjA0NDBjZTNiYjdmMjYxNmEw NzFhYTI1OTI1YyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MSW1hZ2VFbGVt ZW50LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxJbWFnZUVsZW1lbnQuY3BwCkBA IC0xMTcsOCArMTE3LDYgQEAgdm9pZCBIVE1MSW1hZ2VFbGVtZW50OjpwYXJzZUF0dHJpYnV0ZShB dHRyaWJ1dGUqIGF0dHIpCiAgICAgICAgIG1faW1hZ2VMb2FkZXIudXBkYXRlRnJvbUVsZW1lbnRJ Z25vcmluZ1ByZXZpb3VzRXJyb3IoKTsKICAgICBlbHNlIGlmIChhdHRyTmFtZSA9PSB1c2VtYXBB dHRyKQogICAgICAgICBzZXRJc0xpbmsoIWF0dHItPmlzTnVsbCgpKTsKLSAgICBlbHNlIGlmIChh dHRyTmFtZSA9PSBvbmFib3J0QXR0cikKLSAgICAgICAgc2V0QXR0cmlidXRlRXZlbnRMaXN0ZW5l cihldmVudE5hbWVzKCkuYWJvcnRFdmVudCwgY3JlYXRlQXR0cmlidXRlRXZlbnRMaXN0ZW5lcih0 aGlzLCBhdHRyKSk7CiAgICAgZWxzZSBpZiAoYXR0ck5hbWUgPT0gb25sb2FkQXR0cikKICAgICAg ICAgc2V0QXR0cmlidXRlRXZlbnRMaXN0ZW5lcihldmVudE5hbWVzKCkubG9hZEV2ZW50LCBjcmVh dGVBdHRyaWJ1dGVFdmVudExpc3RlbmVyKHRoaXMsIGF0dHIpKTsKICAgICBlbHNlIGlmIChhdHRy TmFtZSA9PSBvbmJlZm9yZWxvYWRBdHRyKQo=