[webkit-reviews] review denied: [Bug 89365] Web Inspector: Geolocation override : [Attachment 153571] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Konrad Piascik
<kpiascik at rim.com>'s request for review:
Bug 89365: Web Inspector: Geolocation override
https://bugs.webkit.org/show_bug.cgi?id=89365

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=153571&action=review


> Source/WebCore/inspector/InspectorPageAgent.cpp:1003
> +    if (controller && m_platformGeolocationPosition.get())

Nit: if (controller && m_platformGeolocationPosition)

> Source/WebCore/inspector/InspectorPageAgent.cpp:1004
> +	  
controller->positionChanged(m_platformGeolocationPosition.release().leakRef());


Does this actually leak the pointer? What is the contract of positionChanged?
Should it adopt the pointer or does it receive refptr? Migrating from raw
pointer to PassRefPtr would really help understanding there.

> Source/WebCore/inspector/InspectorPageAgent.cpp:1023
> +	   if (position && m_geolocationPosition.get() && position !=
m_geolocationPosition.get())

If m_geolocationPosition is 0 when you start emulation, you don't have a chance
to assign to it here. I guess the check should be if (position) only.

> Source/WebCore/inspector/front-end/SettingsScreen.js:665
> +	   checkboxElement.checked = !geolocation || (geolocation.latitude &&
geolocation.longitude);

As I stated previously, this will make geolocation enabled by default (upon the
first settings pane opening). Please fix this.