Web Inspector: Geolocation override

[Konrad Piascik]

Provide a way for the user to override the geolocation of the page.

[Konrad Piascik]

Created attachment 148138 [details]
Screenshot

[Pavel Feldman]

This screenshot seems to be taken on the outdated version of tools. How do you intend to implement it?

[Konrad Piascik]

(In reply to comment #2)
> This screenshot seems to be taken on the outdated version of tools. How do you intend to implement it?

This is a screenshot of Web Inspector on PlayBook which doesn't yet support Device Metrics override. (I'll be implementing that soon as well)

I plan on using GeolocationClientMock and setting it's position and other details through InspectorPageAgent.

[Konrad Piascik]

Created attachment 148865 [details]
WIP

Work in progress.

[Konrad Piascik]

Created attachment 150846 [details]
Work in progress will add tests after more feedback

[Pavel Feldman]

Comment on attachment 150846 [details]
Work in progress will add tests after more feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=150846&action=review

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

I don't think we need a special method for this. setting geolocation with no properties could reset override.

> Source/WebCore/inspector/Inspector.json:388
> +                    { "name": "type", "type": "string", "enum": ["PermissionDenied", "PositionUnavailable"], "description": "Mock error type"},

These could be a part of the message above. Just make all the parameters optional and pass either position or the error message.

> Source/WebCore/inspector/InspectorPageAgent.cpp:977
> +void InspectorPageAgent::overrideGeolocation(ErrorString*, bool value)

Not sure why you need these. We typically use the following scheme: patch existing call site querying for location with InspectorInstrumentation::overrideGeolocation(&latitude, longitude, ...).

> Source/WebCore/inspector/front-end/SettingsScreen.js:655
> +    _createGeolocationOverrideControl: function()

Could you update screenshot for these?

> Source/WebCore/inspector/front-end/UserAgentSupport.js:174
> +WebInspector.UserAgentSupport.GeolocationPosition.prototype = {

Not sure we need such fine-grained utility support for checking for valid values.

[Konrad Piascik]

Created attachment 150932 [details]
Updated Screenshot

[Konrad Piascik]

Comment on attachment 150846 [details]
Work in progress will add tests after more feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=150846&action=review

>> Source/WebCore/inspector/Inspector.json:388
>> +                    { "name": "type", "type": "string", "enum": ["PermissionDenied", "PositionUnavailable"], "description": "Mock error type"},
> 
> These could be a part of the message above. Just make all the parameters optional and pass either position or the error message.

The way the mock geolocation object is set up if you set an error then it clears the position, and if you set the position it clears the error.  I have javascript side checks for whether an error is set or not before calling setGeolocationPosition.

>> Source/WebCore/inspector/InspectorPageAgent.cpp:977
>> +void InspectorPageAgent::overrideGeolocation(ErrorString*, bool value)
> 
> Not sure why you need these. We typically use the following scheme: patch existing call site querying for location with InspectorInstrumentation::overrideGeolocation(&latitude, longitude, ...).

I need to override the geolocation client in order to be able to supply it with coordinates or errors.  Only the mock client can have the postion/error set explicitly.  And calling the setClient method multiple times will have no effect.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:655
>> +    _createGeolocationOverrideControl: function()
> 
> Could you update screenshot for these?

Done.

>> Source/WebCore/inspector/front-end/UserAgentSupport.js:174
>> +WebInspector.UserAgentSupport.GeolocationPosition.prototype = {
> 
> Not sure we need such fine-grained utility support for checking for valid values.

why not?

[Pavel Feldman]

Comment on attachment 150846 [details]
Work in progress will add tests after more feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=150846&action=review

>>> Source/WebCore/inspector/Inspector.json:388
>>> +                    { "name": "type", "type": "string", "enum": ["PermissionDenied", "PositionUnavailable"], "description": "Mock error type"},
>> 
>> These could be a part of the message above. Just make all the parameters optional and pass either position or the error message.
> 
> The way the mock geolocation object is set up if you set an error then it clears the position, and if you set the position it clears the error.  I have javascript side checks for whether an error is set or not before calling setGeolocationPosition.

I think you can achieve what you want with the protocol method I am suggesting. You just tell the user that he is either providing the values or the error message and add a runtime check for that on the backend. You anyways need to validate the numbers on the backend in case user passes invalid data (-10000).

>>> Source/WebCore/inspector/InspectorPageAgent.cpp:977
>>> +void InspectorPageAgent::overrideGeolocation(ErrorString*, bool value)
>> 
>> Not sure why you need these. We typically use the following scheme: patch existing call site querying for location with InspectorInstrumentation::overrideGeolocation(&latitude, longitude, ...).
> 
> I need to override the geolocation client in order to be able to supply it with coordinates or errors.  Only the mock client can have the postion/error set explicitly.  And calling the setClient method multiple times will have no effect.

Why do you need to override the client? Can't you intercepts the calls WebCore makes into the Geo subsystem and use the ones provided by the inspector?

>>> Source/WebCore/inspector/front-end/UserAgentSupport.js:174
>>> +WebInspector.UserAgentSupport.GeolocationPosition.prototype = {
>> 
>> Not sure we need such fine-grained utility support for checking for valid values.
> 
> why not?

Because it is 100 lines of code that does not really do much. For example, why do we allow values in this class be invalid? Removing that case would shrink the class significantly.

[Konrad Piascik]

Ok I can probably add  InspectorInstrumentation calls in GeolocationController::positionChanged and GeolocationController::errorOccurred to get the PageAgent to supply to overwrite the GeolocationPosition GeolocationError if needed.  This way we can get rid of the override command from the JSON as well and use the checkbox to only send the GeolocationPosition/Error as needed using the amalgamated command  with optional parameters.

I'll refactor and post a patch for this soon.  Thanks for the feedback.

[Konrad Piascik]

Created attachment 151324 [details]
Patch

[Konrad Piascik]

Created attachment 151462 [details]
Patch

[Konrad Piascik]

(In reply to comment #12)
> Created an attachment (id=151462) [details]
> Patch

This patch has layout tests :)

[WebKit Review Bot]

Comment on attachment 151462 [details]
Patch

Clearing flags on attachment: 151462

Committed r122232: <http://trac.webkit.org/changeset/122232>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Pavel Feldman]

Yong Li, I would kindly ask that you don't r+ inspector changes any more. With this change you introduced a public WebKit remote debugging protocol method that violates naming conventions and should not in fact be public. For the WebKit reviewer, it is essential that you only review the changes in your area of expertise.

Konrad, would you please revert this change and send it to one of the inspector team reviewers for review.

[Pavel Feldman]

Comment on attachment 151462 [details]
Patch

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.

[Yong Li]

(In reply to comment #16)
> Yong Li, I would kindly ask that you don't r+ inspector changes any more. With this change you introduced a public WebKit remote debugging protocol method that violates naming conventions and should not in fact be public. For the WebKit reviewer, it is essential that you only review the changes in your area of expertise.
> 
> Konrad, would you please revert this change and send it to one of the inspector team reviewers for review.

Sorry for that. I was actually a little bit upset after the r+.

[Konrad Piascik]

Reopening to attach new patch.

[Konrad Piascik]

Created attachment 152074 [details]
Patch

[Pavel Feldman]

> Sorry for that. I was actually a little bit upset after the r+.

What do you mean? I am not sure I follow.

[WebKit Review Bot]

Comment on attachment 152074 [details]
Patch

Clearing flags on attachment: 152074

Committed r122532: <http://trac.webkit.org/changeset/122532>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Konrad Piascik]

Comment on attachment 151462 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151462&action=review

>> Source/WebCore/inspector/InspectorInstrumentation.cpp:1176
>> +        if (pageAgent->sendGeolocationError())
> 
> Instrumentation should have no logic other than agent dispatching.

This method currently has dual functionality.  It notifies geolocation observers of an error, if one is present AND it changes the geolocation position if it's set in the page agent.
I could make 2 functions from inspector instruemntation:
1) bool sendGeolocationError()
2) GeolocationPosition overrideGeolocationPosition(GeolocationPosition*)

and have these 2 both invoked from positionChanged returning early if 1) is true and overriding the position passed in to positionChanged with 2)

>> Source/WebCore/inspector/InspectorPageAgent.cpp:324
>> +    , m_geolocationError()
> 
> There is no need to initialize the refptrs

will remove

>> Source/WebCore/inspector/front-end/SettingsScreen.js:744
>> +            element.maxLength = 12;
> 
> You should style controls using CSS

will update

[Pavel Feldman]

Comment on attachment 151462 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151462&action=review

> Source/WebCore/Modules/geolocation/GeolocationController.cpp:103
> +    position = InspectorInstrumentation::checkGeolocationPositionOrError(m_page, position);

You should return boolean here to distinguish between 0 position and instrumentation override failure.

>> Source/WebCore/inspector/Inspector.json:369
>> +                "name": "setGeolocationData",
> 
> Should be setGeolocationOverride
> 
> There should be canSetGeolocationOverride, also hidden

overrideGeolocation

>> Source/WebCore/inspector/Inspector.json:379
>> +                "name": "clearGeolocationData",
> 
> clearGeolocationOverride, should be hidden

clearGeolocationOverride

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1173
> +GeolocationPosition* InspectorInstrumentation::checkGeolocationPositionOrErrorImpl(InstrumentingAgents* instrumentingAgents, GeolocationPosition* position)

::overrideGeolocationPosition ?

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1175
> +    if (InspectorPageAgent* pageAgent = instrumentingAgents->inspectorPageAgent()) {

I think you should pass it further into the page agent via InspectorPageAgent::overrideGeolocationPosition(position, overridenPosition);

>>> Source/WebCore/inspector/InspectorInstrumentation.cpp:1176
>>> +        if (pageAgent->sendGeolocationError())
>> 
>> Instrumentation should have no logic other than agent dispatching.
> 
> This method currently has dual functionality.  It notifies geolocation observers of an error, if one is present AND it changes the geolocation position if it's set in the page agent.
> I could make 2 functions from inspector instruemntation:
> 1) bool sendGeolocationError()
> 2) GeolocationPosition overrideGeolocationPosition(GeolocationPosition*)
> 
> and have these 2 both invoked from positionChanged returning early if 1) is true and overriding the position passed in to positionChanged with 2)

I meant that you should have InspectorPageAgent::checkGeolocationPositionOrError that would do the necessary changes. I.e. minimize the awareness of the gelocation in the instrumentation class.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1177
> +            return 0;

How do we define which error that is (Permission or Availability)?

>>> Source/WebCore/inspector/InspectorPageAgent.cpp:324
>>> +    , m_geolocationError()
>> 
>> There is no need to initialize the refptrs
> 
> will remove

No need to initialize ref pointers.

> Source/WebCore/inspector/InspectorPageAgent.cpp:984
> +    if (*errorType == "PermissionDenied")

errorType can be 0

>> 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?

It sounds like you should override errorOccurred in GeolocationController as well.

> Source/WebCore/inspector/front-end/SettingsScreen.js:660
> +        const p = document.createElement("p");

we only use const for predefined constants (numeric values, strings)

> Source/WebCore/inspector/front-end/SettingsScreen.js:668
> +        labelElement.appendChild(document.createTextNode(WebInspector.UIString("Override Geolocation")));

Please add this string into WebCore/English.lproj/locallizedStrings.js

> Source/WebCore/inspector/front-end/SettingsScreen.js:673
> +        const geolocationErrorElement = this._createRadioSetting(WebInspector.UIString("Geolocation Error"), [

"Error"

> Source/WebCore/inspector/front-end/SettingsScreen.js:675
> +        [ "PermissionDenied", WebInspector.UIString("PermissionDenied") ],

"PermissionDenied" should not be localized.

> Source/WebCore/inspector/front-end/SettingsScreen.js:676
> +        [ "PositionUnavailable", WebInspector.UIString("PositionUnavailable") ] ], WebInspector.settings.geolocationError);

ditto

> Source/WebCore/inspector/front-end/SettingsScreen.js:689
> +            this._geolocationSectionElement.removeStyleClass("hidden");

Now that we have tabs in settings, we disable controls instead of hiding them.

> Source/WebCore/inspector/front-end/SettingsScreen.js:699
> +            PageAgent.clearGeolocationData();

It sounds like you should do this through the UserAgent helper.

> Source/WebCore/inspector/front-end/SettingsScreen.js:710
> +     * @param {boolean} userInputModified

Is it worth splitting this method into two depending on this parameter?

> Source/WebCore/inspector/front-end/UserAgentSupport.js:180
> +        return (this._latitude || this._latitude == 0) && (this._longitude || this._longitude == 0) ? this._latitude + "@" + this._longitude : "";

typeof this._latitude === "number"

> LayoutTests/inspector/geolocation-error.html:13
> +                                                    mockLongitude,

weird indent

>> 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.

You should do this in the callback from the clearGeolocationData above.

Instead of this long and nasty line, declare a local function and pass "(" + func.toString() + ")()" into the method.

> LayoutTests/inspector/geolocation-error.html:29
> +    PageAgent.setGeolocationData(0, 0, 0, "PositionUnavailable");

setGeolocationData(undefined, undefined, undefined, "PositionUnavailable")

> LayoutTests/inspector/geolocation-error.html:30
> +    InspectorTest.evaluateInPage("navigator.geolocation.getCurrentPosition(function(pos){console.log(pos);}, function(err){console.log('Error: ' + err.code);}, [])", callback);

This should also be in a callback.

> LayoutTests/inspector/geolocation-success.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);

ditto

[Konrad Piascik]

Comment on attachment 151462 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151462&action=review

>> Source/WebCore/Modules/geolocation/GeolocationController.cpp:103
>> +    position = InspectorInstrumentation::checkGeolocationPositionOrError(m_page, position);
> 
> You should return boolean here to distinguish between 0 position and instrumentation override failure.

I've split this into 2 methods. One which returns a boolean and another which returns a GeolocationPosition.

>>> Source/WebCore/inspector/Inspector.json:369
>>> +                "name": "setGeolocationData",
>> 
>> Should be setGeolocationOverride
>> 
>> There should be canSetGeolocationOverride, also hidden
> 
> overrideGeolocation

I've added canOverrideGeolocation and I've changed this method to setGeolocationOverride, both are hidden.

>>> Source/WebCore/inspector/Inspector.json:379
>>> +                "name": "clearGeolocationData",
>> 
>> clearGeolocationOverride, should be hidden
> 
> clearGeolocationOverride

done and hidden

>>>> Source/WebCore/inspector/InspectorInstrumentation.cpp:1176
>>>> +        if (pageAgent->sendGeolocationError())
>>> 
>>> Instrumentation should have no logic other than agent dispatching.
>> 
>> This method currently has dual functionality.  It notifies geolocation observers of an error, if one is present AND it changes the geolocation position if it's set in the page agent.
>> I could make 2 functions from inspector instruemntation:
>> 1) bool sendGeolocationError()
>> 2) GeolocationPosition overrideGeolocationPosition(GeolocationPosition*)
>> 
>> and have these 2 both invoked from positionChanged returning early if 1) is true and overriding the position passed in to positionChanged with 2)
> 
> I meant that you should have InspectorPageAgent::checkGeolocationPositionOrError that would do the necessary changes. I.e. minimize the awareness of the gelocation in the instrumentation class.

This is now 2 methods as described in my comment above.

>> Source/WebCore/inspector/InspectorInstrumentation.cpp:1177
>> +            return 0;
> 
> How do we define which error that is (Permission or Availability)?

The method sendGeolocationError() calls geolocationController->errorOccurred() with the error stored in m_geolocationError

>> Source/WebCore/inspector/InspectorInstrumentation.h:263
>> +    static GeolocationPosition* checkGeolocationPositionOrError(Page*, GeolocationPosition*);
> 
> I think Using PassRefPtrs would make code easier to understand.

then at the call site I would need to release/leak the ptr?

>> Source/WebCore/inspector/InspectorPageAgent.cpp:984
>> +    if (*errorType == "PermissionDenied")
> 
> errorType can be 0

will add guard against it.

>>> 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?
> 
> It sounds like you should override errorOccurred in GeolocationController as well.

So you think that I could/should add a way to overwrite the error passed in if m_geolocationError is set, and return early if m_geolocationPosition is set?  This would be similar to how things are done with positionChanged().
This would allow mean that if you call getCurrentPosition and then denied the request in the browser, you'd get whatever the overwritten geolocation details are from WebInspector.  Sounds good to me.

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

I'm getting rid of this method.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:660
>> +        const p = document.createElement("p");
> 
> we only use const for predefined constants (numeric values, strings)

fixing.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:668
>> +        labelElement.appendChild(document.createTextNode(WebInspector.UIString("Override Geolocation")));
> 
> Please add this string into WebCore/English.lproj/locallizedStrings.js

done.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:673
>> +        const geolocationErrorElement = this._createRadioSetting(WebInspector.UIString("Geolocation Error"), [
> 
> "Error"

Will change to error and add to localizedStrings.js

>> Source/WebCore/inspector/front-end/SettingsScreen.js:675
>> +        [ "PermissionDenied", WebInspector.UIString("PermissionDenied") ],
> 
> "PermissionDenied" should not be localized.

ok

>> Source/WebCore/inspector/front-end/SettingsScreen.js:676
>> +        [ "PositionUnavailable", WebInspector.UIString("PositionUnavailable") ] ], WebInspector.settings.geolocationError);
> 
> ditto

ok

>> Source/WebCore/inspector/front-end/SettingsScreen.js:689
>> +            this._geolocationSectionElement.removeStyleClass("hidden");
> 
> Now that we have tabs in settings, we disable controls instead of hiding them.

will update...was copying code from old version of deviceMetrics

>> Source/WebCore/inspector/front-end/SettingsScreen.js:699
>> +            PageAgent.clearGeolocationData();
> 
> It sounds like you should do this through the UserAgent helper.

I can add to UserAgentSupport.js, but I'm not sure where it belongs.   Should it be part of the GeolocationPosition object as a "static" type of method?

>> Source/WebCore/inspector/front-end/SettingsScreen.js:710
>> +     * @param {boolean} userInputModified
> 
> Is it worth splitting this method into two depending on this parameter?

I don't think this is worthwhile, we can address this in a follow up if you feel strongly about it.

>> Source/WebCore/inspector/front-end/UserAgentSupport.js:180
>> +        return (this._latitude || this._latitude == 0) && (this._longitude || this._longitude == 0) ? this._latitude + "@" + this._longitude : "";
> 
> typeof this._latitude === "number"

done.

>> LayoutTests/inspector/geolocation-error.html:13
>> +                                                    mockLongitude,
> 
> weird indent

fixing

>>> 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.
> 
> You should do this in the callback from the clearGeolocationData above.
> 
> Instead of this long and nasty line, declare a local function and pass "(" + func.toString() + ")()" into the method.

good idea.

>> LayoutTests/inspector/geolocation-error.html:29
>> +    PageAgent.setGeolocationData(0, 0, 0, "PositionUnavailable");
> 
> setGeolocationData(undefined, undefined, undefined, "PositionUnavailable")

will update.

[Konrad Piascik]

re-opening and will have new patch ready soon.

[Konrad Piascik]

Created attachment 152560 [details]
Patch

[Pavel Feldman]

Comment on attachment 152560 [details]
Patch

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

[Konrad Piascik]

Created attachment 152814 [details]
Patch

[Pavel Feldman]

Comment on attachment 152814 [details]
Patch

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->GeolocationController. 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

[Konrad Piascik]

Comment on attachment 152814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152814&action=review

>> 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->GeolocationController. 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;
> }

the current path of GeolocationController->InspectorInstrumentation->InspectorPageAgent->GeolocationController doesn't call positionChanged but errorOccurred, so there is no loop.  I like your change to have overrideGeolocationPosition to take &m_lastPosition and will update accordingly.

>> 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.

Will fix typo and update wording.

>> 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.

So what do you suggest? not have the errorType parameter?

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

will change

>> Source/WebCore/inspector/front-end/UserAgentSupport.js:198
>> +                if (splitPosition.length === 2) {
> 
> No need for { } around one line block

ok

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

fixing...

[Pavel Feldman]

> the current path of GeolocationController->InspectorInstrumentation->InspectorPageAgent->GeolocationController doesn't call positionChanged but errorOccurred, so there is no loop.

I did not mean the stack overflow, but rather the semantics loop where geolocation contorller was asking to eventually make a call on itself.

> So what do you suggest? not have the errorType parameter?

Yep!

[Konrad Piascik]

Created attachment 152882 [details]
Patch

[Konrad Piascik]

Comment on attachment 152882 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152882&action=review

> 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.

> Source/WebCore/inspector/front-end/SettingsScreen.js:676
> +        geolocationErrorCheckboxElement.appendChild(document.createTextNode(WebInspector.UIString("Error")));

For some reason during my testing this label wasn't showing up in Safari, but the same code did work in the BlackBerry port.  I'll need to investigate why, but wanted to submit the rest since this is just cosmetic.

[WebKit Review Bot]

Comment on attachment 152882 [details]
Patch

Attachment 152882 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13274343

New failing tests:
inspector/geolocation-success.html
inspector/geolocation-error.html

[WebKit Review Bot]

Created attachment 152907 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Pavel Feldman]

Comment on attachment 152882 [details]
Patch

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.

[Pavel Feldman]

Comment on attachment 152882 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152882&action=review

>>> 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.

Now that I look at it, I think that the approach is a bit wrong. Imagine that 
1) you enabled the override in the inspector
2) device received new location data and embedder is calling into this method

you'll end up with generating the notification about the location change to the same location. That's not what the page expects. I now think you should instead do:

if (InspectorInstrumentation::geolocationPositionOverriden(m_page))
    return;

>> Source/WebCore/inspector/InspectorPageAgent.cpp:981
>> +    if (sendGeolocationError())
> 
> You don't need this anymore, forcing positionChanged will do it for you, right?

Then you leave this method as is so that upon the protocol command you were either reporting the error or the new position.

[Konrad Piascik]

Comment on attachment 152882 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152882&action=review

> Source/WebCore/Modules/geolocation/GeolocationController.cpp:106
> +        errorOccurred(GeolocationError::create(GeolocationError::PositionUnavailable, "PositionUnavailable").leakRef());

If I go with the above option, then how do I know that there's an GeolocationError or not?  The return value of geolocaitonPositionOverride() is changed to bool, but when true we return early, but on what condition do I send the error?

[Konrad Piascik]

Comment on attachment 152882 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152882&action=review

> Source/WebCore/Modules/geolocation/GeolocationController.cpp:106
> +        errorOccurred(GeolocationError::create(GeolocationError::PositionUnavailable, "PositionUnavailable").leakRef());

If I go with the above option, then how do I know that there's an GeolocationError or not?  The return value of geolocaitonPositionOverride() is changed to bool, but when true we return early, but on what condition do I send the error?

[Konrad Piascik]

Created attachment 153081 [details]
Patch

[Konrad Piascik]

Comment on attachment 153081 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153081&action=review

> Source/WebCore/Modules/geolocation/GeolocationController.cpp:104
> +    position = InspectorInstrumentation::overrideGeolocationPosition(m_page, position);

We need this behaviour otherwise getCurrentPosition won't behave the way users would expect.  That's because getCurrentPosition's handler will only get called when positionChanged notifies the observers, which we prevent if overrideGeolocationPosition returns early.  Also watchPosition does send the same position if it repeats so this is fine here.

> LayoutTests/inspector/geolocation-success.html:14
> +                                                    mockAccuracy);

fixing this..got lost in my rebases. will wait for any other feedback before submitting another patch.

[Pavel Feldman]

(In reply to comment #43)
> (From update of attachment 153081 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153081&action=review
> 
> > Source/WebCore/Modules/geolocation/GeolocationController.cpp:104
> > +    position = InspectorInstrumentation::overrideGeolocationPosition(m_page, position);
> 
> We need this behaviour otherwise getCurrentPosition won't behave the way users would expect.  That's because getCurrentPosition's handler will only get called when positionChanged notifies the observers, which we prevent if overrideGeolocationPosition returns early.  Also watchPosition does send the same position if it repeats so this is fine here.

I glanced through the code and I thought that positionChanged will set m_lastPosition in the controller. Geolocation will then use its lastPosition when reporting the result. We just need to make sure haveSuitableCachedPosition returns true in our case.

> 
> > LayoutTests/inspector/geolocation-success.html:14
> > +                                                    mockAccuracy);
> 
> fixing this..got lost in my rebases. will wait for any other feedback before submitting another patch.

[Konrad Piascik]

Created attachment 153286 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 153286 [details]
Patch

Attachment 153286 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13274942

[Konrad Piascik]

Created attachment 153330 [details]
Patch

[Pavel Feldman]

Comment on attachment 153330 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153330&action=review

> Source/WebCore/inspector/InspectorPageAgent.cpp:982
> +    if (!latitude && !longitude && !accuracy && controller) {

You should not call errorOccurred from both places (controller and the agent), just kick the positionChanged here and things will trigger.

> Source/WebCore/inspector/InspectorPageAgent.cpp:999
> +#if ENABLE(GEOLOCATION)

Clearing the override should kick positionChanged with the platform position. I'll attached my version of InspectorPageAgent to this bug.

> Source/WebCore/inspector/InspectorPageAgent.cpp:1020
> +    if (m_geolocationOverridden && !m_geolocationPosition.get())

This can be written in one line.

> Source/WebCore/inspector/front-end/SettingsScreen.js:665
> +        checkboxElement.checked = !geolocation || (geolocation.latitude && geolocation.longitude);

This leads to the override being enabled by default.

> Source/WebCore/inspector/front-end/SettingsScreen.js:759
> +        geolocationErrorLabelElement.appendChild(document.createTextNode(WebInspector.UIString("Error")));

"Error" -> "Emulate position unavailable".

> LayoutTests/inspector/geolocation-success.html:28
> +    navigator.geolocation.getCurrentPosition(printLocation, printError, [])

You should test watchPosition as well. It should be triggered when:
1) override is being engaged
2) override is removed (and original real position is restored)
3) user is switching from error to regular override

[Pavel Feldman]

Created attachment 153487 [details]
[DIFF] Page agent proposal

[Konrad Piascik]

Created attachment 153571 [details]
Patch

[Pavel Feldman]

Comment on attachment 153571 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153571&action=review

> Source/WebCore/inspector/InspectorPageAgent.cpp:1003
> +    if (controller && m_platformGeolocationPosition.get())

Nit: if (controller && m_platformGeolocationPosition)

> Source/WebCore/inspector/InspectorPageAgent.cpp:1004
> +        controller->positionChanged(m_platformGeolocationPosition.release().leakRef());

Does this actually leak the pointer? What is the contract of positionChanged? Should it adopt the pointer or does it receive refptr? Migrating from raw pointer to PassRefPtr would really help understanding there.

> Source/WebCore/inspector/InspectorPageAgent.cpp:1023
> +        if (position && m_geolocationPosition.get() && position != m_geolocationPosition.get())

If m_geolocationPosition is 0 when you start emulation, you don't have a chance to assign to it here. I guess the check should be if (position) only.

> Source/WebCore/inspector/front-end/SettingsScreen.js:665
> +        checkboxElement.checked = !geolocation || (geolocation.latitude && geolocation.longitude);

As I stated previously, this will make geolocation enabled by default (upon the first settings pane opening). Please fix this.

[Konrad Piascik]

Comment on attachment 153571 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153571&action=review

>> Source/WebCore/inspector/InspectorPageAgent.cpp:1004
>> +        controller->positionChanged(m_platformGeolocationPosition.release().leakRef());
> 
> Does this actually leak the pointer? What is the contract of positionChanged? Should it adopt the pointer or does it receive refptr? Migrating from raw pointer to PassRefPtr would really help understanding there.

From what I understand the contract of positionChanged is that it takes a raw pointer and m_lastPosition is the RefPtr that owns it.  Are you suggesting changing the signature of positionChanged to take a PassRefPtr instead of a raw pointer?  If so I'd suggest leaving this change for a separate bug.

>> Source/WebCore/inspector/InspectorPageAgent.cpp:1023
>> +        if (position && m_geolocationPosition.get() && position != m_geolocationPosition.get())
> 
> If m_geolocationPosition is 0 when you start emulation, you don't have a chance to assign to it here. I guess the check should be if (position) only.

What happens here is that when you call controller->positionChanged(controller->lastPosition()) position is the same as m_geolocationPosition so we set m_platformGeolocationPosition gets set to m_geolocationPosition.  This is why I check "if (position != m_geolocationPosition.get())".  Also when the position is overridden with an error m_geolocationPosition is 0.  So controller->lastPosition() returns the most recent value of m_geolocationPosition, but since m_geolocationPosition is now 0 and position is not, we need to check whether m_geolocationPosition is 0.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:665
>> +        checkboxElement.checked = !geolocation || (geolocation.latitude && geolocation.longitude);
> 
> As I stated previously, this will make geolocation enabled by default (upon the first settings pane opening). Please fix this.

will do...

[Konrad Piascik]

Created attachment 153862 [details]
Patch

[Pavel Feldman]

Comment on attachment 153571 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153571&action=review

>>> Source/WebCore/inspector/InspectorPageAgent.cpp:1004
>>> +        controller->positionChanged(m_platformGeolocationPosition.release().leakRef());
>> 
>> Does this actually leak the pointer? What is the contract of positionChanged? Should it adopt the pointer or does it receive refptr? Migrating from raw pointer to PassRefPtr would really help understanding there.
> 
> From what I understand the contract of positionChanged is that it takes a raw pointer and m_lastPosition is the RefPtr that owns it.  Are you suggesting changing the signature of positionChanged to take a PassRefPtr instead of a raw pointer?  If so I'd suggest leaving this change for a separate bug.

m_lastPosition = position does not adopt the pointer, it just addrefs it. So as I understand, you are introducing the memory leak via leakRef here (PassRefPtr returns the pointer without de-reffing it in leakRef). You should do controller->positionChanged(m_platformGeolocationPosition.get()); as suggested in my patch.

>>> Source/WebCore/inspector/InspectorPageAgent.cpp:1023
>>> +        if (position && m_geolocationPosition.get() && position != m_geolocationPosition.get())
>> 
>> If m_geolocationPosition is 0 when you start emulation, you don't have a chance to assign to it here. I guess the check should be if (position) only.
> 
> What happens here is that when you call controller->positionChanged(controller->lastPosition()) position is the same as m_geolocationPosition so we set m_platformGeolocationPosition gets set to m_geolocationPosition.  This is why I check "if (position != m_geolocationPosition.get())".  Also when the position is overridden with an error m_geolocationPosition is 0.  So controller->lastPosition() returns the most recent value of m_geolocationPosition, but since m_geolocationPosition is now 0 and position is not, we need to check whether m_geolocationPosition is 0.

Again, in case of no position (emulated position unavailable), you are not going to track platform position.
To simplify this code, you should set m_platformGeolocationPosition in setGeolocationOverride, update it here on non-0 values and restore in clearGeolocationOverride. Otherwise it looks confusing.

[Pavel Feldman]

Comment on attachment 153862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153862&action=review

As per comments for previous patch. Btw, there is no need to submit patches unless comments are addressed.

> Source/WebCore/inspector/front-end/SettingsScreen.js:665
> +        checkboxElement.checked = false;

Do you want to store this setting? Otherwise re-opening the front-end will reset its state. See the way device metrics implements it.

[Konrad Piascik]

Comment on attachment 153862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153862&action=review

>> Source/WebCore/inspector/front-end/SettingsScreen.js:665
>> +        checkboxElement.checked = false;
> 
> Do you want to store this setting? Otherwise re-opening the front-end will reset its state. See the way device metrics implements it.

I can change the line above to be:
checkboxElement.checked = !geolocation || (((typeof geolocation.latitude === "number") && (typeof geolocation.longitude === "number")) || geolocation.error);
OR
I can add a second setting to determine whether the element should be checked or not, but this is not what device metrics does.
If we want this setting enabled right away when the inspector is connected what signal or event shouldI listen for? Is there an example that I could follow, because I don't see anything about device metrics that does this either.

[Konrad Piascik]

Comment on attachment 153862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153862&action=review

>> Source/WebCore/inspector/front-end/SettingsScreen.js:665
>> +        checkboxElement.checked = false;
> 
> Do you want to store this setting? Otherwise re-opening the front-end will reset its state. See the way device metrics implements it.

I can change the line above to be:
checkboxElement.checked = !geolocation || (((typeof geolocation.latitude === "number") && (typeof geolocation.longitude === "number")) || geolocation.error);
OR
I can add a second setting to determine whether the element should be checked or not, but this is not what device metrics does.
If we want this setting enabled right away when the inspector is connected what signal or event shouldI listen for? Is there an example that I could follow, because I don't see anything about device metrics that does this either.

[Pavel Feldman]

Comment on attachment 153862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153862&action=review

>>>> Source/WebCore/inspector/front-end/SettingsScreen.js:665
>>>> +        checkboxElement.checked = false;
>>> 
>>> Do you want to store this setting? Otherwise re-opening the front-end will reset its state. See the way device metrics implements it.
>> 
>> I can change the line above to be:
>> checkboxElement.checked = !geolocation || (((typeof geolocation.latitude === "number") && (typeof geolocation.longitude === "number")) || geolocation.error);
>> OR
>> I can add a second setting to determine whether the element should be checked or not, but this is not what device metrics does.
>> If we want this setting enabled right away when the inspector is connected what signal or event shouldI listen for? Is there an example that I could follow, because I don't see anything about device metrics that does this either.
> 
> I can change the line above to be:
> checkboxElement.checked = !geolocation || (((typeof geolocation.latitude === "number") && (typeof geolocation.longitude === "number")) || geolocation.error);
> OR
> I can add a second setting to determine whether the element should be checked or not, but this is not what device metrics does.
> If we want this setting enabled right away when the inspector is connected what signal or event shouldI listen for? Is there an example that I could follow, because I don't see anything about device metrics that does this either.

Lets leave it non-saveable for now. I think it is time to land this change. Let me land it and hide this override behind the experiment  until its UX is polished!

[Pavel Feldman]

Comment on attachment 153862 [details]
Patch

Oh-ah I forgot that it still leaks the pointer.

[Pavel Feldman]

Comment on attachment 153862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153862&action=review

> Source/WebCore/inspector/front-end/SettingsScreen.js:733
> +            var element = parentElement.createChild("input");

I am sure I was asking for it already - please use CSS for styling.

[Konrad Piascik]

Created attachment 154336 [details]
Patch

[Pavel Feldman]

Comment on attachment 154336 [details]
Patch

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.

[Konrad Piascik]

Created attachment 154358 [details]
Patch

[WebKit Review Bot]

Comment on attachment 154358 [details]
Patch

Clearing flags on attachment: 154358

Committed r123634: <http://trac.webkit.org/changeset/123634>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Saboff]

It looks like r123634 breaks debug builds.

In Source/WebCore/inspector/InspectorPageAgent.cpp, line 499 I think you want UNUSED_PARAM() instead of ASSERT_UNUSED().

[Michael Saboff]

I checked in r123639 with build fix.

[Benjamin Poulain]

I lost track of this and I just see it now. I would prefer if that code was #ifdef-ed with a ENABLE.

Is it intended that the GeolocationProvider will keep sending errors in parallel to the geolocation override? This seems odd.

The way GeolocationController is modified is unfortunate.

[Pavel Feldman]

(In reply to comment #68)
> I lost track of this and I just see it now. I would prefer if that code was #ifdef-ed with a ENABLE.
> 

Yeah, it took a lot of iterations. Which ENABLE do you mean?

> Is it intended that the GeolocationProvider will keep sending errors in parallel to the geolocation override? This seems odd.
> 

What is the scenario? Lets follow up, I remember something fishy there, but I thought it was resolved.

> The way GeolocationController is modified is unfortunate.

We should follow up. Do you want this change to be rolled back or you think it cab be fixed live?

[Konrad Piascik]

(In reply to comment #69)
> (In reply to comment #68)
> > I lost track of this and I just see it now. I would prefer if that code was #ifdef-ed with a ENABLE.
> > 
> 
> Yeah, it took a lot of iterations. Which ENABLE do you mean?
> 
> > Is it intended that the GeolocationProvider will keep sending errors in parallel to the geolocation override? This seems odd.
> > 
> 
> What is the scenario? Lets follow up, I remember something fishy there, but I thought it was resolved.
> 
> > The way GeolocationController is modified is unfortunate.
> 
> We should follow up. Do you want this change to be rolled back or you think it cab be fixed live?

Let me know what your exact issues and I'll address them.