[webkit-reviews] review denied: [Bug 82228] GEOLOCATION should be implemented as Page Supplement : [Attachment 133859] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Mark Pilgrim (Google)
<pilgrim at chromium.org>'s request for review:
Bug 82228: GEOLOCATION should be implemented as Page Supplement
https://bugs.webkit.org/show_bug.cgi?id=82228

Attachment 133859: Patch
https://bugs.webkit.org/attachment.cgi?id=133859&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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