Bug 78853

Summary: Always enable ENABLE(CLIENT_BASED_GEOLOCATION)
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, bw80.lee, eric, gustavo.noronha, gustavo, gyuyoung.kim, japhet, jknotten, jonlee, ossy, pnormand, rakuco, ryuan.choi, steveblock, tonikitoo, vestbo, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 79270    
Bug Blocks: 40373    
Attachments:
Description Flags
Patch
none
Patch
pnormand: commit-queue-
Patch
webkit-ews: commit-queue-
Patch
gyuyoung.kim: commit-queue-
Patch
gustavo: commit-queue-
Patch
gustavo.noronha: commit-queue-
Patch abarth: review+

Description Adam Barth 2012-02-16 15:44:53 PST
Always enable ENABLE(CLIENT_BASED_GEOLOCATION)
Comment 1 Adam Barth 2012-02-16 16:00:21 PST
Created attachment 127459 [details]
Patch
Comment 2 Adam Barth 2012-02-16 16:02:45 PST
There's actually some more code to remove related to GeolocationService.cpp, but we can do that in a followup patch.
Comment 3 Eric Seidel (no email) 2012-02-16 16:04:15 PST
Comment on attachment 127459 [details]
Patch

I wonder if any of these ENABLEs you're removing need to be replaced by ENABLE(GEOLOCATION) instead.
Comment 4 Eric Seidel (no email) 2012-02-16 16:04:28 PST
I recommend waiting for the EWS bots to clear.
Comment 5 Adam Barth 2012-02-16 16:06:59 PST
Created attachment 127462 [details]
Patch
Comment 6 Benjamin Poulain 2012-02-16 16:10:50 PST
We still use that code, please wait a bit before removing it.
Comment 7 Adam Barth 2012-02-16 16:12:35 PST
> We still use that code, please wait a bit before removing it.

Which port uses the code?
Comment 8 Philippe Normand 2012-02-16 16:23:19 PST
Comment on attachment 127462 [details]
Patch

Attachment 127462 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11493019
Comment 9 Antonio Gomes 2012-02-16 16:34:08 PST
(In reply to comment #7)
> > We still use that code, please wait a bit before removing it.
> 
> Which port uses the code?

iOS?
Comment 10 Benjamin Poulain 2012-02-16 16:45:47 PST
(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 11 Early Warning System Bot 2012-02-16 17:05:47 PST
Comment on attachment 127462 [details]
Patch

Attachment 127462 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11534984
Comment 12 Adam Barth 2012-02-16 17:24:21 PST
I replied to Benjamin on webkit-dev.
Comment 13 Adam Barth 2012-02-16 17:24:38 PST
Comment on attachment 127462 [details]
Patch

I need to resolve these compile issues.
Comment 14 Raphael Kubo da Costa (:rakuco) 2012-02-17 03:12:24 PST
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.
Comment 15 Raphael Kubo da Costa (:rakuco) 2012-02-17 03:14:59 PST
I wonder if build-webkit doesn't need to be updated as well.
Comment 16 Steve Block 2012-02-17 03:15:39 PST
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
Comment 17 Steve Block 2012-02-17 03:17:11 PST
> 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 18 Gyuyoung Kim 2012-02-17 06:51:39 PST
Comment on attachment 127462 [details]
Patch

Attachment 127462 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11536556
Comment 19 Gyuyoung Kim 2012-02-20 22:31:08 PST
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() {}
Comment 20 Benjamin Poulain 2012-02-20 23:51:20 PST
(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.
Comment 21 Raphael Kubo da Costa (:rakuco) 2012-02-22 13:08:05 PST
(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.
Comment 22 Benjamin Poulain 2012-03-09 17:33:53 PST
Created attachment 131142 [details]
Patch
Comment 23 Early Warning System Bot 2012-03-09 18:01:46 PST
Comment on attachment 131142 [details]
Patch

Attachment 131142 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11911870
Comment 24 Gyuyoung Kim 2012-03-09 20:09:16 PST
Comment on attachment 131142 [details]
Patch

Attachment 131142 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11933014
Comment 25 Benjamin Poulain 2012-03-09 20:42:40 PST
Created attachment 131149 [details]
Patch
Comment 26 Gyuyoung Kim 2012-03-09 21:21:36 PST
Comment on attachment 131149 [details]
Patch

Attachment 131149 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11933039
Comment 27 Benjamin Poulain 2012-03-12 18:21:06 PDT
Created attachment 131482 [details]
Patch
Comment 28 Gustavo Noronha (kov) 2012-03-12 20:40:30 PDT
Comment on attachment 131482 [details]
Patch

Attachment 131482 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11947303
Comment 29 Benjamin Poulain 2012-03-12 21:55:08 PDT
Created attachment 131541 [details]
Patch
Comment 30 Collabora GTK+ EWS bot 2012-03-12 22:29:02 PDT
Comment on attachment 131541 [details]
Patch

Attachment 131541 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11949258
Comment 31 Benjamin Poulain 2012-03-12 23:50:08 PDT
Created attachment 131557 [details]
Patch

Hopefully gtk will build this time :(
Comment 32 Adam Barth 2012-03-12 23:59:05 PDT
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.  :)
Comment 33 Raphael Kubo da Costa (:rakuco) 2012-03-13 04:59:08 PDT
(In reply to comment #32)
> Looks great.   Is GeolocationServiceEFL gone already?

Yes.
Comment 34 Benjamin Poulain 2012-03-13 13:10:48 PDT
Committed r110595: <http://trac.webkit.org/changeset/110595>
Comment 35 Antonio Gomes 2012-03-13 13:48:13 PDT
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
Comment 36 Csaba Osztrogonác 2012-03-14 00:44:42 PDT
(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.
Comment 37 Csaba Osztrogonác 2012-03-14 01:56:03 PDT
Last fix landed in http://trac.webkit.org/changeset/110677
Comment 38 Steve Block 2012-05-21 10:17:26 PDT
*** Bug 40373 has been marked as a duplicate of this bug. ***