Bug 50061 – Move requestGeolocationPermissionForFrame to GeolocationClient

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[John Knottenbelt]

With the client-based geolocation design, it makes sense for the GeolocationClient to be the entry point for geolocation permission policy, rather than on the ChromeClient.

[John Knottenbelt]

Created attachment 74846 [details]
Patch

[Steve Block]

Comment on attachment 74846 [details]
Patch

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.

[John Knottenbelt]

Created attachment 74878 [details]
Patch

[Steve Block]

Comment on attachment 74878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74878&action=review

> WebCore/page/Chrome.cpp:421
> +#if !ENABLE(CLIENT_BASED_GEOLOCATION)

I think we should remove these guards too, for consistency.

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

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

[John Knottenbelt]

Comment on attachment 74878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74878&action=review

>> WebCore/page/Chrome.cpp:421
>> +#if !ENABLE(CLIENT_BASED_GEOLOCATION)
> 
> I think we should remove these guards too, for consistency.

Agree.

>> WebCore/platform/mock/GeolocationClientMock.cpp:95
>> +}
> 
> It is certain that all Geolocation objects will call cancel, so the timer is always stopped, before this object is destroyed?

Yes. Perhaps my comment was too terse. when the Frame that hosts the Window that hosts the Navigator object that hosts the Geolocation object is closed, the disconnectFrame() method is called on the Geolocation object which calls cancelPermissionRequest. 

The timers will also be automatically stopped by the Timer destructor on destruction of the containing GeolocationClientMock object.

[John Knottenbelt]

Created attachment 74918 [details]
Patch

[Steve Block]

Comment on attachment 74918 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74918&action=review

> WebCore/ChangeLog:12
> +        * page/Chrome.cpp:

No longer needed

[John Knottenbelt]

Created attachment 74921 [details]
Patch

[Steve Block]

Comment on attachment 74921 [details]
Patch

r=me

[Jeremy Orlow]

Comment on attachment 74921 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74921&action=review

drive by comments

> WebKit/mac/WebCoreSupport/WebChromeClient.h:177
> +    virtual void requestGeolocationPermissionForFrame(WebCore::Frame*, WebCore::Geolocation*) { };

no ;

> WebKit/win/WebCoreSupport/WebChromeClient.h:164
> +    virtual void requestGeolocationPermissionForFrame(WebCore::Frame*, WebCore::Geolocation*) { };

no ;

[John Knottenbelt]

Created attachment 74927 [details]
Patch

[Steve Block]

Comment on attachment 74927 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 74927 [details]
Patch

Rejecting patch 74927 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
Last 500 characters of output:
Core.build/Debug/WebCore.build/Objects-normal/x86_64/DOMHTMLCanvasElement.o /Projects/CommitQueue/WebKitBuild/Debug/DerivedSources/WebCore/DOMHTMLCanvasElement.mm normal x86_64 objective-c++ com.apple.compilers.gcc.4_2
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/DOMHTMLButtonElement.o /Projects/CommitQueue/WebKitBuild/Debug/DerivedSources/WebCore/DOMHTMLButtonElement.mm normal x86_64 objective-c++ com.apple.compilers.gcc.4_2
(79 failures)


Full output: http://queues.webkit.org/results/6433009

[Steve Block]

Comment on attachment 74927 [details]
Patch

Maybe queue flakiness - trying again

[WebKit Commit Bot]

Comment on attachment 74927 [details]
Patch

Rejecting patch 74927 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
Last 500 characters of output:
RSION_MINOR 0320
    setenv YACC /Developer/usr/bin/yacc
    /bin/sh -c /Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Script-5DF50887116F3077005202AB.sh

** BUILD FAILED **


The following build commands failed:
WebCore:
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/GeolocationClientMock.o /Projects/CommitQueue/WebCore/platform/mock/GeolocationClientMock.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/6366067

[Steve Block]

Created attachment 75315 [details]
Patch

[Steve Block]

Comment on attachment 75315 [details]
Patch

Fixed use of ASSERT_UNUSED in GeolocationClientMock::permissionTimerFired()

[WebKit Commit Bot]

Comment on attachment 75315 [details]
Patch

Clearing flags on attachment: 75315

Committed r73163: <http://trac.webkit.org/changeset/73163>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.