Without these empty clients, crashes occur when DEVICE_ORIENTATION is enabled.
Created attachment 64106 [details] Patch
Comment on attachment 64106 [details] Patch r=me
Comment on attachment 64106 [details] Patch Clearing flags on attachment: 64106 Committed r65185: <http://trac.webkit.org/changeset/65185>
All reviewed patches have been landed. Closing bug.
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?
(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.
> 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
*** Bug 43869 has been marked as a duplicate of this bug. ***
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.