[Webkit-unassigned] [Bug 50061] Move requestGeolocationPermissionForFrame to GeolocationClient

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50061





--- Comment #2 from Steve Block <steveblock at google.com>  2010-11-25 05:01:48 PST ---
(From update of attachment 74846)
View in context: https://bugs.webkit.org/attachment.cgi?id=74846&action=review

> WebCore/page/ChromeClient.h:209
> +#if !ENABLE(CLIENT_BASED_GEOLOCATION)

It's probably confusing to add these guards. Currently, the methods are unguarded, so all ports have to implement the methods. Adding these guards means that all ports not implementing Geolocation, and those implementing non-client-based Geolocation, have to do so, which is odd.

> WebCore/platform/mock/GeolocationClientMock.cpp:47
>  {

Should either init m_permissionState or call reset()

> WebCore/platform/mock/GeolocationClientMock.cpp:93
> +}

It is certain that all Geolocation objects will call cancel, so the timer is always stopped, before this object is destroyed?

> WebCore/platform/mock/GeolocationClientMock.cpp:109
> +    // Add comment here about how reentrancy that modifies m_pendingPermission is not possible.

!!

> WebCore/platform/mock/GeolocationClientMock.h:71
>  private:

usually have a blank line above 'private'

> WebKit/mac/ChangeLog:10
> +        interface.

Maybe describe that you're just moving some code unchanged from the ChromeClient to the GeolocationClient.

> WebKit/mac/WebCoreSupport/WebChromeClient.mm:1
> +

intentional ?

> WebKit/mac/WebCoreSupport/WebGeolocationClient.h:47
> +#if ENABLE(CLIENT_BASED_GEOLOCATION)

I don't think we need these guards - the whole class is for the client-base impl. On a related note, can you file a bug to remove the Mac GeolocationService, which I think is dead code.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.