Bug 82228 - GEOLOCATION should be implemented as Page Supplement
Summary: GEOLOCATION should be implemented as Page Supplement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Pilgrim (Google)
URL:
Keywords:
: 82271 (view as bug list)
Depends on: 82638
Blocks: 82266
  Show dependency treegraph
 
Reported: 2012-03-26 12:08 PDT by Mark Pilgrim (Google)
Modified: 2012-04-02 11:29 PDT (History)
12 users (show)

See Also:


Attachments
Patch (11.10 KB, patch)
2012-03-26 12:09 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (12.32 KB, patch)
2012-03-26 12:49 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (4.87 MB, application/zip)
2012-03-26 20:49 PDT, WebKit Review Bot
no flags Details
Patch (22.49 KB, patch)
2012-03-28 08:57 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (24.91 KB, patch)
2012-03-28 09:06 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (38.02 KB, patch)
2012-03-28 09:27 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (38.02 KB, patch)
2012-03-28 10:00 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (3.54 MB, application/zip)
2012-03-28 12:10 PDT, WebKit Review Bot
no flags Details
Patch (38.03 KB, patch)
2012-03-28 12:24 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (38.42 KB, patch)
2012-03-28 13:52 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (38.55 KB, patch)
2012-03-28 19:11 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (42.49 KB, patch)
2012-03-29 06:38 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (42.58 KB, patch)
2012-03-29 19:35 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Pilgrim (Google) 2012-03-26 12:08:04 PDT
GEOLOCATION should be implemented as Page Supplement
Comment 1 Mark Pilgrim (Google) 2012-03-26 12:09:28 PDT
Created attachment 133859 [details]
Patch
Comment 2 Philippe Normand 2012-03-26 12:17:03 PDT
Comment on attachment 133859 [details]
Patch

Attachment 133859 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12142007
Comment 3 Build Bot 2012-03-26 12:24:27 PDT
Comment on attachment 133859 [details]
Patch

Attachment 133859 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12134966
Comment 4 Build Bot 2012-03-26 12:29:12 PDT
Comment on attachment 133859 [details]
Patch

Attachment 133859 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12132929
Comment 5 Adam Barth 2012-03-26 12:36:45 PDT
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
Comment 6 Mark Pilgrim (Google) 2012-03-26 12:49:17 PDT
Created attachment 133869 [details]
Patch
Comment 7 Mark Pilgrim (Google) 2012-03-26 12:50:09 PDT
New patch fixes two nits mentioned by Adam and hopefully fixes Windows build.
Comment 8 Mark Pilgrim (Google) 2012-03-26 12:53:08 PDT
(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.
Comment 9 Gustavo Noronha (kov) 2012-03-26 12:58:24 PDT
Comment on attachment 133869 [details]
Patch

Attachment 133869 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12134978
Comment 10 Early Warning System Bot 2012-03-26 13:13:38 PDT
Comment on attachment 133869 [details]
Patch

Attachment 133869 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12141054
Comment 11 Adam Barth 2012-03-26 13:16:01 PDT
> 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.
Comment 12 Early Warning System Bot 2012-03-26 13:16:40 PDT
Comment on attachment 133869 [details]
Patch

Attachment 133869 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12143033
Comment 13 Build Bot 2012-03-26 13:19:03 PDT
Comment on attachment 133869 [details]
Patch

Attachment 133869 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12143030
Comment 14 Build Bot 2012-03-26 13:28:34 PDT
Comment on attachment 133869 [details]
Patch

Attachment 133869 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12142038
Comment 15 Mark Pilgrim (Google) 2012-03-26 14:01:38 PDT
(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.
Comment 16 Adam Barth 2012-03-26 18:25:36 PDT
*** Bug 82271 has been marked as a duplicate of this bug. ***
Comment 17 WebKit Review Bot 2012-03-26 20:49:41 PDT
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
Comment 18 WebKit Review Bot 2012-03-26 20:49:47 PDT
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
Comment 19 Mark Pilgrim (Google) 2012-03-28 08:57:14 PDT
Created attachment 134305 [details]
Patch
Comment 20 Mark Pilgrim (Google) 2012-03-28 08:57:40 PDT
Shooting for a green GTK bot on this patch.
Comment 21 Mark Pilgrim (Google) 2012-03-28 08:59:20 PDT
(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.
Comment 22 Mark Pilgrim (Google) 2012-03-28 09:06:44 PDT
Created attachment 134308 [details]
Patch
Comment 23 Mark Pilgrim (Google) 2012-03-28 09:27:29 PDT
Created attachment 134314 [details]
Patch
Comment 24 Mark Pilgrim (Google) 2012-03-28 09:28:28 PDT
Shooting for all bots green on this patch.
Comment 25 Mark Pilgrim (Google) 2012-03-28 10:00:26 PDT
Created attachment 134320 [details]
Patch
Comment 26 Mark Pilgrim (Google) 2012-03-28 10:00:48 PDT
New spin from ToT.
Comment 27 WebKit Review Bot 2012-03-28 10:03:21 PDT
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.
Comment 28 Build Bot 2012-03-28 10:17:21 PDT
Comment on attachment 134320 [details]
Patch

Attachment 134320 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12155237
Comment 29 Early Warning System Bot 2012-03-28 10:31:17 PDT
Comment on attachment 134320 [details]
Patch

Attachment 134320 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12157146
Comment 30 Early Warning System Bot 2012-03-28 10:34:35 PDT
Comment on attachment 134320 [details]
Patch

Attachment 134320 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12155253
Comment 31 Build Bot 2012-03-28 11:13:34 PDT
Comment on attachment 134320 [details]
Patch

Attachment 134320 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12157161
Comment 32 WebKit Review Bot 2012-03-28 12:10:22 PDT
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
Comment 33 WebKit Review Bot 2012-03-28 12:10:28 PDT
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
Comment 34 Mark Pilgrim (Google) 2012-03-28 12:24:48 PDT
Created attachment 134370 [details]
Patch
Comment 35 WebKit Review Bot 2012-03-28 12:30:59 PDT
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.
Comment 36 Mark Pilgrim (Google) 2012-03-28 12:49:38 PDT
(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.
Comment 37 Build Bot 2012-03-28 13:04:29 PDT
Comment on attachment 134370 [details]
Patch

Attachment 134370 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12157232
Comment 38 Mark Pilgrim (Google) 2012-03-28 13:52:38 PDT
Created attachment 134387 [details]
Patch
Comment 39 Mark Pilgrim (Google) 2012-03-28 13:53:15 PDT
This patch should fix the win build and the cr-linux crashes.
Comment 40 WebKit Review Bot 2012-03-28 13:55:53 PDT
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.
Comment 41 Build Bot 2012-03-28 14:30:07 PDT
Comment on attachment 134387 [details]
Patch

Attachment 134387 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12155380
Comment 42 Mark Pilgrim (Google) 2012-03-28 19:11:45 PDT
Created attachment 134480 [details]
Patch
Comment 43 Mark Pilgrim (Google) 2012-03-28 19:13:41 PDT
This should fix the Mac linker failure, and then we'll be good to go.
Comment 44 Adam Barth 2012-03-28 19:28:54 PDT
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.
Comment 45 Build Bot 2012-03-28 19:40:21 PDT
Comment on attachment 134480 [details]
Patch

Attachment 134480 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12203008
Comment 46 Mark Pilgrim (Google) 2012-03-28 20:31:37 PDT
(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?
Comment 47 Adam Barth 2012-03-28 21:28:01 PDT
> 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.
Comment 48 Mark Pilgrim (Google) 2012-03-29 06:38:52 PDT
Created attachment 134566 [details]
Patch
Comment 49 Mark Pilgrim (Google) 2012-03-29 06:39:52 PDT
(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.
Comment 50 Mark Pilgrim (Google) 2012-03-29 06:40:08 PDT
(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.
Comment 51 Adam Barth 2012-03-29 10:36:26 PDT
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
Comment 52 WebKit Review Bot 2012-03-29 11:32:49 PDT
Comment on attachment 134566 [details]
Patch

Clearing flags on attachment: 134566

Committed r112553: <http://trac.webkit.org/changeset/112553>
Comment 53 WebKit Review Bot 2012-03-29 11:33:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 54 Joseph Pecoraro 2012-03-29 12:00:54 PDT
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)
Comment 55 Csaba Osztrogonác 2012-03-29 12:05:57 PDT
(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
Comment 56 Csaba Osztrogonác 2012-03-29 12:26:41 PDT
Rollout landed in http://trac.webkit.org/changeset/112559
Comment 57 zalan 2012-03-29 13:47:42 PDT
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
Comment 58 Mark Pilgrim (Google) 2012-03-29 19:35:12 PDT
Created attachment 134713 [details]
Patch
Comment 59 Mark Pilgrim (Google) 2012-03-29 19:36:17 PDT
(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.
Comment 60 Mark Pilgrim (Google) 2012-03-29 19:37:12 PDT
(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.
Comment 61 Adam Barth 2012-03-30 13:00:35 PDT
Mark, let me know when you'd like me to mark this for commit.
Comment 62 WebKit Review Bot 2012-03-30 13:50:27 PDT
Comment on attachment 134713 [details]
Patch

Clearing flags on attachment: 134713

Committed r112720: <http://trac.webkit.org/changeset/112720>
Comment 63 WebKit Review Bot 2012-03-30 13:50:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 64 Mark Pilgrim (Google) 2012-04-02 07:39:31 PDT
(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]
Comment 65 Mark Pilgrim (Google) 2012-04-02 07:39:55 PDT
Followup bug 82897 to move GeolocationClient.h.
Comment 66 Adam Barth 2012-04-02 11:29:28 PDT
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.