Bug 30318 - ScriptExecutionContext is not anymore needed to create an EventListener - remove old code.
Summary: ScriptExecutionContext is not anymore needed to create an EventListener - rem...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-12 17:56 PDT by Dmitry Titov
Modified: 2009-10-13 18:17 PDT (History)
1 user (show)

See Also:


Attachments
Proposed fix. (12.50 KB, patch)
2009-10-12 18:00 PDT, Dmitry Titov
ap: review+
Details | Formatted Diff | Diff
Updated, with test (17.97 KB, patch)
2009-10-13 17:29 PDT, Dmitry Titov
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Titov 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?
Comment 1 Dmitry Titov 2009-10-12 18:00:11 PDT
Created attachment 41076 [details]
Proposed fix.
Comment 2 Alexey Proskuryakov 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
Comment 3 Dmitry Titov 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.
Comment 4 Alexey Proskuryakov 2009-10-13 17:31:23 PDT
Comment on attachment 41141 [details]
Updated, with test

r=me
Comment 5 Dmitry Titov 2009-10-13 18:17:06 PDT
Landed: http://trac.webkit.org/changeset/49539