Bug 82228

Created attachment 133859 [details] Patch

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

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

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

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

Created attachment 133869 [details] Patch

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

(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 on attachment 133869 [details] Patch Attachment 133869 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12134978

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

> 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 on attachment 133869 [details] Patch Attachment 133869 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12143033

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

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

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

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

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

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

Created attachment 134305 [details] Patch

Shooting for a green GTK bot on this patch.

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

Created attachment 134308 [details] Patch

Created attachment 134314 [details] Patch

Shooting for all bots green on this patch.

Created attachment 134320 [details] Patch

New spin from ToT.

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 on attachment 134320 [details] Patch Attachment 134320 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12155237

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

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

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

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

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

Created attachment 134370 [details] Patch

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.

(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 on attachment 134370 [details] Patch Attachment 134370 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12157232

Created attachment 134387 [details] Patch

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

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 on attachment 134387 [details] Patch Attachment 134387 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12155380

Created attachment 134480 [details] Patch

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

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 on attachment 134480 [details] Patch Attachment 134480 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12203008

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

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

Created attachment 134566 [details] Patch

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

(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 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 on attachment 134566 [details] Patch Clearing flags on attachment: 134566 Committed r112553: <http://trac.webkit.org/changeset/112553>

All reviewed patches have been landed. Closing bug.

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)

(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

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

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

Created attachment 134713 [details] Patch

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

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

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

Comment on attachment 134713 [details] Patch Clearing flags on attachment: 134713 Committed r112720: <http://trac.webkit.org/changeset/112720>

All reviewed patches have been landed. Closing bug.

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

Followup bug 82897 to move GeolocationClient.h.

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.