Bug 50061

Summary: Move requestGeolocationPermissionForFrame to GeolocationClient
Product: WebKit Reporter: John Knottenbelt <jknotten>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jorlow, sam, sfalken, steveblock
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 45752, 46895    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description John Knottenbelt 2010-11-25 02:33:10 PST
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.
Comment 1 John Knottenbelt 2010-11-25 03:12:48 PST
Created attachment 74846 [details]
Patch
Comment 2 Steve Block 2010-11-25 05:01:48 PST
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.
Comment 3 John Knottenbelt 2010-11-25 07:51:09 PST
Created attachment 74878 [details]
Patch
Comment 4 Steve Block 2010-11-25 08:03:59 PST
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?
Comment 5 John Knottenbelt 2010-11-26 04:27:13 PST
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.
Comment 6 John Knottenbelt 2010-11-26 04:29:46 PST
Created attachment 74918 [details]
Patch
Comment 7 Steve Block 2010-11-26 04:31:58 PST
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
Comment 8 John Knottenbelt 2010-11-26 04:34:09 PST
Created attachment 74921 [details]
Patch
Comment 9 Steve Block 2010-11-26 04:45:59 PST
Comment on attachment 74921 [details]
Patch

r=me
Comment 10 Jeremy Orlow 2010-11-26 05:01:33 PST
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 ;
Comment 11 John Knottenbelt 2010-11-26 06:19:19 PST
Created attachment 74927 [details]
Patch
Comment 12 Steve Block 2010-11-26 06:28:06 PST
Comment on attachment 74927 [details]
Patch

r=me
Comment 13 WebKit Commit Bot 2010-11-26 07:17:42 PST
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
Comment 14 Steve Block 2010-11-26 09:21:55 PST
Comment on attachment 74927 [details]
Patch

Maybe queue flakiness - trying again
Comment 15 WebKit Commit Bot 2010-11-26 10:10:33 PST
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
Comment 16 Steve Block 2010-12-01 13:29:53 PST
Created attachment 75315 [details]
Patch
Comment 17 Steve Block 2010-12-02 01:49:20 PST
Comment on attachment 75315 [details]
Patch

Fixed use of ASSERT_UNUSED in GeolocationClientMock::permissionTimerFired()
Comment 18 WebKit Commit Bot 2010-12-02 10:59:22 PST
Comment on attachment 75315 [details]
Patch

Clearing flags on attachment: 75315

Committed r73163: <http://trac.webkit.org/changeset/73163>
Comment 19 WebKit Commit Bot 2010-12-02 10:59:29 PST
All reviewed patches have been landed.  Closing bug.