[webkit-reviews] review denied: [Bug 40374] Client-based Geolocation does not pass high power option to controller and client : [Attachment 59935] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Steve Block <steveblock at google.com>'s
request for review:
Bug 40374: Client-based Geolocation does not pass high power option to
controller and client
https://bugs.webkit.org/show_bug.cgi?id=40374

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

------- Additional Comments from Darin Adler <darin at apple.com>
The mock provider should be changed to support this feature, so it can be
tested.

I suggest using the terminology high accuracy consistently, not mixing in the
term high power. We want to match the Geolocation specification, and need not
include assumptions about the implementation.

> +    if (m_observers.contains(observer))
> +	   m_observers.find(observer)->second |= highPower;
> +    else
> +	   m_observers.add(observer, highPower);

This code does twice as many hash table lookups as it needs to. The more
efficient way to do this is to unconditionally call add. The result from add
tells if a new element was added or not, and if it's not a new element you can
use the iterator to modify the existing hash map entry. The addResult.second is
true if the entry is new, and addResult.first is the iterator to the existing
or added hash map element.

I don't understand why or'ing with the high power boolean is correct. Can the
same observer be added, first with high accuracy and then later correcting its
request to ask only for lower accuracy? In that case wouldn't we want the
boolean to become false?

> +    if (m_client) {
> +	   if (!wasUsingHighPower && useHighPower())
> +	       m_client->setUseHighPower(true);
> +	   if (wasEmpty)
> +	       m_client->startUpdating();
> +    }

The code notices transitions in the high power flag in a way that involves
walking the hash map twice each time we add or remove an observer, which seems
needlessly inefficient. It would be better to do it in a way that didn't
require walking the hash table at all. One simple way to do that would be to
maintain a count of the number of observers that are requesting high power as
we modify the observer map. Another approach that would be simpler would be to
use two HashSet objects, one for all observers, and another for only observers
that are asking for high accuracy, instead of a single HashMap object. Then you
can simply check if the high accuracy set is empty or not.

I would suggest names like: isHighAccuracyRequested and
setHighAccuracyRequested rather than useHighPower and setUseHighPower.

I'm going to say review- because I think at least one of my comments above
should be done.