[webkit-reviews] review denied: [Bug 89365] Web Inspector: Geolocation override : [Attachment 153330] 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 153330: Patch
https://bugs.webkit.org/attachment.cgi?id=153330&action=review

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


> Source/WebCore/inspector/InspectorPageAgent.cpp:982
> +    if (!latitude && !longitude && !accuracy && controller) {

You should not call errorOccurred from both places (controller and the agent),
just kick the positionChanged here and things will trigger.

> Source/WebCore/inspector/InspectorPageAgent.cpp:999
> +#if ENABLE(GEOLOCATION)

Clearing the override should kick positionChanged with the platform position.
I'll attached my version of InspectorPageAgent to this bug.

> Source/WebCore/inspector/InspectorPageAgent.cpp:1020
> +    if (m_geolocationOverridden && !m_geolocationPosition.get())

This can be written in one line.

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

This leads to the override being enabled by default.

> Source/WebCore/inspector/front-end/SettingsScreen.js:759
> +	  
geolocationErrorLabelElement.appendChild(document.createTextNode(WebInspector.U
IString("Error")));

"Error" -> "Emulate position unavailable".

> LayoutTests/inspector/geolocation-success.html:28
> +    navigator.geolocation.getCurrentPosition(printLocation, printError, [])

You should test watchPosition as well. It should be triggered when:
1) override is being engaged
2) override is removed (and original real position is restored)
3) user is switching from error to regular override