Bug 40374

Created attachment 58267 [details] Patch

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

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

Created attachment 58457 [details] Patch

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

Created attachment 58467 [details] Patch

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.

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

Created attachment 59935 [details] Patch

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.

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

Created attachment 60005 [details] Patch

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.

Created attachment 62583 [details] Patch

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

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"

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

(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.

Created attachment 62681 [details] Patch

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

Comment on attachment 62681 [details] Patch Clearing flags on attachment: 62681 Committed r64126: <http://trac.webkit.org/changeset/64126>

All reviewed patches have been landed. Closing bug.