[webkit-reviews] review granted: [Bug 29178] Geolocation should be refactored to use a single map to store all notifiers : [Attachment 40361] Patch 1 for bug 29178

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Steve Block <steveblock at google.com>'s
request for review:
Bug 29178: Geolocation should be refactored to use a single map to store all
notifiers
https://bugs.webkit.org/show_bug.cgi?id=29178

Attachment 40361: Patch 1 for bug 29178
https://bugs.webkit.org/attachment.cgi?id=40361&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
It's a little inelegant to use positive and negative numbers to make the map do
two different things. It's the sort of "cleverness" we often try to avoid. I'll
point out that the same thing can be done with unsigned integers just by
choosing an arbitrary large number and having both sets of IDs go up. The large
number approach avoids having to write the comment about avoiding the magic
number -1. You also would need a function isValidWatchId to use instead of the
"> 0" and "<= 0" checks, but this is actually an advantage rather than a
disadvantage, since the code will be easier to read. A function like that would
be good even if we do stick with the negative numbers.

In both cases, the real issue is guaranteeing the number won't wrap around. I'd
like to see a comment explaining why overflow is known to not be an issue.
Can't people intentionally do something in a tight loop and create the
overflow?

I suppose we can live with it.

Normally I like to name the global something like lastUsedWatchID or
nextAvailableWatchID rather than watchID to make it more clear why it's a
global variable.

> +    GeoNotifierMap::const_iterator end = m_notifiers.end();
> +    for (GeoNotifierMap::const_iterator it = m_notifiers.begin(); it != end;
++it) {
> +	   RefPtr<GeoNotifier> notifier = it->second;
>	   notifier->m_timer.stop();
>      }

This is an unnecessary bit of reference count churn. I suggest writing it like
this:

    it->second->notifier->m_timer.stop();

or like this:

    const RefPtr<GeoNotifier>& notifier = it->second;
    notifier->m_timer.stop();

or like this:

    GeoNotifier* notifier = it->second.get();
    notifier->m_timer.stop();

r=me