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

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted 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 134480: Patch
https://bugs.webkit.org/attachment.cgi?id=134480&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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)->setControll
er(m_page->geolocationController());
> +	  
static_cast<GeolocationClientMock*>(pageClients.geolocationClient)->setControll
er(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.