Bug 43848 - Need EmptyDeviceOrientationClient and EmptyDeviceMotionClient for use with SVGImage
Summary: Need EmptyDeviceOrientationClient and EmptyDeviceMotionClient for use with SV...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 43869 (view as bug list)
Depends on:
Blocks: 30335
  Show dependency treegraph
 
Reported: 2010-08-11 06:34 PDT by Steve Block
Modified: 2010-08-12 07:38 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.35 KB, patch)
2010-08-11 06:40 PDT, Steve Block
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2010-08-11 06:34:45 PDT
Without these empty clients, crashes occur when DEVICE_ORIENTATION is enabled.
Comment 1 Steve Block 2010-08-11 06:40:25 PDT
Created attachment 64106 [details]
Patch
Comment 2 Jeremy Orlow 2010-08-11 07:10:52 PDT
Comment on attachment 64106 [details]
Patch

r=me
Comment 3 WebKit Commit Bot 2010-08-11 14:35:37 PDT
Comment on attachment 64106 [details]
Patch

Clearing flags on attachment: 64106

Committed r65185: <http://trac.webkit.org/changeset/65185>
Comment 4 WebKit Commit Bot 2010-08-11 14:35:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Dean Jackson 2010-08-12 02:05:32 PDT
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?
Comment 6 Jeremy Orlow 2010-08-12 02:22:04 PDT
(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.
Comment 7 Steve Block 2010-08-12 02:24:07 PDT
> 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
Comment 8 Steve Block 2010-08-12 02:26:06 PDT
*** Bug 43869 has been marked as a duplicate of this bug. ***
Comment 9 Dean Jackson 2010-08-12 07:38:16 PDT
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.