Client-based Geolocation does not pass high power option to controller and client

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Steve Block]

Client-based Geolocation does not pass high power option to controller and client. See Geolocation::startUpdating().

[Steve Block]

Created attachment 58267 [details]
Patch

[Steve Block]

Note that this patch depends upon Bug 40148, which is still in review.

[WebKit Review Bot]

Attachment 58267 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3227084

[Steve Block]

Created attachment 58457 [details]
Patch

[WebKit Review Bot]

Attachment 58457 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3224214

[Steve Block]

Created attachment 58467 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 58467 [details]
Patch

> -    HashSet<RefPtr<Geolocation> > m_observers;
> +    typedef HashMap<RefPtr<Geolocation>, bool> ObserversMap;

It feels a little strange that of all PositionOptions, only this one goes here. But I guess that's the way it is.

r=me.

[WebKit Commit Bot]

Comment on attachment 58467 [details]
Patch

Rejecting patch 58467 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
endertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
Testing 19099 test cases.
fast/dom/Geolocation/callback-exception.html -> crashed

Exiting early after 1 failures. 6531 tests run.
273.73s total testing time

6530 test cases (99%) succeeded
1 test case (<1%) crashed
1 test case (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/3338102

[Steve Block]

Created attachment 59935 [details]
Patch

[Darin Adler]

Comment on attachment 59935 [details]
Patch

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.

[Steve Block]

> 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.
I used the 'high power' terminology because the Geolocation V2 spec will adopt this. See http://www.w3.org/2008/geolocation/track/issues/6. However, you're right that it's probably best to remain consistent with 'enableHighAccuracy' for now.

> 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?
An observer calls addObserver each time it starts a new Geolocation request (a call to getCurrentPosition() or watchPosition()). Each of these can use a different value for enablehighAccuracy. The observer only calls removeObserver() when all of its requests are complete. Therefore, the controller has no way to know when the request that enabled high accuracy completes. So the best we can do is keep enablehighAccuracy set for that observer until it calls removeObserver().

This is not ideal, but the same is true of the non-client-based implementation. I have Bug 41341 to track this, but the change is quite invasive and I'd rather hold off on fixing it until the two implementations are merged. At that point, I'll add LayoutTests for enableHighAccuracy.

> 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.
Will do.

[Steve Block]

Created attachment 60005 [details]
Patch

[Jeremy Orlow]

Comment on attachment 60005 [details]
Patch

WebCore/ChangeLog:3
 +          Reviewed by Alexey Proskuryakov.
change

WebKit/mac/ChangeLog:3
 +          Reviewed by Alexey Proskuryakov.
ditto

WebKit/win/ChangeLog:3
 +          Reviewed by Alexey Proskuryakov.
and again

WebCore/page/GeolocationController.cpp:80
 +              m_client->setEnableHighAccuracy(false);
Why not just always set this?  Let it decide if it's a no-op or not.  I hate having state stored in multiple places when it's remotely possible to avoid it.



WebCore/page/GeolocationController.cpp:61
 +              m_client->setEnableHighAccuracy(true);
ditto

WebCore/page/GeolocationController.h:62
 +      ObserversSet m_observers;
I wonder if it'd be more clear to always have observers in just one set or the other.  I.e. s/m_observers/m_lowAccuracyObserver/?  Maybe not...but when I first looked at this code I was confused because I made this assumption.


I actually think using the high powered terminology in WebKit is the right answer since we can't really change that in the future and we know the spec is headed in that way.  In WebCore, maybe add fixmes and wait to do it until the spec changes?

Sorry for so much thrashing.  I'll try to do a follow up review quickly once you make these changes so you can get it in.

[Steve Block]

Created attachment 62583 [details]
Patch

[Steve Block]

> WebCore/ChangeLog:3
>  +          Reviewed by Alexey Proskuryakov.
> change
Done

> WebKit/mac/ChangeLog:3
>  +          Reviewed by Alexey Proskuryakov.
> ditto
Done

> WebKit/win/ChangeLog:3
>  +          Reviewed by Alexey Proskuryakov.
> and again
Done

> WebCore/page/GeolocationController.cpp:80
>  +              m_client->setEnableHighAccuracy(false);
> Why not just always set this?
Done

> WebCore/page/GeolocationController.cpp:61
>  +              m_client->setEnableHighAccuracy(true);
> ditto
Done

> WebCore/page/GeolocationController.h:62
>  +      ObserversSet m_observers;
> I wonder if it'd be more clear to always have observers in just one set or
> the other.
I think it's best as it is. I've added a comment to make it clear.

> I actually think using the high powered terminology in WebKit is the right
> answer since we can't really change that in the future and we know the spec
> is headed in that way.
I've added a comment about this. It's not a FIXME, as it's not clear when/if v2 will happen.

[Jeremy Orlow]

Comment on attachment 62583 [details]
Patch

WebCore/page/GeolocationControllerClient.h:39
 +      // We should update the terminology used in WebCore to describe this property
Add FIXME

WebCore/page/GeolocationController.cpp:77
 +              m_client->setEnableHighAccuracy(false);
You said "done" but this (and above) didn't change.

WebKit/mac/WebCoreSupport/WebGeolocationControllerClient.h:42
 +      void setEnableHighAccuracy(bool) { }
This should be the new terminology since it can't change easily once someone implements it.

WebKit/win/WebCoreSupport/WebGeolocationControllerClient.h:45
 +      virtual void setEnableHighAccuracy(bool) { }
ditto

WebCore/page/GeolocationControllerClient.h:41
 +      // http://www.w3.org/2008/geolocation/track/issues/6
Maybe suggest that webkit ports implementing it call it "highPower"

[Steve Block]

> WebCore/page/GeolocationControllerClient.h:39
>  +      // We should update the terminology used in WebCore to describe this property
> Add FIXME
OK, will do

> WebCore/page/GeolocationController.cpp:77
>  +              m_client->setEnableHighAccuracy(false);
> You said "done" but this (and above) didn't change.
I removed the code which checked whether we were previously using high accuracy as an optimisation. I now call setEnableHighAccuracy(false) every time the list of high accuracy observers becomes empty. There's no way to avoid having this logic in the controller. The client doesn't know which observer requested high accuracy. This is tracked only in the controller, which then commands the client appropriately. The only alternative would be to move all this logic to the client, but that would lead to code replication across platforms.

> WebKit/mac/WebCoreSupport/WebGeolocationControllerClient.h:42
>  +      void setEnableHighAccuracy(bool) { }
> This should be the new terminology since it can't change easily once someone implements it.
I don't think this is a good idea right now, as the future of the V2 spec isn't certain.

[Jeremy Orlow]

(In reply to comment #17)
> > WebCore/page/GeolocationControllerClient.h:39
> >  +      // We should update the terminology used in WebCore to describe this property
> > Add FIXME
> OK, will do
> 
> > WebCore/page/GeolocationController.cpp:77
> >  +              m_client->setEnableHighAccuracy(false);
> > You said "done" but this (and above) didn't change.
> I removed the code which checked whether we were previously using high accuracy as an optimisation. I now call setEnableHighAccuracy(false) every time the list of high accuracy observers becomes empty. There's no way to avoid having this logic in the controller. The client doesn't know which observer requested high accuracy. This is tracked only in the controller, which then commands the client appropriately. The only alternative would be to move all this logic to the client, but that would lead to code replication across platforms.
> 
> > WebKit/mac/WebCoreSupport/WebGeolocationControllerClient.h:42
> >  +      void setEnableHighAccuracy(bool) { }
> > This should be the new terminology since it can't change easily once someone implements it.
> I don't think this is a good idea right now, as the future of the V2 spec isn't certain.

Ok and ok.  Will take another look soon.

[Steve Block]

Created attachment 62681 [details]
Patch

[Jeremy Orlow]

Comment on attachment 62681 [details]
Patch

WebCore/page/GeolocationController.cpp:61
 +              m_client->startUpdating();
Is there any reason this can't always be called?

WebCore/page/GeolocationController.cpp:74
 +          if (m_observers.isEmpty())
Is there any reason this can't always be called?

r=me

[WebKit Commit Bot]

Comment on attachment 62681 [details]
Patch

Clearing flags on attachment: 62681

Committed r64126: <http://trac.webkit.org/changeset/64126>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.