[webkit-reviews] review denied: [Bug 39879] Geolocation activity started after frame has been disconnected can cause crash : [Attachment 58137] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied Steve Block
<steveblock at google.com>'s request for review:
Bug 39879: Geolocation activity started after frame has been disconnected can
cause crash
https://bugs.webkit.org/show_bug.cgi?id=39879

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

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
WebCore/page/Geolocation.cpp: 
 +	// This method is called at most once on a given GeoNotifier object.
Why is this getting removed?  What happens if the timer has already started for
another error?	Etc...	This probably deserves mention in the change log.



WebCore/page/Geolocation.cpp:500
 +	    RefPtr<GeoNotifier> notifier = *it;
Why do you need to do this?  It doesn't seem like it saves any space.  I guess
it's slightly more readable....

WebCore/page/Geolocation.cpp:508
 +	copyToVector(m_oneShots, copy);
Do you need to null out the list?

WebCore/page/Geolocation.cpp:510
 +	m_watchers.getNotifiersVector(copy);
This shouldn't have had get in the name...

WebCore/page/Geolocation.h:138
 +	void cancelAllRequests(Vector<RefPtr<GeoNotifier> >&);
Why name this the same thing and make it public?  It seems like just a helper
function.

WebCore/page/Geolocation.cpp:50
 +  static const char serviceNoLongerAvailableErrorMessage[] = "Geolocation
service no longer available";
Are these messages supposed to not have a period at the end?