Stop using [Client=...] feature of Mojom for GeolocationService

Stop using [Client=...] feature of Mojom for GeolocationService

Change GeolocationService to instead have a QueryNextPosition() method that clients can use to poll for changes to location. The first call will complete immediately, reporting the current location. After that, QueryNextPosition() will "hang" until there is a change to report.

This also solves a defect (anti-pattern) with the older design, which could result in the service consuming a lot of memory. That could happen if the client failed to consume any of the OnLocationUpdate messages. Eventually, the pipe would back up, and queuing of MojoWriteMessage calls would occur in the service's process. By switching to a polling design, this problem is eliminated.

See further description of the anti-pattern here: https://groups.google.com/a/chromium.org/forum/#!topic/mojo-dev/pSNDDN3gpFo

Committed: https://crrev.com/317e773e3499fe6a19871b5498a5c6164d972316
Cr-Commit-Position: refs/heads/master@{#311217}

[darin (slow to review), 2015-01-08 19:12:42]

darin@chromium.org changed reviewers:
+ blundell@chromium.org, tsepez@chromium.org

[Tom Sepez, 2015-01-08 22:11:57]

Messages LGTM

[blundell, 2015-01-09 07:37:26]

https://codereview.chromium.org/841133002/diff/1/content/browser/geolocation/...
File content/browser/geolocation/geolocation_service_impl.h (right):

https://codereview.chromium.org/841133002/diff/1/content/browser/geolocation/...
content/browser/geolocation/geolocation_service_impl.h:54: base::Closure
update_callback_;
I should have documented this variable. Could you do it now, something like

"Callback that allows the instantiator of this class to be notified on position
updates".

https://codereview.chromium.org/841133002/diff/1/content/browser/geolocation/...
content/browser/geolocation/geolocation_service_impl.h:56:
std::vector<PositionCallback> position_callbacks_;
Hmm, we now have |update_callback_| and |position_callbacks_|, which seems
confusing (at least it was confusing to me, and I wrote this code :). What about
|delegate_update_callback_| and |one_shot_update_callbacks_| or
|client_update_callbacks_| or |query_update_callbacks_|?

Related, could you add documentation to this variable?

https://codereview.chromium.org/841133002/diff/1/content/common/geolocation_s...
File content/common/geolocation_service.mojom (right):

https://codereview.chromium.org/841133002/diff/1/content/common/geolocation_s...
content/common/geolocation_service.mojom:16: // call to QueryNextPosition on
this instance. Position updates may be
Nit: What is the meaning of "immediately if this is the first call to
QueryNextPosition on this instance"? The service starts out with
has_position_to_report_ = false.

https://codereview.chromium.org/841133002/diff/1/content/renderer/geolocation...
File content/renderer/geolocation_dispatcher.cc (right):

https://codereview.chromium.org/841133002/diff/1/content/renderer/geolocation...
content/renderer/geolocation_dispatcher.cc:118: QueryNextPosition();
Nit: I think the code would be more clear by putting QueryNextPosition() at the
end of this function...logically the client is processing the current update and
then asking for the next update, even if physically there isn't an ordering
requirement between the two tasks.

[blundell, 2015-01-09 07:41:53]

Mounir:

FYI, as you were asking about removal of Client interfaces. Note especially the
avoidance of the straightforward refactoring to an AddObserver()-like pattern
for reasons that Darin outlines in the CL and linked post.

[darin (slow to review), 2015-01-09 18:00:44]

On 2015/01/09 07:37:26, blundell wrote:
>
https://codereview.chromium.org/841133002/diff/1/content/browser/geolocation/...
> File content/browser/geolocation/geolocation_service_impl.h (right):
> 
>
https://codereview.chromium.org/841133002/diff/1/content/browser/geolocation/...
> content/browser/geolocation/geolocation_service_impl.h:54: base::Closure
> update_callback_;
> I should have documented this variable. Could you do it now, something like
> 
> "Callback that allows the instantiator of this class to be notified on
position
> updates".
> 
>
https://codereview.chromium.org/841133002/diff/1/content/browser/geolocation/...
> content/browser/geolocation/geolocation_service_impl.h:56:
> std::vector<PositionCallback> position_callbacks_;
> Hmm, we now have |update_callback_| and |position_callbacks_|, which seems
> confusing (at least it was confusing to me, and I wrote this code :). What
about
> |delegate_update_callback_| and |one_shot_update_callbacks_| or
> |client_update_callbacks_| or |query_update_callbacks_|?
> 
> Related, could you add documentation to this variable?
> 
>
https://codereview.chromium.org/841133002/diff/1/content/common/geolocation_s...
> File content/common/geolocation_service.mojom (right):
> 
>
https://codereview.chromium.org/841133002/diff/1/content/common/geolocation_s...
> content/common/geolocation_service.mojom:16: // call to QueryNextPosition on
> this instance. Position updates may be
> Nit: What is the meaning of "immediately if this is the first call to
> QueryNextPosition on this instance"? The service starts out with
> has_position_to_report_ = false.

It means we won't wait for the device location to actually change. We'll report
the initial value as though it were a change. I can try to improve the wording
accordingly.


> 
>
https://codereview.chromium.org/841133002/diff/1/content/renderer/geolocation...
> File content/renderer/geolocation_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/841133002/diff/1/content/renderer/geolocation...
> content/renderer/geolocation_dispatcher.cc:118: QueryNextPosition();
> Nit: I think the code would be more clear by putting QueryNextPosition() at
the
> end of this function...logically the client is processing the current update
and
> then asking for the next update, even if physically there isn't an ordering
> requirement between the two tasks.

Oh, I just did that because I wasn't sure what side-effects calling a method on
controller_ might have. I can't tell that it isn't going to delete |this|.

[darin (slow to review), 2015-01-09 20:09:43]

I'm not that bothered by "update_callback". In the context of the class' public
API, it seems sensible. I added some more comments to help clarify the purpose
of the callback.

[blundell, 2015-01-12 18:35:00]

LGTM

[darin (slow to review), 2015-01-13 04:49:54]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2015-01-13 04:51:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/841133002/40001

[commit-bot: I haz the power, 2015-01-13 04:56:46]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-13 04:58:20]

Patchset 3 (id:??) landed as
https://crrev.com/317e773e3499fe6a19871b5498a5c6164d972316
Cr-Commit-Position: refs/heads/master@{#311217}