Bug 30318

Summary: ScriptExecutionContext is not anymore needed to create an EventListener - remove old code.
Product: WebKit Reporter: Dmitry Titov <dimich>
Component: WebCore JavaScriptAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Proposed fix.
ap: review+
Updated, with test ap: review+

Dmitry Titov
Reported 2009-10-12 17:56:16 PDT
Change http://trac.webkit.org/changeset/48884 removed the need to have ScriptExecutionContext to create an EventListener - rather it is pulled from EventTarget at the moment of event handling. It seems we can remove the code in custom add/removeEventListener methods that retrieves the context which is no longer used. The potential change in behavior here is that in case the EventTarget has null context it wasn't adding a listener and now it would, which could later be invoked if the EventTarget will gain context, so it could change the behavior of some page, theoretically. But I don't think this scenario ever realizes. For example, it's impossible to create a Node w/o a pointer to Document and while a Node may change documents during its life, there is no code that can set the Node::m_document to null. I see the original change did not remove this code, is there a reason to keep it?
Attachments
Proposed fix. (12.50 KB, patch)
2009-10-12 18:00 PDT, Dmitry Titov
ap: review+
Updated, with test (17.97 KB, patch)
2009-10-13 17:29 PDT, Dmitry Titov
ap: review+
Dmitry Titov
Comment 1 2009-10-12 18:00:11 PDT
Created attachment 41076 [details] Proposed fix.
Alexey Proskuryakov
Comment 2 2009-10-12 19:15:16 PDT
Comment on attachment 41076 [details] Proposed fix. > I see the original change did not remove this code, is there a reason to keep > it? I just wanted to keep this potential behavior change separate form other changes in that patch. Thanks for following up on this! > it's impossible to create a Node w/o a pointer to Document That's not entirely accurate; see DOMImplementation::createDocumentType() for the one and only case when this happens. Maybe you could test for that before landing (and compare to Firefox), but it's unlikely that there is any harm in such a change. r=me
Dmitry Titov
Comment 3 2009-10-13 17:29:09 PDT
Created attachment 41141 [details] Updated, with test Verified that FF also allows to set an event listener on documentType - and will fire events on it once it is inserted into a live document tree. Added test that verifies that.
Alexey Proskuryakov
Comment 4 2009-10-13 17:31:23 PDT
Comment on attachment 41141 [details] Updated, with test r=me
Dmitry Titov
Comment 5 2009-10-13 18:17:06 PDT
Note You need to log in before you can comment on or make changes to this bug.