[webkit-reviews] review denied: [Bug 89365] Web Inspector: Geolocation override : [Attachment 151462] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied  review:
Bug 89365: Web Inspector: Geolocation override
https://bugs.webkit.org/show_bug.cgi?id=89365

Attachment 151462: Patch
https://bugs.webkit.org/attachment.cgi?id=151462&action=review

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


> Source/WebCore/inspector/Inspector.json:369
> +		   "name": "setGeolocationData",

Should be setGeolocationOverride

There should be canSetGeolocationOverride, also hidden

> Source/WebCore/inspector/Inspector.json:376
> +		   ]

should be marked as hidden

> Source/WebCore/inspector/Inspector.json:379
> +		   "name": "clearGeolocationData",

clearGeolocationOverride, should be hidden

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1176
> +	   if (pageAgent->sendGeolocationError())

Instrumentation should have no logic other than agent dispatching.

> Source/WebCore/inspector/InspectorInstrumentation.h:263
> +    static GeolocationPosition* checkGeolocationPositionOrError(Page*,
GeolocationPosition*);

I think Using PassRefPtrs would make code easier to understand.

> Source/WebCore/inspector/InspectorPageAgent.cpp:324
> +    , m_geolocationError()

There is no need to initialize the refptrs

> Source/WebCore/inspector/InspectorPageAgent.cpp:990
> +	   controller->errorOccurred(m_geolocationError.get());

So you did not override this one, hence error can still be reported by the host
into the controller, even in the override mode, right?

> Source/WebCore/inspector/InspectorPageAgent.h:118
> +    GeolocationPosition* geolocationPosition() const {return
m_geolocationPosition.get();}

PassRefPtr ?

> Source/WebCore/inspector/front-end/SettingsScreen.js:744
> +	       element.maxLength = 12;

You should style controls using CSS

> LayoutTests/inspector/geolocation-error.html:27
> +	  
InspectorTest.evaluateInPage("navigator.geolocation.getCurrentPosition(function
(pos){console.log('lat: ' + pos.coords.latitude + ',' + ' long: ' +
pos.coords.longitude);}, function(err){console.log(err);}, [])",
callbackComplete);

Please use named function and toString serialization when passing code as
strings.