112350 2013-03-14 08:12:00 -0700 [chromium] MediaStream API: Modify WebMediaStream::ExtraData to have a owner() 2013-04-08 16:50:50 -0700 1 1 1 Unclassified WebKit WebKit API 528+ (Nightly build) All All RESOLVED WONTFIX P2 Normal --- 56459 1 tommyw tommyw abarth dglazkov eric.carlson feature-media-reviews fishd jamesr jer.noble jochen schenney tkent+wkapi webkit.review.bot oldest_to_newest 855200 0 tommyw 2013-03-14 08:12:00 -0700 [chromium] MediaStream API: Modify WebMediaStream::ExtraData to have a owner() 855201 1 193125 tommyw 2013-03-14 08:12:45 -0700 Created attachment 193125 Patch 855202 2 webkit.review.bot 2013-03-14 08:16:05 -0700 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. 855205 3 tommyw 2013-03-14 08:20:11 -0700 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. 856261 4 abarth 2013-03-15 13:51:05 -0700 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? 856263 5 193125 abarth 2013-03-15 13:54:02 -0700 Comment on attachment 193125 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. 856490 6 193125 fishd 2013-03-15 20:30:07 -0700 Comment on attachment 193125 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. 856948 7 tommyw 2013-03-18 03:25:36 -0700 +------------+ |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. 856949 8 193125 tommyw 2013-03-18 03:29:27 -0700 Comment on attachment 193125 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... 860358 9 tommyw 2013-03-21 08:17:36 -0700 Ping... 193125 2013-03-14 08:12:45 -0700 2013-04-06 17:30:56 -0700 Patch bug-112350-20130314160841.patch text/plain 5560 tommyw U3VidmVyc2lvbiBSZXZpc2lvbjogMTQ1ODA2CmRpZmYgLS1naXQgYS9Tb3VyY2UvUGxhdGZvcm0v Q2hhbmdlTG9nIGIvU291cmNlL1BsYXRmb3JtL0NoYW5nZUxvZwppbmRleCA0MmZiMTYxMTMzNjg0 YTdkM2NkYTdkZTMxMTc3OTliNTJiMGQ2ZWRmLi44MWM1NWNjODcxMTYzOWU4YTYxY2I2NjVlYjQ0 ZmEzNWM0MWY2Njk4IDEwMDY0NAotLS0gYS9Tb3VyY2UvUGxhdGZvcm0vQ2hhbmdlTG9nCisrKyBi L1NvdXJjZS9QbGF0Zm9ybS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNSBAQAorMjAxMy0wMy0xNCAg VG9tbXkgV2lkZW5mbHljaHQgIDx0b21teXdAZ29vZ2xlLmNvbT4KKworICAgICAgICBbY2hyb21p dW1dIE1lZGlhU3RyZWFtIEFQSTogTW9kaWZ5IFdlYk1lZGlhU3RyZWFtOjpFeHRyYURhdGEgdG8g aGF2ZSBhIG93bmVyKCkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTExMjM1MAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgICogY2hyb21pdW0vcHVibGljL1dlYk1lZGlhU3RyZWFtLmg6CisgICAgICAgIChXZWJL aXQ6OldlYk1lZGlhU3RyZWFtOjpFeHRyYURhdGE6OkV4dHJhRGF0YSk6CisgICAgICAgIChFeHRy YURhdGEpOgorICAgICAgICAoV2ViTWVkaWFTdHJlYW0pOgorCiAyMDEzLTAzLTEzICBUb21teSBX aWRlbmZseWNodCAgPHRvbW15d0Bnb29nbGUuY29tPgogCiAgICAgICAgIFtjaHJvbWl1bV0gTWVk aWFTdHJlYW0gQVBJOiBDbGVhbmluZyBhd2F5IGRlcHJlY2F0ZWQgYW5kIHVudXNlZCBzdGF0ZXMK ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCBhNTA3NDI5ZjM5YjRkMzY0M2Y3OTJmNTA4NjcyZmM4ZWNiMGZhMjZjLi40 NjU0YTU0MTk1YjUwMDhhNDgxNjIyMmQwZDJjNzRlY2E2ZDNjODRjIDEwMDY0NAotLS0gYS9Tb3Vy Y2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0x LDMgKzEsMTcgQEAKKzIwMTMtMDMtMTQgIFRvbW15IFdpZGVuZmx5Y2h0ICA8dG9tbXl3QGdvb2ds ZS5jb20+CisKKyAgICAgICAgW2Nocm9taXVtXSBNZWRpYVN0cmVhbSBBUEk6IE1vZGlmeSBXZWJN ZWRpYVN0cmVhbTo6RXh0cmFEYXRhIHRvIGhhdmUgYSBvd25lcigpCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMTIzNTAKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHBsYXRmb3JtL2Nocm9taXVtL3N1cHBv cnQvV2ViTWVkaWFTdHJlYW0uY3BwOgorICAgICAgICAoV2ViS2l0OjpXZWJNZWRpYVN0cmVhbTo6 RXh0cmFEYXRhOjpvd25lcik6CisgICAgICAgIChXZWJLaXQpOgorICAgICAgICAoV2ViS2l0OjpX ZWJNZWRpYVN0cmVhbTo6RXh0cmFEYXRhOjpzZXRPd25lcik6CisgICAgICAgIChXZWJLaXQ6Oldl Yk1lZGlhU3RyZWFtOjpzZXRFeHRyYURhdGEpOgorICAgICAgICAoV2ViS2l0OjpXZWJNZWRpYVN0 cmVhbTo6b3BlcmF0b3I9PSk6CisKIDIwMTMtMDMtMTQgIE5vJ2FtIFJvc2VudGhhbCAgPG5vYW1A d2Via2l0Lm9yZz4KIAogICAgICAgICBbVGV4bWFwXSBTeW5jaHJvbml6ZSBsYXllcnMgb25seSBp ZiB0aGUgbGF5ZXIgaGFzIGJlZW4gY2hhbmdlZC4KZGlmZiAtLWdpdCBhL1NvdXJjZS9QbGF0Zm9y bS9jaHJvbWl1bS9wdWJsaWMvV2ViTWVkaWFTdHJlYW0uaCBiL1NvdXJjZS9QbGF0Zm9ybS9jaHJv bWl1bS9wdWJsaWMvV2ViTWVkaWFTdHJlYW0uaAppbmRleCBlY2Q1ODZkNjQ3MGQxMGMxOWE4YWNi MjBkZmNhYzA3N2EwNDI3NTQ0Li5kY2YyYzMxNzIzYWQyZjdmNzRhZmNkZTBhMTBmNjk0YjkxZmM5 OWE5IDEwMDY0NAotLS0gYS9Tb3VyY2UvUGxhdGZvcm0vY2hyb21pdW0vcHVibGljL1dlYk1lZGlh U3RyZWFtLmgKKysrIGIvU291cmNlL1BsYXRmb3JtL2Nocm9taXVtL3B1YmxpYy9XZWJNZWRpYVN0 cmVhbS5oCkBAIC00NCw3ICs0NCwxNyBAQCBjbGFzcyBXZWJNZWRpYVN0cmVhbSB7CiBwdWJsaWM6 CiAgICAgY2xhc3MgRXh0cmFEYXRhIHsKICAgICBwdWJsaWM6CisgICAgICAgIEV4dHJhRGF0YSgp IDogbV9vd25lcigwKSB7IH0KICAgICAgICAgdmlydHVhbCB+RXh0cmFEYXRhKCkgeyB9CisKKyAg ICAgICAgV0VCS0lUX0VYUE9SVCBXZWJNZWRpYVN0cmVhbSBvd25lcigpOworCisjaWYgV0VCS0lU X0lNUExFTUVOVEFUSU9OCisgICAgICAgIHZvaWQgc2V0T3duZXIoV2ViQ29yZTo6TWVkaWFTdHJl YW1EZXNjcmlwdG9yKik7CisjZW5kaWYKKworICAgIHByaXZhdGU6CisgICAgICAgIFdlYkNvcmU6 Ok1lZGlhU3RyZWFtRGVzY3JpcHRvciogbV9vd25lcjsKICAgICB9OwogCiAgICAgV2ViTWVkaWFT dHJlYW0oKSB7IH0KQEAgLTU3LDYgKzY3LDggQEAgcHVibGljOgogICAgICAgICByZXR1cm4gKnRo aXM7CiAgICAgfQogCisgICAgV0VCS0lUX0VYUE9SVCBib29sIG9wZXJhdG9yPT0oY29uc3QgV2Vi TWVkaWFTdHJlYW0mIG90aGVyKTsKKwogICAgIFdFQktJVF9FWFBPUlQgdm9pZCBhc3NpZ24oY29u c3QgV2ViTWVkaWFTdHJlYW0mKTsKIAogICAgIC8vIERFUFJFQ0FURUQKZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJDb3JlL3BsYXRmb3JtL2Nocm9taXVtL3N1cHBvcnQvV2ViTWVkaWFTdHJlYW0uY3Bw IGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vY2hyb21pdW0vc3VwcG9ydC9XZWJNZWRpYVN0cmVh bS5jcHAKaW5kZXggMTdhMDJlMjQyZWMyYjZiNzAwZTMxYTkyZGY1M2NkYjQyNDg3YjA4NS4uOGJm ZGRmYjJhOWM4MDcyZDI2MzVlZWMyYzlmYjU2OTEyZmYxNzAxZCAxMDA2NDQKLS0tIGEvU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vY2hyb21pdW0vc3VwcG9ydC9XZWJNZWRpYVN0cmVhbS5jcHAKKysr IGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vY2hyb21pdW0vc3VwcG9ydC9XZWJNZWRpYVN0cmVh bS5jcHAKQEAgLTU3LDYgKzU3LDE4IEBAIHByaXZhdGU6CiAKIH0gLy8gbmFtZXNwYWNlCiAKK1dl Yk1lZGlhU3RyZWFtIFdlYk1lZGlhU3RyZWFtOjpFeHRyYURhdGE6Om93bmVyKCkKK3sKKyAgICBB U1NFUlQobV9vd25lcik7CisgICAgcmV0dXJuIFdlYk1lZGlhU3RyZWFtKG1fb3duZXIpOworfQor Cit2b2lkIFdlYk1lZGlhU3RyZWFtOjpFeHRyYURhdGE6OnNldE93bmVyKFdlYkNvcmU6Ok1lZGlh U3RyZWFtRGVzY3JpcHRvciogb3duZXIpCit7CisgICAgQVNTRVJUKCFtX293bmVyKTsKKyAgICBt X293bmVyID0gb3duZXI7Cit9CisKIFdlYk1lZGlhU3RyZWFtOjpXZWJNZWRpYVN0cmVhbShjb25z dCBQYXNzUmVmUHRyPFdlYkNvcmU6Ok1lZGlhU3RyZWFtRGVzY3JpcHRvcj4mIG1lZGlhU3RyZWFt RGVzY3JpcHRvcikKICAgICA6IG1fcHJpdmF0ZShtZWRpYVN0cmVhbURlc2NyaXB0b3IpCiB7CkBA IC05Miw2ICsxMDQsOCBAQCBXZWJNZWRpYVN0cmVhbTo6RXh0cmFEYXRhKiBXZWJNZWRpYVN0cmVh bTo6ZXh0cmFEYXRhKCkgY29uc3QKIAogdm9pZCBXZWJNZWRpYVN0cmVhbTo6c2V0RXh0cmFEYXRh KEV4dHJhRGF0YSogZXh0cmFEYXRhKQogeworICAgIGlmIChleHRyYURhdGEpCisgICAgICAgIGV4 dHJhRGF0YS0+c2V0T3duZXIobV9wcml2YXRlLmdldCgpKTsKICAgICBtX3ByaXZhdGUtPnNldEV4 dHJhRGF0YShhZG9wdFJlZihuZXcgRXh0cmFEYXRhQ29udGFpbmVyKGV4dHJhRGF0YSkpKTsKIH0K IApAQCAtMTE5LDYgKzEzMywxMSBAQCBXZWJNZWRpYVN0cmVhbSYgV2ViTWVkaWFTdHJlYW06Om9w ZXJhdG9yPShjb25zdCBQYXNzUmVmUHRyPFdlYkNvcmU6Ok1lZGlhU3RyZWFtRAogICAgIHJldHVy biAqdGhpczsKIH0KIAorYm9vbCBXZWJNZWRpYVN0cmVhbTo6b3BlcmF0b3I9PShjb25zdCBXZWJN ZWRpYVN0cmVhbSYgb3RoZXIpCit7CisgICAgcmV0dXJuIG1fcHJpdmF0ZS5nZXQoKSA9PSBvdGhl ci5tX3ByaXZhdGUuZ2V0KCk7Cit9CisKIFdlYk1lZGlhU3RyZWFtOjpvcGVyYXRvciBQYXNzUmVm UHRyPFdlYkNvcmU6Ok1lZGlhU3RyZWFtRGVzY3JpcHRvcj4oKSBjb25zdAogewogICAgIHJldHVy biBtX3ByaXZhdGUuZ2V0KCk7CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cgYi9Ub29scy9D aGFuZ2VMb2cKaW5kZXggMjVkNGZlMmVhNmIxNjFiMjNmY2I3ZDNhNTRkYTUzNTViNTI1ZTk3MC4u MWM1NTMwMzgzMDliNTlhMjRhOWE3NTM3MjY3ZmNlODFiMDEwMjQ5ZSAxMDA2NDQKLS0tIGEvVG9v bHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEzIEBACisyMDEz LTAzLTE0ICBUb21teSBXaWRlbmZseWNodCAgPHRvbW15d0Bnb29nbGUuY29tPgorCisgICAgICAg IFtjaHJvbWl1bV0gTWVkaWFTdHJlYW0gQVBJOiBNb2RpZnkgV2ViTWVkaWFTdHJlYW06OkV4dHJh RGF0YSB0byBoYXZlIGEgb3duZXIoKQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9MTEyMzUwCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgKiBEdW1wUmVuZGVyVHJlZS9jaHJvbWl1bS9UZXN0UnVubmVyL3NyYy9X ZWJVc2VyTWVkaWFDbGllbnRNb2NrLmNwcDoKKyAgICAgICAgKFdlYlRlc3RSdW5uZXI6OldlYlVz ZXJNZWRpYUNsaWVudE1vY2s6OnJlcXVlc3RVc2VyTWVkaWEpOgorCiAyMDEzLTAzLTE0ICBTaW1v biBIYXVzbWFubiAgPHNpbW9uLmhhdXNtYW5uQGRpZ2lhLmNvbT4KIAogICAgICAgICBbUXRdIElt cHJvdmUgdGhlIGhhbmRsaW5nIG9mIG1vY2sgZ2VvbG9jYXRpb24sIGRldmljZSBvcmllbnRhdGlv biBhbmQgbW90aW9uIGNsaWVudHMKZGlmZiAtLWdpdCBhL1Rvb2xzL0R1bXBSZW5kZXJUcmVlL2No cm9taXVtL1Rlc3RSdW5uZXIvc3JjL1dlYlVzZXJNZWRpYUNsaWVudE1vY2suY3BwIGIvVG9vbHMv RHVtcFJlbmRlclRyZWUvY2hyb21pdW0vVGVzdFJ1bm5lci9zcmMvV2ViVXNlck1lZGlhQ2xpZW50 TW9jay5jcHAKaW5kZXggMTgxM2UxOGE1Y2VhMGFkYWQ5ZGE2NWVlMDZhNGU5N2EzM2QzZmFkNS4u M2JiYjdhZDgxM2YwOGM1MzFhYTU3NTRjMjU3MzFkMjQ3NGQ4ZTMyMyAxMDA2NDQKLS0tIGEvVG9v bHMvRHVtcFJlbmRlclRyZWUvY2hyb21pdW0vVGVzdFJ1bm5lci9zcmMvV2ViVXNlck1lZGlhQ2xp ZW50TW9jay5jcHAKKysrIGIvVG9vbHMvRHVtcFJlbmRlclRyZWUvY2hyb21pdW0vVGVzdFJ1bm5l ci9zcmMvV2ViVXNlck1lZGlhQ2xpZW50TW9jay5jcHAKQEAgLTEyNCw2ICsxMjQsNyBAQCB2b2lk IFdlYlVzZXJNZWRpYUNsaWVudE1vY2s6OnJlcXVlc3RVc2VyTWVkaWEoY29uc3QgV2ViVXNlck1l ZGlhUmVxdWVzdCYgc3RyZWFtUgogICAgIHN0cmVhbS5pbml0aWFsaXplKGF1ZGlvVHJhY2tzLCB2 aWRlb1RyYWNrcyk7CiAKICAgICBzdHJlYW0uc2V0RXh0cmFEYXRhKG5ldyBNb2NrRXh0cmFEYXRh KCkpOworICAgIFdFQktJVF9BU1NFUlQoc3RyZWFtLmV4dHJhRGF0YSgpLT5vd25lcigpID09IHN0 cmVhbSk7CiAKICAgICBtX2RlbGVnYXRlLT5wb3N0VGFzayhuZXcgVXNlck1lZGlhUmVxdWVzdFRh c2sodGhpcywgcmVxdWVzdCwgc3RyZWFtKSk7CiB9Cg==