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

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


Looks good, a couple of nits inline.

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

I would think that in case of error, positionChanged should be simply ignored.
Why would we need to send an error like "Permission denied" every time device
is moved? I am not an expert in the domain though, so I might be wrong.

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

ditto

> Source/WebCore/inspector/InspectorPageAgent.cpp:1003
> +    true;

Inlining *out_param = here would be more readable.

> Source/WebCore/inspector/front-end/Settings.js:102
> +    this.geolocationOverride = this.createSetting("geolocationOverride",
"");

Can we combine the two in a single JSON object?

> LayoutTests/inspector/geolocation-error.html:38
> +    InspectorTest.evaluateInPage("navigator.geolocation.getCurrentPosition("
+ printLocation.toString() + ", " + printError.toString() + ", [])", callback);


Actually, your functions here don't even have parameters. Just declare

function getCurrentPosition() above function test() that does all that you
need.

> LayoutTests/inspector/geolocation-success.html:35
> +	  
InspectorTest.evaluateInPage("navigator.geolocation.getCurrentPosition(" +
printPosition.toString() + ", " + printError.toString() + ", [])",
callbackComplete);

ditto