Bug 43848 – Need EmptyDeviceOrientationClient and EmptyDeviceMotionClient for use with SVGImage

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

Without these empty clients, crashes occur when DEVICE_ORIENTATION is enabled.

[Steve Block]

Created attachment 64106 [details]
Patch

[Jeremy Orlow]

Comment on attachment 64106 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 64106 [details]
Patch

Clearing flags on attachment: 64106

Committed r65185: <http://trac.webkit.org/changeset/65185>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dean Jackson]

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?

[Jeremy Orlow]

(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.

[Steve Block]

> 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

[Steve Block]

*** Bug 43869 has been marked as a duplicate of this bug. ***

[Dean Jackson]

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.