43848 2010-08-11 06:34:45 -0700 Need EmptyDeviceOrientationClient and EmptyDeviceMotionClient for use with SVGImage 2010-08-12 07:38:16 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 30335 1 steveblock webkit-unassigned commit-queue dglazkov dino jorlow steveblock oldest_to_newest 263217 0 steveblock 2010-08-11 06:34:45 -0700 Without these empty clients, crashes occur when DEVICE_ORIENTATION is enabled. 263220 1 64106 steveblock 2010-08-11 06:40:25 -0700 Created attachment 64106 Patch 263234 2 64106 jorlow 2010-08-11 07:10:52 -0700 Comment on attachment 64106 Patch r=me 263456 3 64106 commit-queue 2010-08-11 14:35:37 -0700 Comment on attachment 64106 Patch Clearing flags on attachment: 64106 Committed r65185: <http://trac.webkit.org/changeset/65185> 263457 4 commit-queue 2010-08-11 14:35:41 -0700 All reviewed patches have been landed. Closing bug. 263662 5 dino 2010-08-12 02:05:32 -0700 I don't know if I like this approach. This is why I filed https://bugs.webkit.org/show_bug.cgi?id=43533 Rather than create all these empty clients, I think it is better to avoid creating the controller. For example, Page::Page could have this: : m_deviceMotionController((RuntimeEnabledFeatures::deviceMotionEnabled() && pageClients.deviceMotionClient) ? new DeviceMotionController(pageClients.deviceMotionClient) : 0) That way, if the pageClients never gets a deviceMotionClient, then it will not bother constructing a controller. What's the benefit of having an empty client and controller that will never be used? 263667 6 jorlow 2010-08-12 02:22:04 -0700 (In reply to comment #5) > I don't know if I like this approach. This is why I filed > https://bugs.webkit.org/show_bug.cgi?id=43533 > > Rather than create all these empty clients, I think it is better to avoid creating the controller. > > For example, Page::Page could have this: > > : m_deviceMotionController((RuntimeEnabledFeatures::deviceMotionEnabled() && pageClients.deviceMotionClient) ? new DeviceMotionController(pageClients.deviceMotionClient) : 0) > > That way, if the pageClients never gets a deviceMotionClient, then it will not bother constructing a controller. > > What's the benefit of having an empty client and controller that will never be used? I thought about that, but it seemed like the precedent is to create empty clients. Is there something different about this case vs. the others? Is it worth revisiting the approach on the other clients as well? I don't really have a strong opinion either way. 263669 7 steveblock 2010-08-12 02:24:07 -0700 > Rather than create all these empty clients, I think it is better to avoid > creating the controller. Sure, that makes sense. I don't feel strongly either way, but my impression was that code of this kind should generally be written assuming that the client is non-null (if the feature is enabled). Perhaps it's best if you start a discussion on webkit-dev to get some background and to get other people's opinions on this. See also the related discussion in https://lists.webkit.org/pipermail/webkit-dev/2010-July/013597.html 263672 8 steveblock 2010-08-12 02:26:06 -0700 *** Bug 43869 has been marked as a duplicate of this bug. *** 263802 9 dino 2010-08-12 07:38:16 -0700 Pages are created in a number of places (SVGImage, as you show in your patch). They don't always need all the clients (eg. you don't need the inspector client in SVGImage), so there are already cases where null clients are passed in. This gets even more common in the platform implementations. It's a good question as to whether it is better to protect against null clients or to create Empty/Stub versions and use them all over the place. By the way, I'm currently struggling to work out why DeviceOrientation is a client of Page anyway. It seems to me that it would make more sense on Frame, where it could be easily suspended when the user navigates back/forward, or other similar situations. Without this, I get into the case where a Page is still trying to dispatch events even though the document/window have gone away. 64106 2010-08-11 06:40:25 -0700 2010-08-11 14:35:37 -0700 Patch bug-43848-20100811144023.patch text/plain 3426 steveblock SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2NTE1MSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjQgQEAKKzIwMTAtMDgtMTEgIFN0ZXZlIEJsb2NrICA8c3RldmVibG9ja0Bnb29n bGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IE5lZWQgRW1wdHlEZXZpY2VPcmllbnRhdGlvbkNsaWVudCBhbmQgRW1wdHlEZXZpY2VNb3Rpb25D bGllbnQgZm9yIHVzZSB3aXRoIFNWR0ltYWdlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD00Mzg0OAorCisgICAgICAgIFRlc3RlZCBieSBleGlzdGluZyBE ZXZpY2VPcmllbnRhdGlvbiB0ZXN0cy4KKworICAgICAgICAqIGxvYWRlci9FbXB0eUNsaWVudHMu aDoKKyAgICAgICAgKFdlYkNvcmU6OkVtcHR5RGV2aWNlTW90aW9uQ2xpZW50OjpzZXRDb250cm9s bGVyKToKKyAgICAgICAgKFdlYkNvcmU6OkVtcHR5RGV2aWNlTW90aW9uQ2xpZW50OjpzdGFydFVw ZGF0aW5nKToKKyAgICAgICAgKFdlYkNvcmU6OkVtcHR5RGV2aWNlTW90aW9uQ2xpZW50OjpzdG9w VXBkYXRpbmcpOgorICAgICAgICAoV2ViQ29yZTo6RW1wdHlEZXZpY2VNb3Rpb25DbGllbnQ6OmN1 cnJlbnREZXZpY2VNb3Rpb24pOgorICAgICAgICAoV2ViQ29yZTo6RW1wdHlEZXZpY2VPcmllbnRh dGlvbkNsaWVudDo6c2V0Q29udHJvbGxlcik6CisgICAgICAgIChXZWJDb3JlOjpFbXB0eURldmlj ZU9yaWVudGF0aW9uQ2xpZW50OjpzdGFydFVwZGF0aW5nKToKKyAgICAgICAgKFdlYkNvcmU6OkVt cHR5RGV2aWNlT3JpZW50YXRpb25DbGllbnQ6OnN0b3BVcGRhdGluZyk6CisgICAgICAgIChXZWJD b3JlOjpFbXB0eURldmljZU9yaWVudGF0aW9uQ2xpZW50OjpsYXN0T3JpZW50YXRpb24pOgorICAg ICAgICAqIHN2Zy9ncmFwaGljcy9TVkdJbWFnZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpTVkdJ bWFnZTo6ZGF0YUNoYW5nZWQpOgorCiAyMDEwLTA4LTExICBQYXZlbCBGZWxkbWFuICA8cGZlbGRt YW5AY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFJldmlld2VkIGJ5IFl1cnkgU2VtaWtoYXRza3ku CkluZGV4OiBXZWJDb3JlL2xvYWRlci9FbXB0eUNsaWVudHMuaAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJD b3JlL2xvYWRlci9FbXB0eUNsaWVudHMuaAkocmV2aXNpb24gNjUxNDkpCisrKyBXZWJDb3JlL2xv YWRlci9FbXB0eUNsaWVudHMuaAkod29ya2luZyBjb3B5KQpAQCAtMzEsNiArMzEsOCBAQAogI2lu Y2x1ZGUgIkNocm9tZUNsaWVudC5oIgogI2luY2x1ZGUgIkNvbnNvbGUuaCIKICNpbmNsdWRlICJD b250ZXh0TWVudUNsaWVudC5oIgorI2luY2x1ZGUgIkRldmljZU1vdGlvbkNsaWVudC5oIgorI2lu Y2x1ZGUgIkRldmljZU9yaWVudGF0aW9uQ2xpZW50LmgiCiAjaW5jbHVkZSAiRG9jdW1lbnRMb2Fk ZXIuaCIKICNpbmNsdWRlICJEcmFnQ2xpZW50LmgiCiAjaW5jbHVkZSAiRWRpdENvbW1hbmQuaCIK QEAgLTUzMSw2ICs1MzMsMjIgQEAgcHVibGljOgogICAgIHZpcnR1YWwgYm9vbCBzZW5kTWVzc2Fn ZVRvRnJvbnRlbmQoY29uc3QgU3RyaW5nJikgeyByZXR1cm4gZmFsc2U7IH0KIH07CiAKK2NsYXNz IEVtcHR5RGV2aWNlTW90aW9uQ2xpZW50IDogcHVibGljIERldmljZU1vdGlvbkNsaWVudCB7Citw dWJsaWM6CisgICAgdmlydHVhbCB2b2lkIHNldENvbnRyb2xsZXIoRGV2aWNlTW90aW9uQ29udHJv bGxlciopIHsgfQorICAgIHZpcnR1YWwgdm9pZCBzdGFydFVwZGF0aW5nKCkgeyB9CisgICAgdmly dHVhbCB2b2lkIHN0b3BVcGRhdGluZygpIHsgfQorICAgIHZpcnR1YWwgRGV2aWNlTW90aW9uRGF0 YSogY3VycmVudERldmljZU1vdGlvbigpIGNvbnN0IHsgcmV0dXJuIDA7IH0KK307CisKK2NsYXNz IEVtcHR5RGV2aWNlT3JpZW50YXRpb25DbGllbnQgOiBwdWJsaWMgRGV2aWNlT3JpZW50YXRpb25D bGllbnQgeworcHVibGljOgorICAgIHZpcnR1YWwgdm9pZCBzZXRDb250cm9sbGVyKERldmljZU9y aWVudGF0aW9uQ29udHJvbGxlciopIHsgfQorICAgIHZpcnR1YWwgdm9pZCBzdGFydFVwZGF0aW5n KCkgeyB9CisgICAgdmlydHVhbCB2b2lkIHN0b3BVcGRhdGluZygpIHsgfQorICAgIHZpcnR1YWwg RGV2aWNlT3JpZW50YXRpb24qIGxhc3RPcmllbnRhdGlvbigpIGNvbnN0IHsgcmV0dXJuIDA7IH0K K307CisKIH0KIAogI2VuZGlmIC8vIEVtcHR5Q2xpZW50c19oCkluZGV4OiBXZWJDb3JlL3N2Zy9n cmFwaGljcy9TVkdJbWFnZS5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9zdmcvZ3JhcGhpY3Mv U1ZHSW1hZ2UuY3BwCShyZXZpc2lvbiA2NTE0OSkKKysrIFdlYkNvcmUvc3ZnL2dyYXBoaWNzL1NW R0ltYWdlLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMjUzLDcgKzI1MywxMyBAQCBib29sIFNWR0lt YWdlOjpkYXRhQ2hhbmdlZChib29sIGFsbERhdGFSCiAjZW5kaWYKICAgICAgICAgc3RhdGljIElu c3BlY3RvckNsaWVudCogZHVtbXlJbnNwZWN0b3JDbGllbnQgPSBuZXcgRW1wdHlJbnNwZWN0b3JD bGllbnQ7CiAgICAgICAgIHBhZ2VDbGllbnRzLmluc3BlY3RvckNsaWVudCA9IGR1bW15SW5zcGVj dG9yQ2xpZW50OwotICAgICAgICAKKyNpZiBFTkFCTEUoREVWSUNFX09SSUVOVEFUSU9OKQorICAg ICAgICBzdGF0aWMgRGV2aWNlTW90aW9uQ2xpZW50KiBkdW1teURldmljZU1vdGlvbkNsaWVudCA9 IG5ldyBFbXB0eURldmljZU1vdGlvbkNsaWVudDsKKyAgICAgICAgcGFnZUNsaWVudHMuZGV2aWNl TW90aW9uQ2xpZW50ID0gZHVtbXlEZXZpY2VNb3Rpb25DbGllbnQ7CisgICAgICAgIHN0YXRpYyBE ZXZpY2VPcmllbnRhdGlvbkNsaWVudCogZHVtbXlEZXZpY2VPcmllbnRhdGlvbkNsaWVudCA9IG5l dyBFbXB0eURldmljZU9yaWVudGF0aW9uQ2xpZW50OworICAgICAgICBwYWdlQ2xpZW50cy5kZXZp Y2VPcmllbnRhdGlvbkNsaWVudCA9IGR1bW15RGV2aWNlT3JpZW50YXRpb25DbGllbnQ7CisjZW5k aWYKKwogICAgICAgICAvLyBGSVhNRTogSWYgdGhpcyBTVkcgZW5kcyB1cCBsb2FkaW5nIGl0c2Vs Ziwgd2UgbWlnaHQgbGVhayB0aGUgd29ybGQuCiAgICAgICAgIC8vIFRoZSBjb21tZW50IHNhaWQg dGhhdCB0aGUgQ2FjaGUgY29kZSBkb2VzIG5vdCBrbm93IGFib3V0IENhY2hlZEltYWdlcwogICAg ICAgICAvLyBob2xkaW5nIEZyYW1lcyBhbmQgd29uJ3Qga25vdyB0byBicmVhayB0aGUgY3ljbGUu IEJ1dCAK