Bug 82228 – GEOLOCATION should be implemented as Page Supplement

• Component: New Bugs
• Status: RESOLVED FIXED

[Mark Pilgrim (Google)]

GEOLOCATION should be implemented as Page Supplement

[Mark Pilgrim (Google)]

Created attachment 133859 [details]
Patch

[Philippe Normand]

Comment on attachment 133859 [details]
Patch

Attachment 133859 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12142007

[Build Bot]

Comment on attachment 133859 [details]
Patch

Attachment 133859 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12134966

[Build Bot]

Comment on attachment 133859 [details]
Patch

Attachment 133859 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12132929

[Adam Barth]

Comment on attachment 133859 [details]
Patch

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

Thanks for the patch.  This looks like a good start.

> Source/WebCore/Modules/geolocation/Geolocation.h:78
> +    GeolocationController* m_geolocationController;

Generally we put all the member variables together at the bottom of the class declaration.  Do we need to worry about this object being deallocated?  Maybe it would be better to call GeolocationController::from(page) each time we need to grab the controller?

> Source/WebCore/page/GeolocationClient.h:32
> +class GeolocationController;

Why do you need to add this forward declaration?

> Source/WebKit/chromium/src/WebViewImpl.cpp:403
> +    provideGeolocationTo(m_page.get(), m_geolocationClientProxy.get());

Presumably we need to do this for all the other ports in Source/WebKit and also for Source/WebKit2

[Mark Pilgrim (Google)]

Created attachment 133869 [details]
Patch

[Mark Pilgrim (Google)]

New patch fixes two nits mentioned by Adam and hopefully fixes Windows build.

[Mark Pilgrim (Google)]

(In reply to comment #5)
> Maybe it would be better to call GeolocationController::from(page) each time we need to grab the controller?

OK, will do this in the next patch.

[Gustavo Noronha (kov)]

Comment on attachment 133869 [details]
Patch

Attachment 133869 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12134978

[Early Warning System Bot]

Comment on attachment 133869 [details]
Patch

Attachment 133869 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12141054

[Adam Barth]

> OK, will do this in the next patch.

We don't want to land patches with memory errors.  If this patch is going to cause us to dereference pointers to deallocated memory, we'll need to fix that before landing it.

[Early Warning System Bot]

Comment on attachment 133869 [details]
Patch

Attachment 133869 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12143033

[Build Bot]

Comment on attachment 133869 [details]
Patch

Attachment 133869 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12143030

[Build Bot]

Comment on attachment 133869 [details]
Patch

Attachment 133869 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12142038

[Mark Pilgrim (Google)]

(In reply to comment #11)
> > OK, will do this in the next patch.
> 
> We don't want to land patches with memory errors.  If this patch is going to cause us to dereference pointers to deallocated memory, we'll need to fix that before landing it.

Sorry, I meant the next round of patches in this bug. I'm nowhere near getting it to build on all platforms yet.

[Adam Barth]

*** Bug 82271 has been marked as a duplicate of this bug. ***

[WebKit Review Bot]

Comment on attachment 133869 [details]
Patch

Attachment 133869 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12146255

New failing tests:
fast/dom/Geolocation/success.html
fast/dom/Geolocation/reentrant-error.html
accessibility/aria-disabled.html
fast/dom/Geolocation/error.html
http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect.html
fast/dom/Geolocation/callback-to-deleted-context.html
fast/dom/Geolocation/delayed-permission-allowed.html
fast/dom/Geolocation/callback-to-remote-context2.html
fast/dom/Geolocation/position-string.html
fast/loader/text-document-wrapping.html
fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests.html
fast/dom/Geolocation/multiple-requests.html
http/tests/inspector/inspect-element.html
fast/dom/Geolocation/watch.html
fast/dom/Geolocation/window-close-crash.html
fast/dom/Geolocation/timeout.html
fast/dom/Geolocation/timestamp.html
fast/dom/Geolocation/callback-exception.html
fast/dom/Geolocation/maximum-age.html
fast/dom/Geolocation/reentrant-success.html
fast/dom/Geolocation/callback-to-remote-context.html

[WebKit Review Bot]

Created attachment 133968 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Mark Pilgrim (Google)]

Created attachment 134305 [details]
Patch

[Mark Pilgrim (Google)]

Shooting for a green GTK bot on this patch.

[Mark Pilgrim (Google)]

(In reply to comment #17)
> (From update of attachment 133869 [details])
> Attachment 133869 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12146255
> 
> New failing tests:

Is this flake? I can't reproduce any of these failures locally.

[Mark Pilgrim (Google)]

Created attachment 134308 [details]
Patch

[Mark Pilgrim (Google)]

Created attachment 134314 [details]
Patch

[Mark Pilgrim (Google)]

Shooting for all bots green on this patch.

[Mark Pilgrim (Google)]

Created attachment 134320 [details]
Patch

[Mark Pilgrim (Google)]

New spin from ToT.

[WebKit Review Bot]

Attachment 134320 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/gtk/webkit/webkitwebview.cpp:64:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 134320 [details]
Patch

Attachment 134320 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12155237

[Early Warning System Bot]

Comment on attachment 134320 [details]
Patch

Attachment 134320 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12157146

[Early Warning System Bot]

Comment on attachment 134320 [details]
Patch

Attachment 134320 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12155253

[Build Bot]

Comment on attachment 134320 [details]
Patch

Attachment 134320 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12157161

[WebKit Review Bot]

Comment on attachment 134320 [details]
Patch

Attachment 134320 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12153378

New failing tests:
fast/dom/Geolocation/success.html
fast/dom/Geolocation/reentrant-error.html
accessibility/aria-disabled.html
fast/dom/Geolocation/maximum-age.html
fast/dom/Geolocation/callback-to-deleted-context.html
fast/dom/Geolocation/delayed-permission-allowed.html
fast/frames/lots-of-objects.html
fast/dom/Geolocation/callback-to-remote-context2.html
fast/dom/Geolocation/position-string.html
fast/loader/text-document-wrapping.html
fast/dom/Geolocation/delayed-permission-allowed-for-multiple-requests.html
fast/dom/Geolocation/multiple-requests.html
http/tests/inspector/inspect-element.html
fast/dom/Geolocation/watch.html
fast/dom/Geolocation/window-close-crash.html
fast/dom/Geolocation/timeout.html
fast/dom/Geolocation/timestamp.html
fast/dom/Geolocation/callback-exception.html
fast/dom/Geolocation/error.html
fast/dom/Geolocation/reentrant-success.html
fast/dom/Geolocation/callback-to-remote-context.html

[WebKit Review Bot]

Created attachment 134364 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Mark Pilgrim (Google)]

Created attachment 134370 [details]
Patch

[WebKit Review Bot]

Attachment 134370 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/gtk/webkit/webkitwebview.cpp:64:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Pilgrim (Google)]

(In reply to comment #32)
> (From update of attachment 134320 [details])
> Attachment 134320 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12153378
> 
> New failing tests:

OK, I was able to reproduce these. No fix yet.

[Build Bot]

Comment on attachment 134370 [details]
Patch

Attachment 134370 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12157232

[Mark Pilgrim (Google)]

Created attachment 134387 [details]
Patch

[Mark Pilgrim (Google)]

This patch should fix the win build and the cr-linux crashes.

[WebKit Review Bot]

Attachment 134387 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/gtk/webkit/webkitwebview.cpp:64:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 134387 [details]
Patch

Attachment 134387 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12155380

[Mark Pilgrim (Google)]

Created attachment 134480 [details]
Patch

[Mark Pilgrim (Google)]

This should fix the Mac linker failure, and then we'll be good to go.

[Adam Barth]

Comment on attachment 134480 [details]
Patch

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

This looks great.  One small issue, noted below.

> Source/WebCore/ChangeLog:7
> +        GEOLOCATION should be implemented as Page Supplement
> +        https://bugs.webkit.org/show_bug.cgi?id=82228
> +
> +        Reviewed by NOBODY (OOPS!).
> +

I probably would have added some more information to the ChangeLog about why we're making this change.  For example, you might want to say that these are some of the last references to Geolocation in WebCore proper and that by removing them, we remove a bunch of "cruft" from Page.

> Source/WebKit/blackberry/Api/WebPage.cpp:452
> -        static_cast<GeolocationClientMock*>(pageClients.geolocationClient)->setController(m_page->geolocationController());
> +        static_cast<GeolocationClientMock*>(pageClients.geolocationClient)->setController(GeolocationController::from(m_page));

This line isn't quite right.  We should delete geolocationClient from pageClients.  It's not necessary anymore.  Once you do that, this line will need to change slightly.

[Build Bot]

Comment on attachment 134480 [details]
Patch

Attachment 134480 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12203008

[Mark Pilgrim (Google)]

(In reply to comment #45)
> (From update of attachment 134480 [details])
> Attachment 134480 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/12203008

This is probably something really stupid, but I have no idea what is causing this linking error. Can any Mac experts help debug this?

[Adam Barth]

> This is probably something really stupid, but I have no idea what is causing this linking error. Can any Mac experts help debug this?

You need to export these symbols by adding them to WebCore.exp.in.  Whenever you reference a symbol in WebCore from WebKit/mac, you need to add it to WebCore.exp.in so that it's exported from the WebCore DLL in the apple-mac build.

[Mark Pilgrim (Google)]

Created attachment 134566 [details]
Patch

[Mark Pilgrim (Google)]

(In reply to comment #44)
> (From update of attachment 134480 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134480&action=review

> I probably would have added some more information to the ChangeLog about why we're making this change.  For example, you might want to say that these are some of the last references to Geolocation in WebCore proper and that by removing them, we remove a bunch of "cruft" from Page.

Done.

> > Source/WebKit/blackberry/Api/WebPage.cpp:452
> > -        static_cast<GeolocationClientMock*>(pageClients.geolocationClient)->setController(m_page->geolocationController());
> > +        static_cast<GeolocationClientMock*>(pageClients.geolocationClient)->setController(GeolocationController::from(m_page));
> 
> This line isn't quite right.  We should delete geolocationClient from pageClients.  It's not necessary anymore.  Once you do that, this line will need to change slightly.

Done.

[Mark Pilgrim (Google)]

(In reply to comment #47)
> > This is probably something really stupid, but I have no idea what is causing this linking error. Can any Mac experts help debug this?
> 
> You need to export these symbols by adding them to WebCore.exp.in.  Whenever you reference a symbol in WebCore from WebKit/mac, you need to add it to WebCore.exp.in so that it's exported from the WebCore DLL in the apple-mac build.

Thanks. Done.

[Adam Barth]

Comment on attachment 134566 [details]
Patch

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

Thanks Mark!

> Source/WebCore/Modules/geolocation/GeolocationController.h:32
> +#include "Page.h"

You don't actually need to include page.  It's forward declared below.  You can just include Supplementable.h

[WebKit Review Bot]

Comment on attachment 134566 [details]
Patch

Clearing flags on attachment: 134566

Committed r112553: <http://trac.webkit.org/changeset/112553>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

I'm seeing the following build error on the Lion bots:
<http://build.webkit.org/builders/Lion%20Intel%20Release%20%28Build%29/builds/8187/steps/compile-webkit/logs/stdio>

> Ld WebCore
> Undefined symbols for architecture x86_64:
>   "__ZN7WebCore21GeolocationController4fromEPNS_4PageE", referenced from:
>      -exported_symbol[s_list] command line option
> ld: symbol(s) not found for architecture x86_64
> clang: error: linker command failed with exit code 1 (use -v to see invocation)

[Csaba Osztrogonác]

(In reply to comment #52)
> (From update of attachment 134566 [details])
> Clearing flags on attachment: 134566
> 
> Committed r112553: <http://trac.webkit.org/changeset/112553>

It made all tests crash on Qt WK2:
http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Amazon%20EC2%29/builds/415

[Csaba Osztrogonác]

Rollout landed in http://trac.webkit.org/changeset/112559

[zalan]

m_page is NULL when WebCore::provideGeolocationTo() is called with m_page.get(). Page gets constructed a few lines later.

Index: Source/WebKit2/WebProcess/WebPage/WebPage.cpp
===================================================================
--- Source/WebKit2/WebProcess/WebPage/WebPage.cpp	(revision 112557)
+++ Source/WebKit2/WebProcess/WebPage/WebPage.cpp	(working copy)
@@ -231,15 +231,16 @@
     pageClients.editorClient = new WebEditorClient(this);
     pageClients.dragClient = new WebDragClient(this);
     pageClients.backForwardClient = WebBackForwardListProxy::create(this);
-#if ENABLE(GEOLOCATION)
-    WebCore::provideGeolocationTo(m_page.get(), new WebGeolocationClient(this));
-#endif
 #if ENABLE(INSPECTOR)
     pageClients.inspectorClient = new WebInspectorClient(this);
 #endif
     
     m_page = adoptPtr(new Page(pageClients));
 
+#if ENABLE(GEOLOCATION)
+    WebCore::provideGeolocationTo(m_page.get(), new WebGeolocationClient(this));
+#endif
+
 #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
     WebCore::provideNotification(m_page.get(), new WebNotificationClient(this));
 #endif

[Mark Pilgrim (Google)]

Created attachment 134713 [details]
Patch

[Mark Pilgrim (Google)]

(In reply to comment #57)
> m_page is NULL when WebCore::provideGeolocationTo() is called with m_page.get(). Page gets constructed a few lines later.

Thanks. The latest attachment includes your fix.

[Mark Pilgrim (Google)]

(In reply to comment #54)
> I'm seeing the following build error on the Lion bots:
> <http://build.webkit.org/builders/Lion%20Intel%20Release%20%28Build%29/builds/8187/steps/compile-webkit/logs/stdio>
> 
> > Ld WebCore
> > Undefined symbols for architecture x86_64:
> >   "__ZN7WebCore21GeolocationController4fromEPNS_4PageE", referenced from:
> >      -exported_symbol[s_list] command line option
> > ld: symbol(s) not found for architecture x86_64
> > clang: error: linker command failed with exit code 1 (use -v to see invocation)

The latest attachment should fix this. I had added one too many symbols to WebCore.exp.in, and the automated debug build didn't catch the error. The latest attachment only lists the symbols that are needed.

[Adam Barth]

Mark, let me know when you'd like me to mark this for commit.

[WebKit Review Bot]

Comment on attachment 134713 [details]
Patch

Clearing flags on attachment: 134713

Committed r112720: <http://trac.webkit.org/changeset/112720>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Pilgrim (Google)]

(In reply to comment #51)
> (From update of attachment 134566 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134566&action=review
> 
> Thanks Mark!
> 
> > Source/WebCore/Modules/geolocation/GeolocationController.h:32
> > +#include "Page.h"
> 
> You don't actually need to include page.  It's forward declared below.  You can just include Supplementable.h

Changing "#include Page.h" to "#include Supplementable.h" produces the following compiler error:

Source/WebCore/Modules/geolocation/GeolocationController.h: In static member function ‘static WebCore::GeolocationController* WebCore::GeolocationController::from(WebCore::Page*)’:
Source/WebCore/Modules/geolocation/GeolocationController.h:65:142: error: no matching function for call to ‘WebCore::Supplement<WebCore::Page>::from(WebCore::Page*&, const WTF::AtomicString&)’
Source/WebCore/platform/Supplementable.h:49:27: note: candidate is: static WebCore::Supplement<T>* WebCore::Supplement<T>::from(WebCore::Supplementable<T>*, const WTF::AtomicString&) [with T = WebCore::Page]

[Mark Pilgrim (Google)]

Followup bug 82897 to move GeolocationClient.h.

[Adam Barth]

Comment on attachment 134713 [details]
Patch

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

> Source/WebCore/Modules/geolocation/GeolocationController.h:65
> +    static GeolocationController* from(Page* page) { return static_cast<GeolocationController*>(Supplement<Page>::from(page, supplementName())); }

I see.  You can fix that by moving this function out of line, but I'm not sure it's worth it.