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

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


> Source/WebCore/inspector/InspectorPageAgent.cpp:981
> +    if (controller)

The WebKit pattern for this would be
if (!controller)
    return;

> Source/WebCore/inspector/InspectorPageAgent.cpp:984
> +	   return;

You should provide error message here.

> Source/WebCore/inspector/InspectorPageAgent.cpp:994
> +    controller->positionChanged(controller->lastPosition()); // Kick
location update.

I thought we agreed that you only pass 0 here in case positionChanged is called
from within inspector.

> Source/WebCore/inspector/InspectorPageAgent.cpp:1028
> +    if (m_geolocationOverridden)

And below it is safe to do:
if (position)
    m_platformGeolocationPosition = position;

> Source/WebCore/inspector/front-end/SettingsScreen.js:352
> +    if (Capabilities.canOverrideGeolocation &&
WebInspector.experimentsSettings.experimentsEnabled)

You need a dedicated experiment as I suggested in the review.