[webkit-reviews] review denied: [Bug 27944] Geolocation error callback not called if permissions have already been denied : [Attachment 39382] Patch 3 for bug 27944

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Steve Block <steveblock at google.com>'s
request for review:
Bug 27944: Geolocation error callback not called if permissions have already
been denied
https://bugs.webkit.org/show_bug.cgi?id=27944

Attachment 39382: Patch 3 for bug 27944
https://bugs.webkit.org/attachment.cgi?id=39382&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +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.