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.
Created attachment 67806 [details] Patch
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.
> 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.
Someone should review this from the mac port side. Darin, any thoughts about the naming of the methods?
> 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().
(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.
Created attachment 68107 [details] Patch
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?
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.
Comment on attachment 68107 [details] Patch Clearing flags on attachment: 68107 Committed r67949: <http://trac.webkit.org/changeset/67949>
All reviewed patches have been landed. Closing bug.