Bug 52078 – Make Geolocation features work in WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Sam Weinig]

Make Geolocation features work in WebKit2

[Sam Weinig]

<rdar://problem/8502929>

[Sam Weinig]

Created attachment 78263 [details]
Patch WIP (Do not review)

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 78263 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7424021

[Sam Weinig]

Created attachment 78276 [details]
Attempt 2 (DO NOT REVIEW)

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 78276 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7405044

[Sam Weinig]

Created attachment 78292 [details]
Step 1

[Anders Carlsson]

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?

[Sam Weinig]

Part 1 landed in http://trac.webkit.org/changeset/75395.

[Sam Weinig]

Created attachment 78480 [details]
Step 2

[Anders Carlsson]

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

[Sam Weinig]

Step 2 landed in http://trac.webkit.org/changeset/75460.

[Early Warning System Bot]

Attachment 78480 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7472042

[WebKit Review Bot]

http://trac.webkit.org/changeset/75460 might have broken SnowLeopard Intel Release (Build)

[Csaba Osztrogonác]

Reopen, because it was rolled out: http://trac.webkit.org/changeset/75474

[Csaba Osztrogonác]

And the unreviewed build fix was rolled out too: http://trac.webkit.org/changeset/75475

[Csaba Osztrogonác]

#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

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Comment on attachment 78480 [details]
Step 2

remove r+ from rolled out patch

[Sam Weinig]

(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.

[Sam Weinig]

Rolled back in, with a slightly different fix in http://trac.webkit.org/changeset/75508.