Bug 45891

Summary:

DeviceOrientationClient and DeviceMotionClient should have controllerDestroyed() methods

Product:

WebKit

Reporter:

Steve Block <steveblock>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cmarrin, commit-queue, fishd, hans, jorlow, joth, simon.fraser, steveblock

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

30335

Attachments:

Description	Flags
Patch	none
Patch	none

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	Details Formatted Diff Diff
Patch (9.49 KB, patch) 2010-09-20 10:22 PDT, Steve Block	no flags	Details Formatted Diff Diff
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.