Bug 78853 - Always enable ENABLE(CLIENT_BASED_GEOLOCATION)
: Always enable ENABLE(CLIENT_BASED_GEOLOCATION)
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 79270
: 40373
  Show dependency treegraph
 
Reported: 2012-02-16 15:44 PST by
Modified: 2012-05-21 10:17 PST (History)


Attachments
Patch (91.72 KB, patch)
2012-02-16 16:00 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Patch (91.73 KB, patch)
2012-02-16 16:06 PST, Adam Barth
pnormand: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch (124.06 KB, patch)
2012-03-09 17:33 PST, Benjamin Poulain
webkit-ews: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch (124.71 KB, patch)
2012-03-09 20:42 PST, Benjamin Poulain
gyuyoung.kim: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch (127.16 KB, patch)
2012-03-12 18:21 PST, Benjamin Poulain
gns: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch (127.45 KB, patch)
2012-03-12 21:55 PST, Benjamin Poulain
gustavo.noronha: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch (127.42 KB, patch)
2012-03-12 23:50 PST, Benjamin Poulain
abarth: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-02-16 15:44:53 PST
Always enable ENABLE(CLIENT_BASED_GEOLOCATION)
------- Comment #1 From 2012-02-16 16:00:21 PST -------
Created an attachment (id=127459) [details]
Patch
------- Comment #2 From 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 From 2012-02-16 16:04:15 PST -------
(From update of attachment 127459 [details])
I wonder if any of these ENABLEs you're removing need to be replaced by ENABLE(GEOLOCATION) instead.
------- Comment #4 From 2012-02-16 16:04:28 PST -------
I recommend waiting for the EWS bots to clear.
------- Comment #5 From 2012-02-16 16:06:59 PST -------
Created an attachment (id=127462) [details]
Patch
------- Comment #6 From 2012-02-16 16:10:50 PST -------
We still use that code, please wait a bit before removing it.
------- Comment #7 From 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 From 2012-02-16 16:23:19 PST -------
(From update of attachment 127462 [details])
Attachment 127462 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11493019
------- Comment #9 From 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 From 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 From 2012-02-16 17:05:47 PST -------
(From update of attachment 127462 [details])
Attachment 127462 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11534984
------- Comment #12 From 2012-02-16 17:24:21 PST -------
I replied to Benjamin on webkit-dev.
------- Comment #13 From 2012-02-16 17:24:38 PST -------
(From update of attachment 127462 [details])
I need to resolve these compile issues.
------- Comment #14 From 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 From 2012-02-17 03:14:59 PST -------
I wonder if build-webkit doesn't need to be updated as well.
------- Comment #16 From 2012-02-17 03:15:39 PST -------
(From update of attachment 127462 [details])
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 From 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 From 2012-02-17 06:51:39 PST -------
(From update of attachment 127462 [details])
Attachment 127462 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11536556
------- Comment #19 From 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 From 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 From 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 From 2012-03-09 17:33:53 PST -------
Created an attachment (id=131142) [details]
Patch
------- Comment #23 From 2012-03-09 18:01:46 PST -------
(From update of attachment 131142 [details])
Attachment 131142 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11911870
------- Comment #24 From 2012-03-09 20:09:16 PST -------
(From update of attachment 131142 [details])
Attachment 131142 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11933014
------- Comment #25 From 2012-03-09 20:42:40 PST -------
Created an attachment (id=131149) [details]
Patch
------- Comment #26 From 2012-03-09 21:21:36 PST -------
(From update of attachment 131149 [details])
Attachment 131149 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11933039
------- Comment #27 From 2012-03-12 18:21:06 PST -------
Created an attachment (id=131482) [details]
Patch
------- Comment #28 From 2012-03-12 20:40:30 PST -------
(From update of attachment 131482 [details])
Attachment 131482 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11947303
------- Comment #29 From 2012-03-12 21:55:08 PST -------
Created an attachment (id=131541) [details]
Patch
------- Comment #30 From 2012-03-12 22:29:02 PST -------
(From update of attachment 131541 [details])
Attachment 131541 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11949258
------- Comment #31 From 2012-03-12 23:50:08 PST -------
Created an attachment (id=131557) [details]
Patch

Hopefully gtk will build this time :(
------- Comment #32 From 2012-03-12 23:59:05 PST -------
(From update of attachment 131557 [details])
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 From 2012-03-13 04:59:08 PST -------
(In reply to comment #32)
> Looks great.   Is GeolocationServiceEFL gone already?

Yes.
------- Comment #34 From 2012-03-13 13:10:48 PST -------
Committed r110595: <http://trac.webkit.org/changeset/110595>
------- Comment #35 From 2012-03-13 13:48:13 PST -------
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 From 2012-03-14 00:44:42 PST -------
(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 From 2012-03-14 01:56:03 PST -------
Last fix landed in http://trac.webkit.org/changeset/110677
------- Comment #38 From 2012-05-21 10:17:26 PST -------
*** Bug 40373 has been marked as a duplicate of this bug. ***