[chromium] MediaStream API: Modify WebMediaStream::ExtraData to have a owner()
Created attachment 193125 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Darin and Adam, How do you feel regarding this modification of the ExtraData pattern? It would solve a circular reference problem that the chromium port is having; where the ExtraData descendant gets a callback from e.g. the network stack and needs to modify its related object. If it keeps a copy then the objects can never be cleaned away. If it's OK I'll modify a few more classes to have the same functionality.
I'm not sure I understand the problem this patch is solving. Can you give me a more concrete example or point me towards the chromium code that's having the trouble?
Comment on attachment 193125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193125&action=review > Source/Platform/chromium/public/WebMediaStream.h:57 > + WebCore::MediaStreamDescriptor* m_owner; This worries me because there's no connection between the lifetime of the m_owner and the ExtraData instance. The ExtraDataContainer is RefCounted, which means it could outlive the m_owner.
Comment on attachment 193125 [details] Patch Yeah, I need to understand better why this is necessary. I'd imagine there may be other solutions, but without more context, it is hard to suggest any. Maybe a link to the Chromium CL that might use this API change would help.
+------------+ |WebCoreObject| +------------+ | | * +------------+ |WebKitWrapper| +------------+ * | | +-------+ |ExtraData| +-------+ ^ | ==================================== WebKit/chromium boundary | +----------+ |SomeListener| +----------+ The main issue (for me that is) is that chromiums WebKit platform pattern is tailored for data objects, not "intelligent" objects. Here's a breakdown: In the above picture (understandable I hope) --> means inheritance and *-- means ownership. An event happens and the SomeLister is notified. As an effect of this event the state of the WebCoreObject has to be notified. Here our problems starts because there's no way we can do that because SomeListener can't have the WebKitWrapper as a member. If it were do keep one it would create a circular memory reference and teardown of the world can't take place properly. What would work is if SomeListener inherits from ExtraData (because there are some data to be stored as well) AND has a way to get its WebKitWrapper when necessary which is what this patch is all about. For the MediaStream major classes we have another pattern that works really well (Handler/HandlerClient) but it would be a major overhead if all objects need to switch to this pattern. The ideal case would be if all major classes use the Handler/HandlerClient pattern and the minor classes where just a state needs to be changed can use the proposed pattern.
Comment on attachment 193125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193125&action=review >> Source/Platform/chromium/public/WebMediaStream.h:57 >> + WebCore::MediaStreamDescriptor* m_owner; > > This worries me because there's no connection between the lifetime of the m_owner and the ExtraData instance. The ExtraDataContainer is RefCounted, which means it could outlive the m_owner. yes, that right but the ExtraDataContainer is only visible in the .cpp file and under complete control which should leave no loopholes. Unless someone later modifies the code and exposes the ExtraDataContainer of course...
Ping...