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

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


I think if you encode position unavailable with missing values, you'll be able
to make your backend code twice simpler.

> Source/WebCore/Modules/geolocation/GeolocationController.cpp:103
> +    if (InspectorInstrumentation::sendGeolocationError(m_page))

This is still very strange. Your call flow is as follows:
GeolocationController->InspectorInstrumentation->InspectorPageAgent->Geolocatio
nController. I.e. it makes a loop. It would be way cleaner if you did 

bool positionAvailable =
InspectorInstrumentation::overrideGeolocationPosition(m_page, position,
&m_lastPosition);
if (!positionAvailable) {
   
errorOccurred(GeolocationError::create(GeolocationError::PositionUnavailable,
*errorType));
    return;
}

> Source/WebCore/inspector/Inspector.json:370
> +		   "description": "Wverrides the GeolocationPosition or
GeolocationError in the GeolocationController.",

"Overrides". You should not mention WebCore class names since other browsers
supporting this protocol may have it under different name.

> Source/WebCore/inspector/Inspector.json:375
> +		       { "name": "errorType", "type": "string", "optional":
true, "enum": ["PermissionDenied", "PositionUnavailable"], "description":
"Error type"}

PermissionDenied should not be here. Since long/lat/acc are optional, missing
values could encode unavailable position.

> Source/WebCore/inspector/front-end/UserAgentSupport.js:172
> +WebInspector.UserAgentSupport.GeolocationPosition = function (latitude,
longitude, error)

no space after function

> Source/WebCore/inspector/front-end/UserAgentSupport.js:198
> +		   if (splitPosition.length === 2) {

No need for { } around one line block

> Source/WebCore/inspector/front-end/UserAgentSupport.js:201
> +	       }

poor indent