Bug 27944 – Geolocation error callback not called if permissions have already been denied

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Steve Block]

If geolocation.getCurrentPosition() or geolocation.watchPosition() is called after Geolocation permissions have previously been denied, and the implementation obtains a position fix, the error callback is not called to indicate that permissions have been denied. Instead, no callback is made.

In Geolocation::geolocationServicePositionChanged(), if Geolocation permissions have previously been denied, isAllowed() returns false and the method returns without any callbacks being called. The error callback should instead be called with code PERMISSION_DENIED to indicate that permissions have been denied.

[Steve Block]

Created attachment 34779 [details]
Patch 1 for bug 27944

When a Geolocation method is called, immediately calls the error callback asynchronously if permissions have already been denied.

Note that the naming of the new 'setFatalError' and 'fatalErrorOccurred' methods is intentionally generic. This is because this framework will be used for other fatal error conditions. Currently, if the Geolocation service fails to start, the error callback is invoked synchronously. This is incorrect. See Bug 28276. This framework will be used to fix this behavior also.

LayoutTests for this behavior will be added once the mock Geolocation service framework is in place. See Bug 28264.

[Steve Block]

Created attachment 38375 [details]
Patch 2 for bug 27944

> We don't have a find method for this type?
I'm not aware of one that searches the value, rather than the key, of the map.

> Style:
Fixed

> No need to store it locally first:
Fixed

> Aren't the two if (isDenied()) blocks your editing identical?  Cant' we share
> code there?
Fixed

> Arguments are only named when the name adds clarity:
Fixed

> Who would know about this code?  I certainly don't.
It was written by Greg Bolsinga, but he's not a WebKit reviewer.

[Greg Bolsinga]

CC'ing Sam. He was the original Geolocation reviewer.

[Adam Barth]

Comment on attachment 38375 [details]
Patch 2 for bug 27944

I'm sorry, but we need test coverage before we can move forward with these patches.

[Steve Block]

Created attachment 39382 [details]
Patch 3 for bug 27944

Updated patch to include LayoutTests.

[Darin Adler]

Comment on attachment 39382 [details]
Patch 3 for bug 27944

> +static const char* permissionDeniedErrorMessage = "User denied Geolocation";

This creates a non-constant pointer to a constant string. You probably want either "static const char* const" or "static const char[]". Or an inline function that returns a constant string instead.

>      , m_timer(this, &Geolocation::GeoNotifier::timerFired)
> +    , m_fatalError(0)

Not needed. A RefPtr will start as 0 even if you don't list it.

> +    m_fatalError = error;
> +    m_timer.startOneShot(0);

Can this be called when m_fatalError is already non-null? If not, should we assert that it's null?

Can this be called when the timer has already started? Would it do any harm to call startOneShot again in that case?

> +    if (notifier == 0)

WebKit coding style writes this as:

    if (!notifier)

> +    static int sIdentifier = 0;

I know you're just moving code, but the variable name here is strange. Since this is an extremely-local variable with tiny scope, I think it's best not to use a prefix on the name at all. But if you do want to use a prefix, it should not be lowercase s followed by a capital letter. Note also that this is non-thread-safe, which may be fine, depending on whether workers are allowed to use geolocation or not.

> +    for (GeoNotifierMap::iterator iter = m_watchers.begin(); iter != m_watchers.end(); ++iter) {
> +        if (iter->second == notifier) {
> +            m_watchers.remove(iter);
> +            break;
> +        }
> +    }

Can a notifier have more than one watcher? It's unfortunate that we have to iterate the map like this. Normally that's an anti-pattern -- we'd use a pair of maps to avoid it.

> +        void setFatalError(PassRefPtr<PositionError> error);

We omit argument names like "error" in cases like this where the name simply repeats the type.

> +<p id="description"></p>
> +<div id="console"></div>

It's really unfortunate to not just use the standard template for tests like this. Do you really need these elements in markup? You could just use a couple of lines of JavaScript to create them instead.

[Steve Block]

> This creates a non-constant pointer to a constant string.
Will fix.

> Not needed. A RefPtr will start as 0 even if you don't list it.
Will fix.

> Can this be called when m_fatalError is already non-null? If not, should we
> assert that it's null?
Will add an assertion.

> Can this be called when the timer has already started? Would it do any harm to
> call startOneShot again in that case?
No, this method will be called at most once on a given GeoNotifier object. Will add a comment.

> WebKit coding style writes this as:
> 
>     if (!notifier)
Will fix.

> I know you're just moving code, but the variable name here is strange.
Will rename to identifier.

> Can a notifier have more than one watcher?
GeoNotifier represents a single request, which may be either a one-shot or a watcher. This structure maps from watch ID (type int) to watcher (type GeoNotifier). There's a one-to-one mapping between watch ID and watcher.

> It's unfortunate that we have to
> iterate the map like this. Normally that's an anti-pattern -- we'd use a pair
> of maps to avoid it.
An alternative to having to maintain a pair of maps would be to pass the watch ID to the GeoNotifier constructor. The GeoNotifier would then pass this watch ID, rather than its 'this' pointer, to this method. For one-shots, which don't have a watch ID, we could use either a single special value, or assign each a unique ID for internal use only. What do you think? This might be best done in a separate patch.

> We omit argument names like "error" in cases like this where the name simply
> repeats the type.
Will fix.

> > +<p id="description"></p>
> > +<div id="console"></div>
> 
> It's really unfortunate to not just use the standard template for tests like
> this. Do you really need these elements in markup? You could just use a couple
> of lines of JavaScript to create them instead.
Can you point me to the standard template? I've used the templates from fast/js (eg constructor.html) for the basis of all of the Geolocation tests. These templates include these elements in markup. (Note that since most of the Geolocation tests are asynchronous, I've modified the templates to remove js-test-post.js.)

[Darin Adler]

(In reply to comment #7)
> > It's unfortunate that we have to
> > iterate the map like this. Normally that's an anti-pattern -- we'd use a pair
> > of maps to avoid it.
> An alternative to having to maintain a pair of maps would be to pass the watch
> ID to the GeoNotifier constructor. The GeoNotifier would then pass this watch
> ID, rather than its 'this' pointer, to this method. For one-shots, which don't
> have a watch ID, we could use either a single special value, or assign each a
> unique ID for internal use only. What do you think? This might be best done in
> a separate patch.

That sounds like a reasonable design. I don't feel that strongly but it's always a warning sign when someone is iterating a map. It typically means that either:

    1) The map is always small, so maybe it should just be a vector instead.

or

    2) There's a latent performance problem

> > It's really unfortunate to not just use the standard template for tests like
> > this. Do you really need these elements in markup? You could just use a couple
> > of lines of JavaScript to create them instead.
> Can you point me to the standard template? I've used the templates from fast/js
> (eg constructor.html) for the basis of all of the Geolocation tests. These
> templates include these elements in markup. (Note that since most of the
> Geolocation tests are asynchronous, I've modified the templates to remove
> js-test-post.js.)

My mistake. It looks like this is the standard template! That is the file named TEMPLATE.html in each directory. The concept is that you don't write the HTML at all. The make-script-test-wrappers script does it.

The goal here is to be able to change the template later, though, so the need to remove js-test-post.js is unfortunate. Maybe there's a way to handle that without customizing the HTML driver for each test. Maybe a function call to indicate the test is async? Or a different TEMPLATE.html file in a particular directory, but still using the automatic generation rather than hand-modifying the HTML file.

[Steve Block]

> That sounds like a reasonable design.
OK, I'll refactor to use an ID to identify all notifiers. This will also simplify some of the logic. I've been wanting to do this for a while. I've opened Bug 29178, which block this bug, to address this.

> The goal here is to be able to change the template later, though, so the need
> to remove js-test-post.js is unfortunate. Maybe there's a way to handle that
> without customizing the HTML driver for each test. Maybe a function call to
> indicate the test is async? Or a different TEMPLATE.html file in a particular
> directory, but still using the automatic generation rather than hand-modifying
> the HTML file.
A JS function call sounds like the best approach. I've opened Bug 29179, which block this bug, to address this.

[Steve Block]

Created attachment 41986 [details]
Patch 4 for Bug 27944

Updated patch now that blocking issues have been fixed.

[WebKit Commit Bot]

Comment on attachment 41986 [details]
Patch 4 for Bug 27944

Clearing flags on attachment: 41986

Committed r50190: <http://trac.webkit.org/changeset/50190>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.