Bug 50061

Created attachment 74846 [details] Patch

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.

Created attachment 74878 [details] Patch

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

Created attachment 74918 [details] Patch

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

Created attachment 74921 [details] Patch

Comment on attachment 74921 [details] Patch r=me

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 ;

Created attachment 74927 [details] Patch

Comment on attachment 74927 [details] Patch r=me

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 on attachment 74927 [details] Patch Maybe queue flakiness - trying again

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

Created attachment 75315 [details] Patch

Comment on attachment 75315 [details] Patch Fixed use of ASSERT_UNUSED in GeolocationClientMock::permissionTimerFired()

Comment on attachment 75315 [details] Patch Clearing flags on attachment: 75315 Committed r73163: <http://trac.webkit.org/changeset/73163>

All reviewed patches have been landed. Closing bug.