Add a mechanism to pause and resume geolocation requests.

Add a mechanism to pause and resume geolocation requests. Currently Android WebView is the only platform to make use of this new API. We implement this as a new API on the Browser-side GeolocationDispatcherHost.

In doing so:
 - Refactor the current LocationProvider such that it doesn't rely on ActivityStatus, updating LocationProviderTest accordingly
 - Introduce ContentViewLocationTest.java that verifies the LocationProvider implementation is paused and resumed when the ContentView is hidde/shown
 - Introduce a AwGeolocationTest that verifies the LocationProvider implementation is paused and resumed when the new API is invoked
 - Introduce LocationProviderFactory and a MockLocationProvider to avoid relying on the system location provider when running tests, as it's not possible to enable mock locations on Android user builds without physical access to the device.

BUG=b/11336074

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240719

[benm (inactive), 2013-11-07 21:07:50]

Hi joth, would you be able to take a look at this? Seems like this does what we
need to wire up onPause/onResume per AwContents for backwards compat.

PS1 is a copy of the pre-existing patch that wires up pause/resume geolocation
to pause/resumeTimers

PS2 ads the new logic, which again is mostly plumbing. It needs the Blink CL at
https://codereview.chromium.org/65353003/

The interesting guts of the CL is the refactoring inside
geolocation_dispatcher_host.cc

WDYT?

https://codereview.chromium.org/65273002/diff/30001/content/browser/geolocati...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/65273002/diff/30001/content/browser/geolocati...
content/browser/geolocation/geolocation_dispatcher_host.cc:74: // Updates the
|location_arbitrator_| with the currently required update
The geolocation_provider/arbitrator comment changes here are artifacts of
merging M30 -> trunk. Will revert them.

[joth, 2013-11-07 21:51:59]

size of change is a bit hairy, but I can see where it's headed and seems
reasonable.

https://codereview.chromium.org/65353003/ is not quite clear to me: one page can
have N embedded frames each with their own location watcher, but we're not
getting involved in that. So is there any need to have the IPC and blink
plumbing at all? Can we go from WebContents to routing ID and straight to the
GeolocationArbitrator?

Hmmm plus one more scenario to test: what if WebView.onPause() is called prior
to the page { registering the geolocaiton watch | receiving permission for
location updates }

https://codereview.chromium.org/65273002/diff/30001/content/browser/geolocati...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/65273002/diff/30001/content/browser/geolocati...
content/browser/geolocation/geolocation_dispatcher_host.cc:90: // Only used on
the IO thread.
(update comment)

https://codereview.chromium.org/65273002/diff/30001/content/browser/geolocati...
content/browser/geolocation/geolocation_dispatcher_host.cc:198:
geolocation_renderers_[render_view_id] = opts;
if we were previously paused we should now unpause

Should cover this with a test

https://codereview.chromium.org/65273002/diff/30001/content/browser/geolocati...
content/browser/geolocation/geolocation_dispatcher_host.cc:207:
geolocation_renderers_.erase(render_view_id);
if we just erased the only unpaused provider is should now pause

(again, cover in a test)

https://codereview.chromium.org/65273002/diff/30001/content/browser/geolocati...
content/browser/geolocation/geolocation_dispatcher_host.cc:242: void
GeolocationDispatcherHostImpl::RefreshHighAccuracy() {
this should probably be RefeshProviederOptions and have it resolve both the high
accuracy and paused-ness state

https://codereview.chromium.org/65273002/diff/30001/content/browser/geolocati...
content/browser/geolocation/geolocation_dispatcher_host.cc:246:
geolocation_provider_->RemoveLocationUpdateCallback(callback_);
can we just use RemoveLocationUpdateCallback instead of the new 
PauseAllCallbacks call, and AddLocationUpdateCallback instead of resume?

then your edit to the specific location providers can be reverted.

code here would become simples:-

bool register_for_updates = false;
bool use_high_accuracy = false;

for (it : geolocation_renderers_) {
  register_for_updates |= !(it->is_pause);
  use_high_accuracy |= it->high_accuracy;
  if (register_for_updates && use_high_accuracy)
    break;  // optimization
}

if (register_for_updates) {
  geolocation_provider_->AddLocationUpdateCallback(callback, use_high_accuract);
} else {
  geolocation_provider_->RemoveLocationUpdateCallback(callback_);
}

[joth, 2013-11-07 21:58:15]

https://codereview.chromium.org/65273002/diff/30001/content/browser/geolocati...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/65273002/diff/30001/content/browser/geolocati...
content/browser/geolocation/geolocation_dispatcher_host.cc:246:
geolocation_provider_->RemoveLocationUpdateCallback(callback_);
On 2013/11/07 21:52:00, joth wrote:
> can we just use RemoveLocationUpdateCallback instead of the new 
> PauseAllCallbacks call, and AddLocationUpdateCallback instead of resume?
> 
> then your edit to the specific location providers can be reverted.
> 
> code here would become simples:-
> 
> bool register_for_updates = false;
> bool use_high_accuracy = false;
> 
> for (it : geolocation_renderers_) {
>   register_for_updates |= !(it->is_pause);
>   use_high_accuracy |= it->high_accuracy;
>   if (register_for_updates && use_high_accuracy)
>     break;  // optimization
> }
> 
> if (register_for_updates) {
>   geolocation_provider_->AddLocationUpdateCallback(callback,
use_high_accuract);
> } else {
>   geolocation_provider_->RemoveLocationUpdateCallback(callback_);
> }

Actually my change above is needed otherwise this introduces a bug:
there is one GeolocationDispatcherHostImpl instance per render *process*. each
one can independently register for updates.

We can't call a global "pause all providers" from in here, as even if one render
process no longer needs updates, other render processes may still have
non-paused render views.

[Ben Murdoch, 2013-11-07 22:22:54]

Thanks joth!

On 7 Nov 2013 21:52, <joth@chromium.org> wrote:
>
> size of change is a bit hairy, but I can see where it's headed and seems
> reasonable.
>
> https://codereview.chromium.org/65353003/ is not quite clear to me: one
page can
> have N embedded frames each with their own location watcher, but we're not
> getting involved in that. So is there any need to have the IPC and blink
> plumbing at all? Can we go from WebContents to routing ID and straight to
the
> GeolocationArbitrator?

Yes quite possibly, that would cut down the plumbing somewhat and avoid the
blink change as you say.  But currently the geolocation dispatcher host is
an IPC message filter, so not sure how we could expose its API to
AwContents?

>
> Hmmm plus one more scenario to test: what if WebView.onPause() is called
prior
> to the page { registering the geolocaiton watch | receiving permission for
> location updates }

Hmm, interesting test case. I think that right now it would work, as I
early out of onPause in the geolocation dispatcher host if there are no
renderers using geolocation.

>
>
>
>
>
https://codereview.chromium.org/65273002/diff/30001/content/browser/geolocati...
> File content/browser/geolocation/geolocation_dispatcher_host.cc (right):
>
>
https://codereview.chromium.org/65273002/diff/30001/content/browser/geolocati...
> content/browser/geolocation/geolocation_dispatcher_host.cc:90: // Only
> used on the IO thread.
> (update comment)
>
>
https://codereview.chromium.org/65273002/diff/30001/content/browser/geolocati...
> content/browser/geolocation/geolocation_dispatcher_host.cc:198:
> geolocation_renderers_[render_view_id] = opts;
> if we were previously paused we should now unpause
>
> Should cover this with a test
>
>
https://codereview.chromium.org/65273002/diff/30001/content/browser/geolocati...
> content/browser/geolocation/geolocation_dispatcher_host.cc:207:
> geolocation_renderers_.erase(render_view_id);
> if we just erased the only unpaused provider is should now pause
>
> (again, cover in a test)
>
>
https://codereview.chromium.org/65273002/diff/30001/content/browser/geolocati...
> content/browser/geolocation/geolocation_dispatcher_host.cc:242: void
> GeolocationDispatcherHostImpl::RefreshHighAccuracy() {
> this should probably be RefeshProviederOptions and have it resolve both
> the high accuracy and paused-ness state
>
>
https://codereview.chromium.org/65273002/diff/30001/content/browser/geolocati...
> content/browser/geolocation/geolocation_dispatcher_host.cc:246:
> geolocation_provider_->RemoveLocationUpdateCallback(callback_);
> can we just use RemoveLocationUpdateCallback instead of the new
> PauseAllCallbacks call, and AddLocationUpdateCallback instead of resume?
>
> then your edit to the specific location providers can be reverted.
>
> code here would become simples:-
>
> bool register_for_updates = false;
> bool use_high_accuracy = false;
>
> for (it : geolocation_renderers_) {
>   register_for_updates |= !(it->is_pause);
>   use_high_accuracy |= it->high_accuracy;
>   if (register_for_updates && use_high_accuracy)
>     break;  // optimization
> }
>
> if (register_for_updates) {
>   geolocation_provider_->AddLocationUpdateCallback(callback,
> use_high_accuract);
> } else {
>   geolocation_provider_->RemoveLocationUpdateCallback(callback_);
> }
>
> https://codereview.chromium.org/65273002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[benm (inactive), 2013-11-07 22:27:51]

On 7 Nov 2013 21:58, <joth@chromium.org> wrote:
>
>
>
https://codereview.chromium.org/65273002/diff/30001/content/browser/geolocati...
> File content/browser/geolocation/geolocation_dispatcher_host.cc (right):
>
>
https://codereview.chromium.org/65273002/diff/30001/content/browser/geolocati...
> content/browser/geolocation/geolocation_dispatcher_host.cc:246:
> geolocation_provider_->RemoveLocationUpdateCallback(callback_);
> On 2013/11/07 21:52:00, joth wrote:
>>
>> can we just use RemoveLocationUpdateCallback instead of the new
>> PauseAllCallbacks call, and AddLocationUpdateCallback instead of
>
> resume?
>
>> then your edit to the specific location providers can be reverted.
>
>
>> code here would become simples:-
>
>
>> bool register_for_updates = false;
>> bool use_high_accuracy = false;
>
>
>> for (it : geolocation_renderers_) {
>>    register_for_updates |= !(it->is_pause);
>>    use_high_accuracy |= it->high_accuracy;
>>    if (register_for_updates && use_high_accuracy)
>>      break;  // optimization
>> }
>
>
>> if (register_for_updates) {
>>    geolocation_provider_->AddLocationUpdateCallback(callback,
>
> use_high_accuract);
>>
>> } else {
>>    geolocation_provider_->RemoveLocationUpdateCallback(callback_);
>> }
>
>
> Actually my change above is needed otherwise this introduces a bug:
> there is one GeolocationDispatcherHostImpl instance per render
> *process*. each one can independently register for updates.
>
> We can't call a global "pause all providers" from in here, as even if
> one render process no longer needs updates, other render processes may
> still have non-paused render views.
>

Ah, yes that's a very good point. The reason I had the pause/resume rather
than reusing start/stop was that I was intending this to be a lighter
weight operation then bringing it all down and then up again. But to be
honest I don't think it makes a huge difference in practise and would
dramatically simplify the change if we could get rid of all that plumbing
too, especially now if we control this at the dispatcher level where we
have information about needing to restart callbacks with high accuracy, or
not.

> https://codereview.chromium.org/65273002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Michael van Ouwerkerk, 2013-11-08 12:27:27]

https://codereview.chromium.org/65273002/diff/30001/content/common/geolocatio...
File content/common/geolocation_messages.h (right):

https://codereview.chromium.org/65273002/diff/30001/content/common/geolocatio...
content/common/geolocation_messages.h:75: int,
Comment what this is

https://codereview.chromium.org/65273002/diff/30001/content/common/geolocatio...
content/common/geolocation_messages.h:76: bool)
Comment what this is

[benm (inactive), 2013-11-08 19:26:30]

Hi guys, please take a look at PS6. I think we're almost there. This eliminates
the extra IPC from earlier patch sets, and obsoletes the Blink changes.

What do you think? Still needs a test.

Cheers!

[joth, 2013-11-08 20:41:14]

Like it. This approach seems more contained.

https://codereview.chromium.org/65273002/diff/250001/content/browser/geolocat...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/65273002/diff/250001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:222:
&(geolocation_renderers_[render_view_id]);
nit: capture the find() result in an iterator above and use it->second here, to
avoid double lookup (and potential bug that [] can create an empty entry)

https://codereview.chromium.org/65273002/diff/250001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:241: if
(AreAllRenderersPaused()) {
Do we really need to special case this AreAllRenderersPaused upfront?

The pseudo code I wrote in previous review should naturally deal with that case
too without the need for an extra method (which also implies walking the map
twice)


bool need_updates = false;
bool use_high_accuracy = false;

for (it : geolocation_renderers_) {
  need_updates |= !(it->is_pause);
  use_high_accuracy |= it->high_accuracy;
  if (register_for_updates && use_high_accuracy)
    break;  // optimization
}

if (need_updates) {
  if (!geolocation_provider_)
    geolocation_provider_ = GeolocationProviderImpl::GetInstance();
  geolocation_provider_->AddLocationUpdateCallback(callback, use_high_accuract);
} else {
  if (geolocation_provider_)
    geolocation_provider_->RemoveLocationUpdateCallback(callback_);
  geolocation_provider_ = NULL;
}



(I'm suspicious whether the geolocation_provider_ member serves any real
purpose, but that's a different discussion)

https://codereview.chromium.org/65273002/diff/250001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/65273002/diff/250001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.cc:849: base::Unretained(
GeolocationDispatcherHost is refcounted so the Unretained() is probably not
needed here.

https://codereview.chromium.org/65273002/diff/250001/content/public/browser/w...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/65273002/diff/250001/content/public/browser/w...
content/public/browser/web_contents.h:470: virtual void ResumeGeolocation() = 0;
do we need this on WebContents? Why not just add to the public RenderViewHost
base class?

tactically, as it's only needed on Android, it maybe simpler to add this to
ContentViewCore .java / .cc, as content_view_core_impl.cc can see other content
internal APIs and so could poke straight into
GeolocationDispatcherHost::PauseOrResume() etc. No need for cross-plat public
API?

[benm (inactive), 2013-11-08 21:10:28]

Thank you Joth!

https://codereview.chromium.org/65273002/diff/250001/content/browser/geolocat...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/65273002/diff/250001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:222:
&(geolocation_renderers_[render_view_id]);
On 2013/11/08 20:41:14, joth wrote:
> nit: capture the find() result in an iterator above and use it->second here,
to
> avoid double lookup (and potential bug that [] can create an empty entry)

Done.

https://codereview.chromium.org/65273002/diff/250001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:241: if
(AreAllRenderersPaused()) {
Gotcha, this makes sense. Sorry think I missed this comment with all the other
refactoring and shuffling. Cheers!

https://codereview.chromium.org/65273002/diff/250001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/65273002/diff/250001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.cc:849: base::Unretained(
On 2013/11/08 20:41:14, joth wrote:
> GeolocationDispatcherHost is refcounted so the Unretained() is probably not
> needed here.

Perfect, thanks.

https://codereview.chromium.org/65273002/diff/250001/content/public/browser/w...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/65273002/diff/250001/content/public/browser/w...
content/public/browser/web_contents.h:470: virtual void ResumeGeolocation() = 0;
On 2013/11/08 20:41:14, joth wrote:
> do we need this on WebContents? Why not just add to the public RenderViewHost
> base class?

i thought about this at first, but it seemed more a WebContents-y API than RVH.
But I really don't feel strongly either way.

> 
> tactically, as it's only needed on Android, it maybe simpler to add this to
> ContentViewCore .java / .cc, as content_view_core_impl.cc can see other
content
> internal APIs and so could poke straight into
> GeolocationDispatcherHost::PauseOrResume() etc. No need for cross-plat public
> API?

SGTM. no need for CVC.java though, is there? Can go straight to native from
AwContents native?

[joth, 2013-11-08 21:16:28]

On 2013/11/08 21:10:28, benm wrote:
> Thank you Joth!
> 
>
https://codereview.chromium.org/65273002/diff/250001/content/browser/geolocat...
> File content/browser/geolocation/geolocation_dispatcher_host.cc (right):
> 
>
https://codereview.chromium.org/65273002/diff/250001/content/browser/geolocat...
> content/browser/geolocation/geolocation_dispatcher_host.cc:222:
> &(geolocation_renderers_[render_view_id]);
> On 2013/11/08 20:41:14, joth wrote:
> > nit: capture the find() result in an iterator above and use it->second here,
> to
> > avoid double lookup (and potential bug that [] can create an empty entry)
> 
> Done.
> 
>
https://codereview.chromium.org/65273002/diff/250001/content/browser/geolocat...
> content/browser/geolocation/geolocation_dispatcher_host.cc:241: if
> (AreAllRenderersPaused()) {
> Gotcha, this makes sense. Sorry think I missed this comment with all the other
> refactoring and shuffling. Cheers!
> 
>
https://codereview.chromium.org/65273002/diff/250001/content/browser/renderer...
> File content/browser/renderer_host/render_view_host_impl.cc (right):
> 
>
https://codereview.chromium.org/65273002/diff/250001/content/browser/renderer...
> content/browser/renderer_host/render_view_host_impl.cc:849: base::Unretained(
> On 2013/11/08 20:41:14, joth wrote:
> > GeolocationDispatcherHost is refcounted so the Unretained() is probably not
> > needed here.
> 
> Perfect, thanks.
> 
>
https://codereview.chromium.org/65273002/diff/250001/content/public/browser/w...
> File content/public/browser/web_contents.h (right):
> 
>
https://codereview.chromium.org/65273002/diff/250001/content/public/browser/w...
> content/public/browser/web_contents.h:470: virtual void ResumeGeolocation() =
0;
> On 2013/11/08 20:41:14, joth wrote:
> > do we need this on WebContents? Why not just add to the public
RenderViewHost
> > base class?
> 
> i thought about this at first, but it seemed more a WebContents-y API than
RVH.
> But I really don't feel strongly either way.
> 
> > 
> > tactically, as it's only needed on Android, it maybe simpler to add this to
> > ContentViewCore .java / .cc, as content_view_core_impl.cc can see other
> content
> > internal APIs and so could poke straight into
> > GeolocationDispatcherHost::PauseOrResume() etc. No need for cross-plat
public
> > API?
> 
> SGTM. no need for CVC.java though, is there? Can go straight to native from
> AwContents native?

Ah true forgot we had  public content_view_core.h base class. That already has a
PauseVideo method so this looks like it would be a on a consistent path with
that.

[benm (inactive), 2013-11-10 15:23:55]

PTAL. Still no test, I couldn't work where best to add one. Let's chat about
this Michael.

Cheers

[joth, 2013-11-10 17:59:02]

lgtm w/ 2 suggestions

https://codereview.chromium.org/65273002/diff/380001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/65273002/diff/380001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:356: }
I think we could skip the RenderViewHostImpl altogether and go straight to
RenderProcessHost->geolocation_dispatcher here.

I think this would be simpler if you made this a single method taking a bool
right through the stack.

void ContentViewCoreImpl::SetGeolocationPaused(bool paused) {
  RenderViewHost* rvh  = web_contents_->GetRenderViewHost();
  if (rvh) {
    BrowserThread::PostTask(FROM_HERE, IO,
        &GeolocationDispatcherHost::PauseOrResume,
        static_cast<RenderProcessHostImpl*>(
            web_contents_->GetRenderProcessHost())->geolocation_dispatcher(),
        rvh->GetRoutingID(), 
        paused);
  }
}

https://codereview.chromium.org/65273002/diff/380001/content/browser/geolocat...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/65273002/diff/380001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:144: Send(new
GeolocationMsg_PositionUpdated(it->first, geoposition));
technically should only send  if (!it->second.is_paused)  ?

[Michael van Ouwerkerk, 2013-11-11 14:19:48]

https://codereview.chromium.org/65273002/diff/380001/content/browser/geolocat...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/65273002/diff/380001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:144: Send(new
GeolocationMsg_PositionUpdated(it->first, geoposition));
On 2013/11/10 17:59:02, joth wrote:
> technically should only send  if (!it->second.is_paused)  ?  

Agreed, I think that would be clearer. Otherwise the behavior of one view would
depend more closely on that of others. We already strange ties between them
through geolocation.

[benm (inactive), 2013-11-11 14:50:40]

thanks all!

https://codereview.chromium.org/65273002/diff/380001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/65273002/diff/380001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:356: }
On 2013/11/10 17:59:02, joth wrote:
> I think we could skip the RenderViewHostImpl altogether and go straight to
> RenderProcessHost->geolocation_dispatcher here.
> 
> I think this would be simpler if you made this a single method taking a bool
> right through the stack.
> 
> void ContentViewCoreImpl::SetGeolocationPaused(bool paused) {
>   RenderViewHost* rvh  = web_contents_->GetRenderViewHost();
>   if (rvh) {
>     BrowserThread::PostTask(FROM_HERE, IO,
>         &GeolocationDispatcherHost::PauseOrResume,
>         static_cast<RenderProcessHostImpl*>(
>             web_contents_->GetRenderProcessHost())->geolocation_dispatcher(),
>         rvh->GetRoutingID(), 
>         paused);
>   }
> }

Done.

https://codereview.chromium.org/65273002/diff/380001/content/browser/geolocat...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/65273002/diff/380001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:144: Send(new
GeolocationMsg_PositionUpdated(it->first, geoposition));
Right. Otherwise in this case as long as one renderer using geolocation is not
paused all renderers will get updates.

[benm (inactive), 2013-11-13 20:07:15]

Current plan for testing this is to add an AwGeolocationTest that will be
inspired by the WebView Geolocation CTS test, and trigger
AwContents.onPause/onResume and check that location updates are stopped and
started as appropriate.

[benm (inactive), 2013-11-27 20:12:22]

Latest updates on this puppy (PS9 still needs some work but almost there...
again :)

 - Geolocation is currently broken for the WebView due to recent ActivityStatus
based refactorings inside LocationProvider.java
 - The recent changes in Blink around page visibility mean that Clank doesn't
need LocationProvider to listen to ActivityState, anyway
 - Because of this, I have refactored LocationProvider to *not* depend on
ActivityStatus at all.
 - This means I've removed the ActivityState based test cases from
LocationProviderTest.java
 - I have added an android_webview GeolocationTest that provides a basic smoke
test for geolocation in the WebView, and a test to ensure the onPause/onResume
works with respect to geolocation.
 - To implement this test, I had to add a slightly ugly
IsGeolocationActiveForTesting method to ContentViewCore. Suggestions for
replacing it's functionality with something that doesn't require us Wait()ing
(or reassurance that this isn't so terrible in a test method :-) ), are welcome!


Still to do to finish this up:
 - Remember the pause/resumed state of the WebView if onPause/onResume are
called before a navigation, and plumb it through to GeolocationDispatcher as
appropriate once RVH comes up.
 - Replace the removed cases from LocationProviderTest with something akin to
android_webviews GeolocationTest, but
s/AwContents.onPause/ContentViewCore.onHide/ to trigger the page visibility code
path in Blink.

[benm (inactive), 2013-11-27 20:29:30]

https://codereview.chromium.org/65273002/diff/650001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java
(right):

https://codereview.chromium.org/65273002/diff/650001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:249:
protected AwBrowserContext mBrowserContext =
revert me

[benm (inactive), 2013-11-29 17:29:22]

+mkosiba for eyes on the new AwGeolocationTest.

It works perfectly on my local machine/device, however the bots don't have the
ALLOW_MOCK_LOCATION permission so I'm at a bit of a loss as to how we can
actually run these :-/

[Michael van Ouwerkerk, 2013-11-29 19:09:58]

On 2013/11/29 17:29:22, benm wrote:
> +mkosiba for eyes on the new AwGeolocationTest.
> 
> It works perfectly on my local machine/device, however the bots don't have the
> ALLOW_MOCK_LOCATION permission so I'm at a bit of a loss as to how we can
> actually run these :-/

You could give the bots the ALLOW_MOCK_LOCATION permission, perhaps?

[benm (inactive), 2013-11-29 23:16:59]

On 2013/11/29 19:09:58, Michael van Ouwerkerk wrote:
> On 2013/11/29 17:29:22, benm wrote:
> > +mkosiba for eyes on the new AwGeolocationTest.
> > 
> > It works perfectly on my local machine/device, however the bots don't have
the
> > ALLOW_MOCK_LOCATION permission so I'm at a bit of a loss as to how we can
> > actually run these :-/
> 
> You could give the bots the ALLOW_MOCK_LOCATION permission, perhaps?

Sadly it's not as easy as that ... As well as the permission, the device needs
to have the System.Secure.ALLOW_MOCK_LOCATION bit set, but there's no way for a
(standard) app to write to this permission. The only way I know of is to
physically interact with the device and enable the setting by clicking through
some UI in the systems settings app ... or run the tests on a non-user build
(where the setting is true by default). I believe we always run on user builds
upstream :-/

[mkosiba (inactive), 2013-12-02 10:49:03]

On 2013/11/29 17:29:22, benm wrote:
> +mkosiba for eyes on the new AwGeolocationTest.
> 
> It works perfectly on my local machine/device, however the bots don't have the
> ALLOW_MOCK_LOCATION permission so I'm at a bit of a loss as to how we can
> actually run these :-/

From a really quick glance it looks like the best approach would be to not use
the Android platform location mocking, since there is no easy way to script
enabling
it.

One place that might be easy to hook into is the
org.chromium.content.browser.LocationProvider
It already has a LocationProviderImpl split out so it looks like the ideal place
for
injecting a TestLocationProviderImpl.

[mkosiba (inactive), 2013-12-02 10:49:31]

https://codereview.chromium.org/65273002/diff/750001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java
(right):

https://codereview.chromium.org/65273002/diff/750001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java:30:
private static final String RAW_HTML =
nit, minimally shorter:
  CommonResources.makeHtmlPageFrom(
    "<title> ...."
    ...
    "</script>", "");

[benm (inactive), 2013-12-03 17:00:27]

On 2013/12/02 10:49:03, mkosiba wrote:
> On 2013/11/29 17:29:22, benm wrote:
> > +mkosiba for eyes on the new AwGeolocationTest.
> > 
> > It works perfectly on my local machine/device, however the bots don't have
the
> > ALLOW_MOCK_LOCATION permission so I'm at a bit of a loss as to how we can
> > actually run these :-/
> 
> From a really quick glance it looks like the best approach would be to not use
> the Android platform location mocking, since there is no easy way to script
> enabling
> it.
> 
> One place that might be easy to hook into is the
> org.chromium.content.browser.LocationProvider
> It already has a LocationProviderImpl split out so it looks like the ideal
place
> for
> injecting a TestLocationProviderImpl.

Thanks Marcin, great idea. This works well. It does mean that if new logic is
added to the real LocationProvider then we won't test that to ensure it's not
breaking WebView, but certainly better than nothing and a great starting point
to get this change landed. (And we have CTS to check that real geolocation is
actually working too).

[benm (inactive), 2013-12-05 17:03:58]

Hi reviewers,

I think I'm pretty happy with PS18; it resolves the issues the WebView has been
having and adds the tests I wanted too. Would you mind taking another look now?

Thanks!

[mkosiba (inactive), 2013-12-09 16:26:07]

lgtm

https://codereview.chromium.org/65273002/diff/830001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/LocationProvider.java
(right):

https://codereview.chromium.org/65273002/diff/830001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LocationProvider.java:42:
public static void enableMockLocationProvider() {
nit: I think it would be a bit cleaner if the LocationProvder could be
configured to take a MockLocationProviderImplFactory rather than having a mock
impl baked in directly. It would require adding a test-only Delegate to enable
calling nativeNewLocationAvailable, so I'm not sure we'd save any lines of code
but we'd be shipping less test-only code with the product.

[benm (inactive), 2013-12-11 11:01:27]

+bulach, can you take a look please? Thanks!

[bulach, 2013-12-11 13:29:26]

I have a couple of meetings right now, some comments so far, will finish in
~2h... I quite like the general direction, it's mostly nits and small
suggestions! thanks a ton for doing this!!

https://codereview.chromium.org/65273002/diff/870001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java
(right):

https://codereview.chromium.org/65273002/diff/870001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java:111:
@SmallTest
nit: I think all of these tests are at least Medium... Small is normally just a
pure unit-test..

https://codereview.chromium.org/65273002/diff/870001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java:149:
return getPositionCountFromJS() > 3;
nit: isn't > 0 enough? or if you really want more than one, perhaps > 1?
3 seems...arbitrary :) ditto for the other places..

https://codereview.chromium.org/65273002/diff/870001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/65273002/diff/870001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:16: #include
"base/synchronization/waitable_event.h"
nit: no longer needed

https://codereview.chromium.org/65273002/diff/870001/content/browser/geolocat...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/65273002/diff/870001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:132:
base::Unretained(geolocation_provider_), callback_));
I think GeolocationDispatcherHostImpl lives in the IO thread, right?
the problem with posting the task here is that GeolocationProvider would have
still have the callback pointing to |this| here... why is this post needed?

https://codereview.chromium.org/65273002/diff/870001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:229:
opts->high_accuracy = enable_high_accuracy;
nit: unindent, but tbh, perhaps just:

it->second.opts.high_accuracy = enable_high_accuracy;

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java
(right):

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java:24:
public interface LocationProviderImpl {
nit: an interface can't be an Impl.. since it's scoped to this class, just call
it LocationProvider

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java:102:
LocationProvider.nativeNewLocationAvailable(
nit: we don't normally expose native from other classes..

[mkosiba (inactive), 2013-12-11 15:47:43]

https://codereview.chromium.org/65273002/diff/870001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java
(right):

https://codereview.chromium.org/65273002/diff/870001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java:111:
@SmallTest
On 2013/12/11 13:29:27, bulach wrote:
> nit: I think all of these tests are at least Medium... Small is normally just
a
> pure unit-test..

hmm.. I've been using Small for most of the integration tests in the
android_webview/ folder because all of them easily fit in the timeout value set
for Small (they're like > 10x faster).
I thought Medium was for tests that did some non-trivial amount of work.

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java
(right):

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java:24:
public interface LocationProviderImpl {
On 2013/12/11 13:29:27, bulach wrote:
> nit: an interface can't be an Impl.. since it's scoped to this class, just
call
> it LocationProvider

or LocationProviderProvider :D

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewLocationTest.java
(right):

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewLocationTest.java:141:
public void testWatchHideNewWatchShow() throws Throwable {
Good tests! I think the only combination you didn't check is that if you disable
geolocation when paused it doesn't get re-enabled after resuming.

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewLocationTest.java:158:

uber-nit: extra newline

[bulach, 2013-12-11 15:48:39]

lgtm, thanks! just a few more nits and suggestions...

I think the most important comments are re: the naming of the interface (remove
Impl) and outer class (append Adapter), and the post task to remove callback,
which seems wrong.. feel free to go ahead once those are addressed! thanks!

https://codereview.chromium.org/65273002/diff/870001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java
(right):

https://codereview.chromium.org/65273002/diff/870001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java:23:
private static final long TEST_TIMEOUT = 20000L;
nit: looks huge! would 5secs work? also, "_MS" here and below.

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/LocationProvider.java
(right):

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LocationProvider.java:29:
public class LocationProvider {
I think this would now be better as LocationProviderAdapter: it matches the .cc
side, and better differentiates between the new "real" LocationProvider and its
factory... wdyt?

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LocationProvider.java:84:
public static native void nativeNewLocationAvailable(
just realized these should've been private...

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java
(right):

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java:51:
private static class SystemLocationProviderImpl
nit: and this would be just LocationProviderImpl

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewLocationTest.java
(right):

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewLocationTest.java:140:

nit: maybe @SmallTest / @Feature

https://codereview.chromium.org/65273002/diff/870001/content/public/test/andr...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/MockLocationProvider.java
(right):

https://codereview.chromium.org/65273002/diff/870001/content/public/test/andr...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/MockLocationProvider.java:37:
if (mIsRunning) return;
nit: would be clearer to move this check first.

[benm (inactive), 2013-12-11 20:08:45]

Thanks guys!

I think all comments are addressed other than the PostTask, which I need to
remind myself about...

https://codereview.chromium.org/65273002/diff/870001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java
(right):

https://codereview.chromium.org/65273002/diff/870001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java:23:
private static final long TEST_TIMEOUT = 20000L;
On 2013/12/11 15:48:39, bulach wrote:
> nit: looks huge! would 5secs work? also, "_MS" here and below.

Done.

https://codereview.chromium.org/65273002/diff/870001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java:111:
@SmallTest
On 2013/12/11 13:29:27, bulach wrote:
> nit: I think all of these tests are at least Medium... Small is normally just
a
> pure unit-test..

Done.

https://codereview.chromium.org/65273002/diff/870001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java:149:
return getPositionCountFromJS() > 3;
On 2013/12/11 13:29:27, bulach wrote:
> nit: isn't > 0 enough? or if you really want more than one, perhaps > 1?
> 3 seems...arbitrary :) ditto for the other places..

3 was completely arbitrary :)

I wanted to make sure there was at least 1, so will change to that.

https://codereview.chromium.org/65273002/diff/870001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/65273002/diff/870001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:16: #include
"base/synchronization/waitable_event.h"
On 2013/12/11 13:29:27, bulach wrote:
> nit: no longer needed

Done.

https://codereview.chromium.org/65273002/diff/870001/content/browser/geolocat...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/65273002/diff/870001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:132:
base::Unretained(geolocation_provider_), callback_));
On 2013/12/11 13:29:27, bulach wrote:
> I think GeolocationDispatcherHostImpl lives in the IO thread, right?
> the problem with posting the task here is that GeolocationProvider would have
> still have the callback pointing to |this| here... why is this post needed?

hmm... I need to double check this again, IIRC calling it directly was causing
some tests to fail.

https://codereview.chromium.org/65273002/diff/870001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:229:
opts->high_accuracy = enable_high_accuracy;
On 2013/12/11 13:29:27, bulach wrote:
> nit: unindent, but tbh, perhaps just:
> 
> it->second.opts.high_accuracy = enable_high_accuracy;

Done.

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/LocationProvider.java
(right):

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LocationProvider.java:29:
public class LocationProvider {
On 2013/12/11 15:48:39, bulach wrote:
> I think this would now be better as LocationProviderAdapter: it matches the
.cc
> side, and better differentiates between the new "real" LocationProvider and
its
> factory... wdyt?

SGTM, done

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LocationProvider.java:84:
public static native void nativeNewLocationAvailable(
On 2013/12/11 15:48:39, bulach wrote:
> just realized these should've been private...

OK, made them private but added public static methods to wrap them.

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java
(right):

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java:24:
public interface LocationProviderImpl {
On 2013/12/11 13:29:27, bulach wrote:
> nit: an interface can't be an Impl.. since it's scoped to this class, just
call
> it LocationProvider

Done.

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java:51:
private static class SystemLocationProviderImpl
On 2013/12/11 15:48:39, bulach wrote:
> nit: and this would be just LocationProviderImpl

Done.

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java:51:
private static class SystemLocationProviderImpl
On 2013/12/11 15:48:39, bulach wrote:
> nit: and this would be just LocationProviderImpl

Done.

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java:102:
LocationProvider.nativeNewLocationAvailable(
On 2013/12/11 13:29:27, bulach wrote:
> nit: we don't normally expose native from other classes..

Yeah, I thought it was a little odd that the native function was public in
LocationProvider, but ran with it :)

Fixed.

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewLocationTest.java
(right):

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewLocationTest.java:140:

On 2013/12/11 15:48:39, bulach wrote:
> nit: maybe @SmallTest / @Feature

Done.

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewLocationTest.java:141:
public void testWatchHideNewWatchShow() throws Throwable {
On 2013/12/11 15:47:44, mkosiba wrote:
> Good tests! I think the only combination you didn't check is that if you
disable
> geolocation when paused it doesn't get re-enabled after resuming.

Thanks!

good idea, added.

https://codereview.chromium.org/65273002/diff/870001/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewLocationTest.java:158:

On 2013/12/11 15:47:44, mkosiba wrote:
> uber-nit: extra newline

Done.

https://codereview.chromium.org/65273002/diff/870001/content/public/test/andr...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/MockLocationProvider.java
(right):

https://codereview.chromium.org/65273002/diff/870001/content/public/test/andr...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/MockLocationProvider.java:37:
if (mIsRunning) return;
On 2013/12/11 15:48:39, bulach wrote:
> nit: would be clearer to move this check first.

Done.

[benm (inactive), 2013-12-11 20:50:01]

+sky for content/browser/renderer_host/render_process_host_impl.*

[sky, 2013-12-11 21:40:58]

LGTM

[Michael van Ouwerkerk, 2013-12-12 14:45:02]

lgtm

https://codereview.chromium.org/65273002/diff/910001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/65273002/diff/910001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:269:
PauseOrResumeGeolocation(true);
This line actually doesn't read well. Does 'true' pause or resume it? Maybe
PauseGeolocation(true) would be clearer. I don't mean to push you in the
direction of a PAUSED NOT_PAUSED enum :-)

https://codereview.chromium.org/65273002/diff/910001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java
(right):

https://codereview.chromium.org/65273002/diff/910001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java:26:
private static LocationProviderFactory.LocationProvider sProviderImpl;
Is there documentation somewhere indicating that this must be a singleton?
Presumably because it's expensive.

[benm (inactive), 2013-12-12 18:24:03]

Thanks Michael!

https://codereview.chromium.org/65273002/diff/910001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/65273002/diff/910001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:269:
PauseOrResumeGeolocation(true);
On 2013/12/12 14:45:03, Michael van Ouwerkerk wrote:
> This line actually doesn't read well. Does 'true' pause or resume it? Maybe
> PauseGeolocation(true) would be clearer. I don't mean to push you in the
> direction of a PAUSED NOT_PAUSED enum :-)

Yeah, it's kinda clunky I agree... but when taken in conjunction with the line
above maybe it's OK? Would PauseOrResumeGeolocation(geolocation_needs_pause_) be
any better?

https://codereview.chromium.org/65273002/diff/910001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java
(right):

https://codereview.chromium.org/65273002/diff/910001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java:26:
private static LocationProviderFactory.LocationProvider sProviderImpl;
On 2013/12/12 14:45:03, Michael van Ouwerkerk wrote:
> Is there documentation somewhere indicating that this must be a singleton?
> Presumably because it's expensive.

No, I don't think it's documented anywhere; it's just so that we are able to
inject a MockProvider for the tests.

[commit-bot: I haz the power, 2013-12-13 11:40:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benm@chromium.org/65273002/930001

[commit-bot: I haz the power, 2013-12-13 15:37:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benm@chromium.org/65273002/950001

[commit-bot: I haz the power, 2013-12-13 19:19:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benm@chromium.org/65273002/950001

[commit-bot: I haz the power, 2013-12-13 19:43:54]

Change committed as 240719