DeviceOrientationClient and DeviceMotionClient should have controllerDestroyed() methods

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Steve Block]

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.

[Steve Block]

Created attachment 67806 [details]
Patch

[Hans Wennborg]

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]

> 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]

Someone should review this from the mac port side.  Darin, any thoughts about the naming of the methods?

[Steve Block]

> 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)]

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

Created attachment 68107 [details]
Patch

[Jeremy Orlow]

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 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 on attachment 68107 [details]
Patch

Clearing flags on attachment: 68107

Committed r67949: <http://trac.webkit.org/changeset/67949>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.