Summary: | [V8] Refactored V8 event listeners. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vitaly Repeshko <vitalyr> | ||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, dglazkov, eric, ojan, sam | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Vitaly Repeshko
2009-09-28 14:09:26 PDT
Created attachment 40260 [details] patch: WIP Dimitri, please have a look. This is still WIP (testing lazy listeners) and depends on http://code.google.com/p/v8/source/detail?r=2980 (should be in V8 trunk this week), but I think it's almost done. drive-by comment: Is there any way to test for leaks so this doesn't regress? If this is still a WIP, this should not be marked for review. One question though, why are the attribute event listeners marked as OnProto? This does not match the JSC bindings. Are there tests that indicate it should be this way? (In reply to comment #2) > drive-by comment: Is there any way to test for leaks so this doesn't regress? Yes, for fixed leaks I'll extend src/webkit/tools/test_shell/listener_leak_test.cc. (In reply to comment #3) > If this is still a WIP, this should not be marked for review. One question > though, why are the attribute event listeners marked as OnProto? This does not > match the JSC bindings. Are there tests that indicate it should be this way? They are marked with v8onproto attribute because old custom V8 code used to look up the holder of the property in prototype chain. I tried to copy the old behavior. I'll double-check if this is in fact needed. If JSC doesn't need this, chances are V8 doesn't need either. Comment on attachment 40260 [details]
patch: WIP
+1 on investigating v8onproto.
Created attachment 40360 [details]
updated patch
Notification does not require v8onproto. Removed.
Remaining memory leaks in lazy listeners are unrelated to handles and are caused by Function cache for functions created by FunctionTemplate. I'm going to fix usages of FunctionTemplate in V8 bindings in a separate patch.
Comment on attachment 40360 [details] updated patch This is super-awesome. I only have style nits. I would really like to use enums for isAttribute params in getEventListener. That should clarify the call sites. > + V8Proxy* proxy = V8Proxy::retrieve(worker->scriptExecutionContext()); > + if (proxy) { > + return findOnly ? V8EventListenerList::findWrapper(value, isAttribute) : V8EventListenerList::findOrCreateWrapper<V8EventListener>(proxy->frame(), value, isAttribute); > + } Don't need braces here. > + if (proxy) { > + return findOnly ? V8EventListenerList::findWrapper(value, isAttribute) : V8EventListenerList::findOrCreateWrapper<V8EventListener>(proxy->frame(), value, isAttribute); > + } Ditto. > +v8::Handle<v8::String> V8HiddenPropertyName::listener() > +{ > + static v8::Persistent<v8::String>* string = createString("WebCore::V8HiddenPropertyName::listener"); > + no line break here. > + return *string; > +} > + > +v8::Handle<v8::String> V8HiddenPropertyName::attributeListener() > +{ > + static v8::Persistent<v8::String>* string = createString("WebCore::V8HiddenPropertyName::attributeListener"); Ditto. > +void V8Proxy::disconnectEventListeners() > +{ > +} > + Why is this here? Can we just get rid of it? Created attachment 40386 [details]
updated patch v2
Comment on attachment 40386 [details]
updated patch v2
r=me.
Comment on attachment 40386 [details] updated patch v2 Clearing flags on attachment: 40386 Committed r48978: <http://trac.webkit.org/changeset/48978> All reviewed patches have been landed. Closing bug. See http://code.google.com/p/chromium/issues/detail?id=23597 I guess this caused some sort of regression. I'm happy to roll this out if that's what's desired. bugzilla-tool rollout 48978 will prepare the rollout. bugzilla-tool rollout 48978 --complete-rollout --force-clean would do the full rollout including testing to make sure it builds/passes tests and updating the bug. Let's give vitalyr a chance to fix this. Sadness that it broke. (In reply to comment #14) > Let's give vitalyr a chance to fix this. Sadness that it broke. Reverting this patch doesn't fix the breakage. Please see discussion in http://code.google.com/p/chromium/issues/detail?id=23597. |