Bug 52078

Summary: Make Geolocation features work in WebKit2
Product: WebKit Reporter: Sam Weinig <sam>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, ossy, webkit-ews, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 52191, 52193    
Bug Blocks:    
Attachments:
Description Flags
Patch WIP (Do not review)
none
Attempt 2 (DO NOT REVIEW)
none
Step 1
none
Step 2 none

Description Sam Weinig 2011-01-07 13:18:21 PST 
Make Geolocation features work in WebKit2
Comment 1 Sam Weinig 2011-01-07 13:18:34 PST 
<rdar://problem/8502929>
Comment 2 Sam Weinig 2011-01-07 13:19:41 PST 
Created attachment 78263 [details]
Patch WIP (Do not review)
Comment 3 WebKit Review Bot 2011-01-07 13:50:57 PST 
Attachment 78263 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit2/ChangeLog', u'WebKit2/DerivedSources.make', u'WebKit2/DerivedSources.pro', u'WebKit2/Platform/CoreIPC/HandleMessage.h', u'WebKit2/Platform/CoreIPC/MessageID.h', u'WebKit2/Shared/API/c/WKBase.h', u'WebKit2/Shared/APIObject.h', u'WebKit2/Shared/WebGeolocationPosition.cpp', u'WebKit2/Shared/WebGeolocationPosition.h', u'WebKit2/UIProcess/API/C/WKAPICast.h', u'WebKit2/UIProcess/API/C/WKContext.cpp', u'WebKit2/UIProcess/API/C/WKContext.h', u'WebKit2/UIProcess/API/C/WKGeolocationManager.cpp', u'WebKit2/UIProcess/API/C/WKGeolocationManager.h', u'WebKit2/UIProcess/API/C/WKGeolocationPosition.cpp', u'WebKit2/UIProcess/API/C/WKGeolocationPosition.h', u'WebKit2/UIProcess/API/C/WebKit2.h', u'WebKit2/UIProcess/WebContext.cpp', u'WebKit2/UIProcess/WebContext.h', u'WebKit2/UIProcess/WebGeolocationManagerProxy.cpp', u'WebKit2/UIProcess/WebGeolocationManagerProxy.h', u'WebKit2/UIProcess/WebGeolocationManagerProxy.messages.in', u'WebKit2/UIProcess/WebGeolocationProvider.cpp', u'WebKit2/UIProcess/WebGeolocationProvider.h', u'WebKit2/UIProcess/WebProcessProxy.cpp', u'WebKit2/WebKit2.pro', u'WebKit2/WebKit2.xcodeproj/project.pbxproj', u'WebKit2/WebProcess/Geolocation/WebGeolocationManager.cpp', u'WebKit2/WebProcess/Geolocation/WebGeolocationManager.h', u'WebKit2/WebProcess/Geolocation/WebGeolocationManager.messages.in', u'WebKit2/WebProcess/WebCoreSupport/WebGeolocationClient.cpp', u'WebKit2/WebProcess/WebCoreSupport/WebGeolocationClient.h', u'WebKit2/WebProcess/WebPage/WebPage.cpp', u'WebKit2/WebProcess/WebProcess.cpp', u'WebKit2/WebProcess/WebProcess.h', u'WebKit2/win/WebKit2.vcproj']" exit_code: 1
WebKit2/WebProcess/Geolocation/WebGeolocationManager.h:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit2/UIProcess/WebGeolocationManagerProxy.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Early Warning System Bot 2011-01-07 14:26:36 PST 
Attachment 78263 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7424021
Comment 5 Sam Weinig 2011-01-07 15:00:59 PST 
Created attachment 78276 [details]
Attempt 2 (DO NOT REVIEW)
Comment 6 WebKit Review Bot 2011-01-07 15:04:15 PST 
Attachment 78276 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit2/ChangeLog', u'WebKit2/DerivedSources.make', u'WebKit2/DerivedSources.pro', u'WebKit2/Platform/CoreIPC/HandleMessage.h', u'WebKit2/Platform/CoreIPC/MessageID.h', u'WebKit2/Shared/API/c/WKBase.h', u'WebKit2/Shared/APIObject.h', u'WebKit2/Shared/WebGeolocationPosition.cpp', u'WebKit2/Shared/WebGeolocationPosition.h', u'WebKit2/UIProcess/API/C/WKAPICast.h', u'WebKit2/UIProcess/API/C/WKContext.cpp', u'WebKit2/UIProcess/API/C/WKContext.h', u'WebKit2/UIProcess/API/C/WKGeolocationManager.cpp', u'WebKit2/UIProcess/API/C/WKGeolocationManager.h', u'WebKit2/UIProcess/API/C/WKGeolocationPosition.cpp', u'WebKit2/UIProcess/API/C/WKGeolocationPosition.h', u'WebKit2/UIProcess/API/C/WebKit2.h', u'WebKit2/UIProcess/WebContext.cpp', u'WebKit2/UIProcess/WebContext.h', u'WebKit2/UIProcess/WebGeolocationManagerProxy.cpp', u'WebKit2/UIProcess/WebGeolocationManagerProxy.h', u'WebKit2/UIProcess/WebGeolocationManagerProxy.messages.in', u'WebKit2/UIProcess/WebGeolocationProvider.cpp', u'WebKit2/UIProcess/WebGeolocationProvider.h', u'WebKit2/UIProcess/WebProcessProxy.cpp', u'WebKit2/WebKit2.pro', u'WebKit2/WebKit2.xcodeproj/project.pbxproj', u'WebKit2/WebProcess/Geolocation/WebGeolocationManager.cpp', u'WebKit2/WebProcess/Geolocation/WebGeolocationManager.h', u'WebKit2/WebProcess/Geolocation/WebGeolocationManager.messages.in', u'WebKit2/WebProcess/WebCoreSupport/WebGeolocationClient.cpp', u'WebKit2/WebProcess/WebCoreSupport/WebGeolocationClient.h', u'WebKit2/WebProcess/WebPage/WebPage.cpp', u'WebKit2/WebProcess/WebProcess.cpp', u'WebKit2/WebProcess/WebProcess.h', u'WebKit2/win/WebKit2.vcproj']" exit_code: 1
WebKit2/WebProcess/Geolocation/WebGeolocationManager.h:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit2/UIProcess/WebGeolocationManagerProxy.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Early Warning System Bot 2011-01-07 16:48:16 PST 
Attachment 78276 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7405044
Comment 8 Sam Weinig 2011-01-07 17:21:18 PST 
Created attachment 78292 [details]
Step 1
Comment 9 Anders Carlsson 2011-01-10 11:15:47 PST 
Comment on attachment 78292 [details]
Step 1

View in context: https://bugs.webkit.org/attachment.cgi?id=78292&action=review

> WebKit2/WebProcess/Geolocation/WebGeolocationManager.h:47
> +    WebGeolocationManager(WebProcess*);

Please make this explicit.

> WebKit2/WebProcess/WebCoreSupport/WebGeolocationClient.cpp:55
> +{

Should this be implemented?

> WebKit2/WebProcess/WebCoreSupport/WebGeolocationClient.cpp:64
> +{

Should this be implemented?

> WebKit2/WebProcess/WebCoreSupport/WebGeolocationClient.cpp:68
> +{

Should this be implemented?
Comment 10 Sam Weinig 2011-01-10 11:20:58 PST 
Part 1 landed in http://trac.webkit.org/changeset/75395.
Comment 11 Sam Weinig 2011-01-10 17:49:35 PST 
Created attachment 78480 [details]
Step 2
Comment 12 Anders Carlsson 2011-01-10 17:54:52 PST 
Comment on attachment 78480 [details]
Step 2

View in context: https://bugs.webkit.org/attachment.cgi?id=78480&action=review

> WebKit2/UIProcess/GeolocationPermissionRequestManagerProxy.cpp:43
> +    for (; it != end; ++it)

This should read

for (PendingRequestMap::const_iterator it = m_pendingRequests.begin(), end = m_pendingRequests.end(); it != end; ++it)

> WebKit2/WebProcess/Geolocation/GeolocationPermissionRequestManager.cpp:73
> +    m_idToGeolocationMap.remove(it->second);

Is this allowed? I thought it would be invalidated by the first call to .remove
Comment 13 Sam Weinig 2011-01-10 18:15:45 PST 
Step 2 landed in http://trac.webkit.org/changeset/75460.
Comment 14 Early Warning System Bot 2011-01-10 18:28:15 PST 
Attachment 78480 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7472042
Comment 15 WebKit Review Bot 2011-01-10 18:46:07 PST 
http://trac.webkit.org/changeset/75460 might have broken SnowLeopard Intel Release (Build)
Comment 16 Csaba Osztrogonác 2011-01-10 20:43:30 PST 
Reopen, because it was rolled out: http://trac.webkit.org/changeset/75474
Comment 17 Csaba Osztrogonác 2011-01-10 20:55:06 PST 
And the unreviewed build fix was rolled out too: http://trac.webkit.org/changeset/75475
Comment 18 Csaba Osztrogonác 2011-01-11 00:27:49 PST 
#webkit:
<weinig> It is not clear why the geolocation patch I landed today was rolled out
<weinig> The bug doesn't mention what broke
<weinig> and the buildbot seems to indicate that the issue was with drawing area related code, though I may be missing where there was geolocation related code

Qt EWS mentioned what the problem is in Comment #14
Comment 19 Csaba Osztrogonác 2011-01-11 02:40:22 PST 
Comment on attachment 78480 [details]
Step 2

View in context: https://bugs.webkit.org/attachment.cgi?id=78480&action=review

> WebKit2/WebProcess/Geolocation/WebGeolocationManager.h:42
> +namespace WebCore {
> +class Geolocation;
> +}

These lines caused the build fail. If you remove them, and 
add #include <WebCore/Geolocation.h> , Qt build will happy.
Comment 20 Csaba Osztrogonác 2011-01-11 09:08:48 PST 
Comment on attachment 78480 [details]
Step 2

remove r+ from rolled out patch
Comment 21 Sam Weinig 2011-01-11 09:56:48 PST 
(In reply to comment #18)
> #webkit:
> <weinig> It is not clear why the geolocation patch I landed today was rolled out
> <weinig> The bug doesn't mention what broke
> <weinig> and the buildbot seems to indicate that the issue was with drawing area related code, though I may be missing where there was geolocation related code
> 
> Qt EWS mentioned what the problem is in Comment #14

Thanks, I couldn't find that on the buildbot output for some reason.
Comment 22 Sam Weinig 2011-01-11 09:57:09 PST 
Rolled back in, with a slightly different fix in http://trac.webkit.org/changeset/75508.