[Webkit-unassigned] [Bug 40374] Client-based Geolocation does not pass high power option to controller and client

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=40374


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #59935|review?                     |review-
               Flag|                            |




--- Comment #10 from Darin Adler <darin at apple.com>  2010-06-28 16:25:56 PST ---
(From update of attachment 59935)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.