RESOLVED FIXED 45891
DeviceOrientationClient and DeviceMotionClient should have controllerDestroyed() methods 
https://bugs.webkit.org/show_bug.cgi?id=45891
Summary DeviceOrientationClient and DeviceMotionClient should have controllerDestroye...
Steve Block
Reported 2010-09-16 07:50:10 PDT
This is the pattern used for other controller/client pairs and will allow the client to be destroyed by the embedder when the Page and hence controller is destroyed.
Attachments
Patch (9.21 KB, patch)
2010-09-16 09:41 PDT, Steve Block 		no flags
DetailsFormatted DiffDiff
Patch (9.49 KB, patch)
2010-09-20 10:22 PDT, Steve Block 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Steve Block
Comment 1 2010-09-16 09:41:18 PDT
Created attachment 67806 [details] Patch
Hans Wennborg
Comment 2 2010-09-17 02:31:29 PDT
This looks fine. My only concern was the empty clients, and whether they might be leaked by some implementations because they implement controllerDestroyed() as a nop. But the way they're used in SVGImage, it doesn't seem to be a real concern.
Steve Block
Comment 3 2010-09-17 03:02:17 PDT
> My only concern was the empty clients, and whether they might be leaked by some > implementations because they implement controllerDestroyed() as a nop. But the > way they're used in SVGImage, it doesn't seem to be a real concern. Yes, I considered this too, but all of the other empty clients implement it as a no-op, so I followed the pattern.
Jeremy Orlow
Comment 4 2010-09-17 05:12:26 PDT
Someone should review this from the mac port side. Darin, any thoughts about the naming of the methods?
Steve Block
Comment 5 2010-09-20 02:34:18 PDT
> Someone should review this from the mac port side. Added Chris and Simon who looked at the original patch to add the Mac client. > Darin, any thoughts about the naming of the methods? The naming for existing clients doesn't seem to follow a precise pattern. Usually, with a FooController and a FooClient, the method is fooControllerDestroyed(), but there are exceptions, such as {Chrome, ChromeClient, chromeDestroyed()} and {GeolocationController, GeolocationControllerClient and geolocationDestroyed()}. deviceOrientationControllerDestroyed() seemed needlessly wordy, as it's pretty obvious which controller has been destroyed, so I went with controllerDestroyed().
Darin Fisher (:fishd, Google)
Comment 6 2010-09-20 09:33:29 PDT
(In reply to comment #5) > > Someone should review this from the mac port side. > Added Chris and Simon who looked at the original patch to add the Mac client. > > > Darin, any thoughts about the naming of the methods? > The naming for existing clients doesn't seem to follow a precise pattern. Usually, with a FooController and a FooClient, the method is fooControllerDestroyed(), but there are exceptions, such as {Chrome, ChromeClient, chromeDestroyed()} and {GeolocationController, GeolocationControllerClient and geolocationDestroyed()}. > > deviceOrientationControllerDestroyed() seemed needlessly wordy, as it's pretty obvious which controller has been destroyed, so I went with controllerDestroyed(). You could run into a problem if you ever want to have a class that implements multiple Client interfaces. Suppose two Client interfaces both declare controllerDestroyed() methods.
Steve Block
Comment 7 2010-09-20 10:22:01 PDT
Created attachment 68107 [details] Patch
Jeremy Orlow
Comment 8 2010-09-21 05:41:21 PDT
Comment on attachment 68107 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68107&action=review > WebKit/mac/WebCoreSupport/WebDeviceOrientationClient.mm:65 > + delete this; I'm not positive about this part. If you are, then I guess we can go with it. If you aren't, maybe this should go in its own patch for someone from the mac port to review?
Steve Block
Comment 9 2010-09-21 06:40:28 PDT
Comment on attachment 68107 [details] Patch I'm pretty confident this is correct. Will happily roll back or update if the Mac guys have any comments.
WebKit Commit Bot
Comment 10 2010-09-21 07:03:45 PDT
Comment on attachment 68107 [details] Patch Clearing flags on attachment: 68107 Committed r67949: <http://trac.webkit.org/changeset/67949>
WebKit Commit Bot
Comment 11 2010-09-21 07:03:52 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.