WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
43848
Need EmptyDeviceOrientationClient and EmptyDeviceMotionClient for use with SVGImage
https://bugs.webkit.org/show_bug.cgi?id=43848
Summary
Need EmptyDeviceOrientationClient and EmptyDeviceMotionClient for use with SV...
Steve Block
Reported
2010-08-11 06:34:45 PDT
Without these empty clients, crashes occur when DEVICE_ORIENTATION is enabled.
Attachments
Patch
(3.35 KB, patch)
2010-08-11 06:40 PDT
,
Steve Block
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Steve Block
Comment 1
2010-08-11 06:40:25 PDT
Created
attachment 64106
[details]
Patch
Jeremy Orlow
Comment 2
2010-08-11 07:10:52 PDT
Comment on
attachment 64106
[details]
Patch r=me
WebKit Commit Bot
Comment 3
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
>
WebKit Commit Bot
Comment 4
2010-08-11 14:35:41 PDT
All reviewed patches have been landed. Closing bug.
Dean Jackson
Comment 5
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?
Jeremy Orlow
Comment 6
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.
Steve Block
Comment 7
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
Steve Block
Comment 8
2010-08-12 02:26:06 PDT
***
Bug 43869
has been marked as a duplicate of this bug. ***
Dean Jackson
Comment 9
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug