Created attachment 127459 [details] Patch

There's actually some more code to remove related to GeolocationService.cpp, but we can do that in a followup patch.

Comment on attachment 127459 [details] Patch I wonder if any of these ENABLEs you're removing need to be replaced by ENABLE(GEOLOCATION) instead.

I recommend waiting for the EWS bots to clear.

Created attachment 127462 [details] Patch

We still use that code, please wait a bit before removing it.

> We still use that code, please wait a bit before removing it. Which port uses the code?

Comment on attachment 127462 [details] Patch Attachment 127462 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11493019

(In reply to comment #7) > > We still use that code, please wait a bit before removing it. > > Which port uses the code? iOS?

(In reply to comment #9) > (In reply to comment #7) > > > We still use that code, please wait a bit before removing it. > > > > Which port uses the code? iOS. We have reasons to keep maintaining this code at the moment. We can remove that later. Is this blocking other architectural changes?

Comment on attachment 127462 [details] Patch Attachment 127462 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11534984

I replied to Benjamin on webkit-dev.

Comment on attachment 127462 [details] Patch I need to resolve these compile issues.

BTW, feel free to remove the GeolocationServiceEfl code (which seems to be one of the last things still interacting with GeolocationService) if/when needed, it's mostly dummy code with no real use.

I wonder if build-webkit doesn't need to be updated as well.

Comment on attachment 127462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127462&action=review > Source/JavaScriptCore/wtf/Platform.h:1090 > pre-emptive permission policy is enabled by default for all client-based implementations. */ Stale comment

> I wonder if any of these ENABLEs you're removing need to be replaced by > ENABLE(GEOLOCATION) instead. That shouldn't be the case. When we introduced ENABLE(CLIENT_BASED_GEOLOCATION), it was always as a switch _within_ the Geolocation code path.

Comment on attachment 127462 [details] Patch Attachment 127462 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11536556

I fix build breaks simply for EFL port after looking over this patch. But, I'm not sure if this modification is right for your patch. Anyway, please check EFL port build as well. :-) --- a/Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.h +++ b/Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.h @@ -151,8 +151,6 @@ public: virtual void scrollRectIntoView(const IntRect&) const { } - virtual void requestGeolocationPermissionForFrame(Frame*, Geolocation*); - virtual void cancelGeolocationPermissionRequestForFrame(Frame*, Geolocation*); --- a/Source/WebCore/page/Geolocation.cpp +++ b/Source/WebCore/page/Geolocation.cpp #if USE(PREEMPT_GEOLOCATION_PERMISSION) @@ -768,6 +714,10 @@ Geolocation::~Geolocation() {} void Geolocation::setIsAllowed(bool) {} +void Geolocation::setError(GeolocationError*) {} + +void Geolocation::positionChanged() {}

(In reply to comment #14) > BTW, feel free to remove the GeolocationServiceEfl code (which seems to be one of the last things still interacting with GeolocationService) if/when needed, it's mostly dummy code with no real use. Can you please already do that in a separate patch? That will make this patch easier to fix when it will land.

(In reply to comment #20) > (In reply to comment #14) > > BTW, feel free to remove the GeolocationServiceEfl code (which seems to be one of the last things still interacting with GeolocationService) if/when needed, it's mostly dummy code with no real use. > > Can you please already do that in a separate patch? > That will make this patch easier to fix when it will land. Sure, files http://webkit.org/b/79270 for that.

Created attachment 131142 [details] Patch

Comment on attachment 131142 [details] Patch Attachment 131142 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11911870

Comment on attachment 131142 [details] Patch Attachment 131142 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11933014

Created attachment 131149 [details] Patch

Comment on attachment 131149 [details] Patch Attachment 131149 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11933039

Created attachment 131482 [details] Patch

Comment on attachment 131482 [details] Patch Attachment 131482 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11947303

Created attachment 131541 [details] Patch

Comment on attachment 131541 [details] Patch Attachment 131541 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11949258

Created attachment 131557 [details] Patch Hopefully gtk will build this time :(

Comment on attachment 131557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131557&action=review Looks great. Is GeolocationServiceEFL gone already? > ChangeLog:1 > +2012-03-12 Adam Barth <abarth@webkit.org> && Benjamin Poulain <bpoulain@apple.com> Feel free to take full credit. :)

(In reply to comment #32) > Looks great. Is GeolocationServiceEFL gone already? Yes.

Committed r110595: <http://trac.webkit.org/changeset/110595>

Did qtwebkit fail to build or needs a clean build after this? In file included from ../../../Source/WebKit/qt/WebCoreSupport/GeolocationPermissionClientQt.cpp:33: ../../../Source/WebKit/qt/WebCoreSupport/GeolocationPermissionClientQt.h:49: error: 'Geolocation' has not been declared ../../../Source/WebKit/qt/WebCoreSupport/GeolocationPermissionClientQt.h:50: error: 'Geolocation' has not been declared ../../../Source/WebKit/qt/WebCoreSupport/GeolocationPermissionClientQt.h:55: error: 'Geolocation' was not declared in this scope ../../../Source/WebKit/qt/WebCoreSupport/GeolocationPermissionClientQt.h:55: error: template argument 2 is invalid /usr/local/Trolltech/Qt-4.8.0/bin/moc -DENABLE_REQUEST_ANIMATION_FRAME=0 -DENABLE_DOWNLOAD_ATTRIBUTE=0 -DENABLE_WEBGL=0 -DENABLE_3D_RENDERING=0 -DENABLE_ACCELERATED_2D_CANVAS=0 -DENABLE_ANIMATION_API=0 -DENABLE_BLOB=0 -DENABLE_CHANNEL_MESSAGING=0 -DENABLE_CSS_FILTERS=0 -DENABLE_CSS_GRID_LAYOUT=0 -DENABLE_CSS_SHADERS=0 -DENABLE_SQL_DATABASE=0 -DENABLE_DATALIST=0 -DENABLE_DATA_TRANSFER_ITEMS=0 -DENABLE_DETAILS=0 -DENABLE_DEVICE_ORIENTATION=0 -DENABLE_DIRECTORY_UPLOAD=0 -DENABLE_FILE_SYSTEM=0 -DENABLE_FILTERS=0 -DENABLE_FTPDIR=0 -DENABLE_FULLSCREEN_API=0 -DENABLE_GAMEPAD=0 -DENABLE_GEOLOCATION=0 -DENABLE_ICONDATABASE=0 -DENABLE_INDEXED_DATABASE=0 -DENABLE_INPUT_COLOR=0 -DENABLE_INPUT_SPEECH=0 -DENABLE_SCRIPTED_SPEECH=0 -DENABLE_INPUT_TYPE_DATE=0 -DENABLE_INPUT_TYPE_DATETIME=0 -DENABLE_INPUT_TYPE_DATETIMELOCAL=0 -DENABLE_INPUT_TYPE_MONTH=0 -DENABLE_INPUT_TYPE_TIME=0 -DENABLE_INPUT_TYPE_WEEK=0 -DENABLE_INSPECTOR=0 -DENABLE_JAVASCRIPT_DEBUGGER=0 -DENABLE_LEGACY_NOTIFICATIONS=0 -DENABLE_LINK_PREFETCH=0 -DENABLE_MATHML=0 -DENABLE_MEDIA_SOURCE=0 -DENABLE_MEDIA_STATISTICS=0 -DENABLE_MEDIA_STREAM=0 -DENABLE_METER_TAG=0 -DENABLE_MHTML=0 -DENABLE_MICRODATA=0 -DENABLE_MUTATION_OBSERVERS=0 -DENABLE_NETSCAPE_PLUGIN_API=0 -DENABLE_NOTIFICATIONS=0 -DENABLE_ORIENTATION_EVENTS=0 -DENABLE_PAGE_VISIBILITY_API=0 -DENABLE_PROGRESS_TAG=0 -DENABLE_QUOTA=0 -DENABLE_REGISTER_PROTOCOL_HANDLER=0 -DUSE_SYSTEM_MALLOC=0 -DENABLE_SHADOW_DOM=0 -DENABLE_SHARED_WORKERS=0 -DENABLE_STYLE_SCOPED=0 -DENABLE_SVG=0 -DENABLE_SVG_DOM_OBJC_BINDINGS=0 -DENABLE_SVG_FONTS=0 -DWTF_USE_TILED_BACKING_STORE=0 -DENABLE_TOUCH_EVENTS=0 -DENABLE_TOUCH_ICON_LOADING=0 -DENABLE_VIBRATION=0 -DENABLE_VIDEO=0 -DENABLE_VIDEO_TRACK=0 -DENABLE_WEB_AUDIO=0 -DENABLE_WEB_SOCKETS=0 -DENABLE_WEB_TIMING=0 -DENABLE_WORKERS=0 -DWTF_USE_WTFURL=0 -DENABLE_XSLT=0 -DENABLE_NETSCAPE_PLUGIN_API=0 -DWTF_USE_QT4_UNICODE=1 -DENABLE_SVG_FONTS=0 -DHAVE_FONTCONFIG=1 -DENABLE_DASHBOARD_SUPPORT=0 -DWTF_USE_QT_IMAGE_DECODER=1 -DENABLE_SVG_FONTS=0 -DPLUGIN_ARCHITECTURE_UNSUPPORTED=1 -DHAVE_QSTYLE=1 -DENABLE_GESTURE_EVENTS=1 -DENABLE_JAVASCRIPT_DEBUGGER=0 -DHAVE_QQUICK1=1 -DQT_MAKEDLL -DBUILDING_QT__=1 -DNDEBUG -DBUILDING_QtWebKit -DBUILDING_WEBKIT -DQT_ASCII_CAST_WARNINGS -DWTF_USE_TEXTURE_MAPPER=1 -DWTF_USE_TEXTURE_MAPPER_GL=1 -DENABLE_GLIB_SUPPORT=1 -DQT_NO_DEBUG -DQT_SQL_LIB -DQT_OPENGL_LIB -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_CORE_LIB -DQT_SHARED -I/usr/local/Trolltech/Qt-4.8.0/mkspecs/linux-g++-32 -I../../../Source -I/usr/local/Trolltech/Qt-4.8.0/include/QtCore -I/usr/local/Trolltech/Qt-4.8.0/include/QtNetwork -I/usr/local/Trolltech/Qt-4.8.0/include/QtGui -I/usr/local/Trolltech/Qt-4.8.0/include/QtOpenGL -I/usr/local/Trolltech/Qt-4.8.0/include/QtSql -I/usr/local/Trolltech/Qt-4.8.0/include -I/usr/local/Trolltech/qt-mobility-opensource-1.2.0-for-Qt-4.8.0/include/QtSensors -I. -I../../../Source/WebKit/qt/Api -I../../../Source/WebKit/qt/WebCoreSupport -I../../../Source -I/ramdisk/qt-linux-release-minimal/build/WebKitBuild/Release/Source/include -I../../../Source/WebCore -I../../../Source/WebCore/Modules/filesystem -I../../../Source/WebCore/Modules/geolocation -I../../../Source/WebCore/Modules/indexeddb -I../../../Source/WebCore/Modules/webdatabase -I../../../Source/WebCore/Modules/websockets -I../../../Source/WebCore/accessibility -I../../../Source/WebCore/bindings -I../../../Source/WebCore/bindings/generic -I../../../Source/WebCore/bridge -I../../../Source/WebCore/bridge/qt -I../../../Source/WebCore/css -I../../../Source/WebCore/dom -I../../../Source/WebCore/dom/default -I../../../Source/WebCore/editing -I../../../Source/WebCore/fileapi -I../../../Source/WebCore/history -I../../../Source/WebCore/html -I../../../Source/WebCore/html/canvas -I../../../Source/WebCore/html/parser -I../../../Source/WebCore/html/shadow -I../../../Source/WebCore/html/track -I../../../Source/WebCore/inspector -I../../../Source/WebCore/loader -I../../../Source/WebCore/loader/appcache -I../../../Source/WebCore/loader/archive -I../../../Source/WebCore/loader/cache -I../../../Source/WebCore/loader/icon -I../../../Source/WebCore/mathml -I../../../Source/WebCore/notifications -I../../../Source/WebCore/page -I../../../Source/WebCore/page/animation -I../../../Source/WebCore/page/qt -I../../../Source/WebCore/page/scrolling -I../../../Source/WebCore/platform -I../../../Source/WebCore/platform/animation -I../../../Source/WebCore/platform/audio -I../../../Source/WebCore/platform/graphics -I../../../Source/WebCore/platform/graphics/filters -I../../../Source/WebCore/platform/graphics/filters/arm -I../../../Source/WebCore/platform/graphics/opengl -I../../../Source/WebCore/platform/graphics/qt -I../../../Source/WebCore/platform/graphics/texmap -I../../../Source/WebCore/platform/graphics/transforms -I../../../Source/WebCore/platform/image-decoders -I../../../Source/WebCore/platform/leveldb -I../../../Source/WebCore/platform/mock -I../../../Source/WebCore/platform/network -I../../../Source/WebCore/platform/network/qt -I../../../Source/WebCore/platform/qt -I../../../Source/WebCore/platform/sql -I../../../Source/WebCore/platform/text -I../../../Source/WebCore/platform/text/transcoder -I../../../Source/WebCore/plugins -I../../../Source/WebCore/rendering -I../../../Source/WebCore/rendering/mathml -I../../../Source/WebCore/rendering/style -I../../../Source/WebCore/rendering/svg -I../../../Source/WebCore/storage -I../../../Source/WebCore/svg -I../../../Source/WebCore/svg/animation -I../../../Source/WebCore/svg/graphics -I../../../Source/WebCore/svg/graphics/filters -I../../../Source/WebCore/svg/properties -I../../../Source/WebCore/testing -I../../../Source/WebCore/webaudio -I/ramdisk/qt-linux-release-minimal/build/Source/WebCore/websockets -I../../../Source/WebCore/workers -I../../../Source/WebCore/xml -I../../../Source/WebCore/xml/parser -I../../../Source/ThirdParty -I../../../Source/WebCore/bridge/jsc -I../../../Source/WebCore/bindings/js -I/ramdisk/qt-linux-release-minimal/build/Source/WebCore/bindings/js/specialization -I../../../Source/WebCore/bridge/c -I../../../Source/WebCore/testing/js -IWebCore/generated -I/usr/local/Trolltech/Qt-4.8.0/src/3rdparty/sqlite/ -I../../../Source/JavaScriptCore -I../../../Source -I../../../Source/JavaScriptCore/assembler -I../../../Source/JavaScriptCore/bytecode -I../../../Source/JavaScriptCore/bytecompiler -I../../../Source/JavaScriptCore/heap -I../../../Source/JavaScriptCore/dfg -I../../../Source/JavaScriptCore/debugger -I../../../Source/JavaScriptCore/interpreter -I../../../Source/JavaScriptCore/jit -I../../../Source/JavaScriptCore/llint -I../../../Source/JavaScriptCore/parser -I../../../Source/JavaScriptCore/profiler -I../../../Source/JavaScriptCore/runtime -I../../../Source/JavaScriptCore/tools -I../../../Source/JavaScriptCore/yarr -I../../../Source/JavaScriptCore/API -I../../../Source/JavaScriptCore/ForwardingHeaders -IJavaScriptCore/generated -I../../../Source/JavaScriptCore -I../../../Source/JavaScriptCore/wtf -I../../../Source/JavaScriptCore/wtf/gobject -I../../../Source/JavaScriptCore/wtf/qt -I../../../Source/JavaScriptCore/wtf/unicode -I/usr/local/Trolltech/qt-mobility-opensource-1.2.0-for-Qt-4.8.0/include -I/usr/local/Trolltech/qt-mobility-opensource-1.2.0-for-Qt-4.8.0/include/QtMobility -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/X11R6/include -I../../../Source -I. ../../../Source/WebKit/qt/WebCoreSupport/InspectorClientQt.cpp -o InspectorClientQt.moc ICECC[20912] 20:17:39: Compiled on 10.109.213.1 make[3]: *** [obj/release/GeolocationPermissionClientQt.o] Error 1 make[3]: *** Waiting for unfinished jobs.... ../../../Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp: In static member function 'static void DumpRenderTreeSupportQt::resetGeolocationMock(QWebPage*)': ../../../Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:840: error: 'class WebCore::Page' has no member named 'geolocationController' ../../../Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp: In static member function 'static void DumpRenderTreeSupportQt::setMockGeolocationPermission(QWebPage*, bool)': ../../../Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:847: error: 'class WebCore::Page' has no member named 'geolocationController' ../../../Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp: In static member function 'static void DumpRenderTreeSupportQt::setMockGeolocationPosition(QWebPage*, double, double, double)': ../../../Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:854: error: 'class WebCore::Page' has no member named 'geolocationController' ../../../Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp: In static member function 'static void DumpRenderTreeSupportQt::setMockGeolocationError(QWebPage*, int, const QString&)': ../../../Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:872: error: 'class WebCore::Page' has no member named 'geolocationController' ../../../Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp: In static member function 'static int DumpRenderTreeSupportQt::numberOfPendingGeolocationPermissionRequests(QWebPage*)': ../../../Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:879: error: 'class WebCore::Page' has no member named 'geolocationController' ICECC[20881] 20:17:41: Compiled on 10.109.211.1 make[3]: *** [obj/release/DumpRenderTreeSupportQt.o] Error 1 make[3]: Leaving directory `/ramdisk/qt-linux-release-minimal/build/WebKitBuild/Release/Source' make[2]: *** [sub-api-pri-make_default-ordered] Error 2 make[2]: Leaving directory `/ramdisk/qt-linux-release-minimal/build/WebKitBuild/Release/Source' make[1]: *** [sub-Source-QtWebKit-pro-make_default-ordered] Error 2 make[1]: Leaving directory `/ramdisk/qt-linux-release-minimal/build/WebKitBuild/Release' make: *** [incremental] Error 2

(In reply to comment #34) > Committed r110595: <http://trac.webkit.org/changeset/110595> And trivial, speculative, ... fixes landed in: - http://trac.webkit.org/changeset/110604 - http://trac.webkit.org/changeset/110609 - http://trac.webkit.org/changeset/110610 They fixed the minimal build, but break all other Qt builds ... Great ... Reopen to fix it.

Last fix landed in http://trac.webkit.org/changeset/110677

*** Bug 40373 has been marked as a duplicate of this bug. ***