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

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


You also need to look at the tests - they seem to fail on chromium.

>> Source/WebCore/Modules/geolocation/GeolocationController.cpp:104
>> +	position =
InspectorInstrumentation::overrideGeolocationPosition(m_page, position);
> 
> This needs to return a position since setting of the RefPtr m_lastPosition
needs to happen here and passing in a reference to the RefPtr did not work in
my testing.

This looks good to me.

> Source/WebCore/inspector/InspectorPageAgent.cpp:976
> +    clearGeolocationOverride(0);

Nit: create empty ErrorString object above (in case someone decides to populate
it from the clear method. Actually you might want to populate it for the non
ENABLE(GEOLOCATION) case.

> Source/WebCore/inspector/InspectorPageAgent.cpp:979
> +	   m_geolocationError =
GeolocationError::create(GeolocationError::PositionUnavailable,
"PositionUnavailable").leakRef();

You don't need this object anymore, it is created in the controller itself,
right?

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

You don't need this anymore, forcing positionChanged will do it for you, right?


> Source/WebCore/inspector/InspectorPageAgent.cpp:1006
> +bool InspectorPageAgent::sendGeolocationError()

You don't need this method.

> Source/WebCore/inspector/InspectorPageAgent.cpp:1020
> +bool InspectorPageAgent::sendGeolocationPosition()

rename to notifyPositionChanged ?

> Source/WebCore/inspector/InspectorPageAgent.cpp:1023
> +    if (m_geolocationPosition.get()) {

If you remove this check, you can combine error notification with position
notification. This method could simply do:

GeolocationController* controller = GeolocationController::from(m_page);
if (controller)
    controller->positionChanged(0);

You will anyways override the position.

> Source/WebCore/inspector/InspectorPageAgent.cpp:1037
> +    if (m_geolocationError.get())

This is now !m_geolocationPosition, right?

> Source/WebCore/inspector/front-end/SettingsScreen.js:352
> +    p.appendChild(this._createGeolocationOverrideControl());

Sorry for missing this one earlier: you should check for canOverrideGeolocation
here as with the device metrics above. See how device metrics override is wired
in the front-end.