Bug 52078 - Make Geolocation features work in WebKit2
Summary: Make Geolocation features work in WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 52191 52193
Blocks:
  Show dependency tree / graph
 
Reported: 2011-01-07 13:18 PST by Sam Weinig
Modified: 2011-01-11 09:57 PST (History)
5 users (show)

See Also:

Attachments
Patch WIP (Do not review) (89.45 KB, patch)
2011-01-07 13:19 PST, Sam Weinig 		no flags Details | Formatted Diff | Diff
Attempt 2 (DO NOT REVIEW) (89.89 KB, patch)
2011-01-07 15:00 PST, Sam Weinig 		no flags Details | Formatted Diff | Diff
Step 1 (95.95 KB, patch)
2011-01-07 17:21 PST, Sam Weinig 		no flags Details | Formatted Diff | Diff
Step 2 (55.01 KB, patch)
2011-01-10 17:49 PST, Sam Weinig 		no flags Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.