Dispatch geolocation IPCs on the UI thread. Aside from simplifying the code to avoid a lot of threa…

Dispatch geolocation IPCs on the UI thread. Aside from simplifying the code to avoid a lot of thread hops, this sets things up for converting the code to work on RenderFrames instead of RenderViews.

In the process I also switched the code to use CallbackList which simplifies the registration.

BUG=304341
R=benm@chromium.org, brettw@chromium.org, bulach@chromium.org, caitkp@chromium.org, isherman@chromium.org, mvanouwerkerk@chromium.org, nasko@chromium.org

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

[jam, 2014-05-08 07:12:40]

mvanouwerkerk: main reviewer
caitkp: base/callback_list.h
benm: android_webview and the changes to pausing a request which you added

[jam, 2014-05-08 07:13:15]

+site-isolation-reviews

[Michael van Ouwerkerk, 2014-05-08 13:20:00]

Nice! That does clean up a lot.

Adding Marcus Bulach as additional Geolocation reviewer. I worry that there are
some subtle changes here.

https://codereview.chromium.org/273523007/diff/60001/base/callback_list.h
File base/callback_list.h (right):

https://codereview.chromium.org/273523007/diff/60001/base/callback_list.h#new...
base/callback_list.h:3: // DO NOT EDIT BY HAND!!!
Where are the corresponding .pump changes?

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

https://codereview.chromium.org/273523007/diff/60001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:193: void
GeolocationDispatcherHost::OnLocationUpdate(
nit: if this method remains before OnRequestPermission the diff might read more
easily

https://codereview.chromium.org/273523007/diff/60001/content/browser/geolocat...
File content/browser/geolocation/geolocation_dispatcher_host.h (right):

https://codereview.chromium.org/273523007/diff/60001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.h:58:
scoped_ptr<content::GeolocationProvider::Subscription>
nit: no need for content::

https://codereview.chromium.org/273523007/diff/60001/content/browser/geolocat...
File content/browser/geolocation/geolocation_provider_impl.cc (right):

https://codereview.chromium.org/273523007/diff/60001/content/browser/geolocat...
content/browser/geolocation/geolocation_provider_impl.cc:27:
scoped_ptr<GeolocationProvider::Subscription> rv;
Nit: s/rv/subscription/

https://codereview.chromium.org/273523007/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/273523007/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:17: #include
"content/browser/geolocation/geolocation_dispatcher_host.h"
Delete this as it's no longer used.

https://codereview.chromium.org/273523007/diff/60001/content/common/geolocati...
File content/common/geolocation_messages.h (right):

https://codereview.chromium.org/273523007/diff/60001/content/common/geolocati...
content/common/geolocation_messages.h:45: // The |bridge_id| representing |host|
is requesting permission to access
Judging from the comment for the next message: s/host/GURL/

https://codereview.chromium.org/273523007/diff/60001/content/public/browser/g...
File content/public/browser/geolocation_provider.h (right):

https://codereview.chromium.org/273523007/diff/60001/content/public/browser/g...
content/public/browser/geolocation_provider.h:18: //   receiving updates until
unregistered by RemoveLocationUpdateCallback().
There is no RemoveLocationUpdateCallback method anymore.

[jam, 2014-05-08 15:04:57]

https://codereview.chromium.org/273523007/diff/60001/base/callback_list.h
File base/callback_list.h (right):

https://codereview.chromium.org/273523007/diff/60001/base/callback_list.h#new...
base/callback_list.h:3: // DO NOT EDIT BY HAND!!!
On 2014/05/08 13:20:02, Michael van Ouwerkerk wrote:
> Where are the corresponding .pump changes?

ah, I missed that message so I was editing this file directly. Thanks, I've
updated the .pump instead and ran the script to update this file

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

https://codereview.chromium.org/273523007/diff/60001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:193: void
GeolocationDispatcherHost::OnLocationUpdate(
On 2014/05/08 13:20:02, Michael van Ouwerkerk wrote:
> nit: if this method remains before OnRequestPermission the diff might read
more
> easily

Done.

https://codereview.chromium.org/273523007/diff/60001/content/browser/geolocat...
File content/browser/geolocation/geolocation_dispatcher_host.h (right):

https://codereview.chromium.org/273523007/diff/60001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.h:58:
scoped_ptr<content::GeolocationProvider::Subscription>
On 2014/05/08 13:20:02, Michael van Ouwerkerk wrote:
> nit: no need for content::

Done.

https://codereview.chromium.org/273523007/diff/60001/content/browser/geolocat...
File content/browser/geolocation/geolocation_provider_impl.cc (right):

https://codereview.chromium.org/273523007/diff/60001/content/browser/geolocat...
content/browser/geolocation/geolocation_provider_impl.cc:27:
scoped_ptr<GeolocationProvider::Subscription> rv;
On 2014/05/08 13:20:02, Michael van Ouwerkerk wrote:
> Nit: s/rv/subscription/

Done.

https://codereview.chromium.org/273523007/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/273523007/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:17: #include
"content/browser/geolocation/geolocation_dispatcher_host.h"
On 2014/05/08 13:20:02, Michael van Ouwerkerk wrote:
> Delete this as it's no longer used.

Done.

https://codereview.chromium.org/273523007/diff/60001/content/common/geolocati...
File content/common/geolocation_messages.h (right):

https://codereview.chromium.org/273523007/diff/60001/content/common/geolocati...
content/common/geolocation_messages.h:45: // The |bridge_id| representing |host|
is requesting permission to access
On 2014/05/08 13:20:02, Michael van Ouwerkerk wrote:
> Judging from the comment for the next message: s/host/GURL/

I'm guessing that comment was meant to refer to the url as the host; and wasn't
referring to the parameter type (since we don't have a "Host" class). since this
is the existing comment, and to me it's not unclear, let's leave as is.

https://codereview.chromium.org/273523007/diff/60001/content/public/browser/g...
File content/public/browser/geolocation_provider.h (right):

https://codereview.chromium.org/273523007/diff/60001/content/public/browser/g...
content/public/browser/geolocation_provider.h:18: //   receiving updates until
unregistered by RemoveLocationUpdateCallback().
On 2014/05/08 13:20:02, Michael van Ouwerkerk wrote:
> There is no RemoveLocationUpdateCallback method anymore.

Done.

[benm (inactive), 2014-05-08 15:10:46]

aw changes look fine (but could we add the thread check back in case something
changes in the future? It's important that these are on the UI thread for the
webview)

Still just reminding myself about how the pause/resume thing worked, added tests
for it thankfully ;-)

https://codereview.chromium.org/273523007/diff/60001/android_webview/native/a...
File android_webview/native/aw_geolocation_permission_context.cc (left):

https://codereview.chromium.org/273523007/diff/60001/android_webview/native/a...
android_webview/native/aw_geolocation_permission_context.cc:27:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
can we keep this DCHECK?

https://codereview.chromium.org/273523007/diff/60001/android_webview/native/a...
android_webview/native/aw_geolocation_permission_context.cc:72:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
ditto, can we keep this?

[jam, 2014-05-08 15:54:31]

https://codereview.chromium.org/273523007/diff/60001/android_webview/native/a...
File android_webview/native/aw_geolocation_permission_context.cc (left):

https://codereview.chromium.org/273523007/diff/60001/android_webview/native/a...
android_webview/native/aw_geolocation_permission_context.cc:27:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2014/05/08 15:10:47, benm wrote:
> can we keep this DCHECK?

i had removed it for two reasons:
-i prefer embedders of content don't check that content is calling on the right
thread; these kind of checks should be done in content and not at every embedder
-the web_contents parameter now should make it obvious that this is called on
the UI thread

I think a better check would be in AwContents::FromID? I added one there
(hopefully it doesn't trigger!). I also removed the one in AwContents::FromID
since RenderViewHost::FromID checks that.

I defer to you since you're an owner there, if you're not convinced :)

[jam, 2014-05-08 17:23:37]

+isherman for components/autofill

[bulach, 2014-05-08 17:29:25]

lgtm, truly love this simplification, thanks a lot jam!

small nits and suggestions below:

https://codereview.chromium.org/273523007/diff/120001/content/browser/geoloca...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/273523007/diff/120001/content/browser/geoloca...
content/browser/geolocation/geolocation_dispatcher_host.cc:90: // initialization
here, defer to OnRegisterBridge which is triggered whenever
nit: while at it if you don't mind: OnStartUpdating (RegisterBridge is long
gone...)

https://codereview.chromium.org/273523007/diff/120001/content/browser/geoloca...
content/browser/geolocation/geolocation_dispatcher_host.cc:121:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
probably a spurious optimization, but anyways...
we could potentially register the callback and tell the GeolocationProvider
which thread we want to be dispatched?
it'd avoid hopping here from the geolocation thread to ui to io..

https://codereview.chromium.org/273523007/diff/120001/content/browser/geoloca...
content/browser/geolocation/geolocation_dispatcher_host.cc:191: if
(watching_requested_ & !paused_) {
nit: s/&/&&/

https://codereview.chromium.org/273523007/diff/120001/content/browser/geoloca...
File content/browser/geolocation/geolocation_dispatcher_host.h (right):

https://codereview.chromium.org/273523007/diff/120001/content/browser/geoloca...
content/browser/geolocation/geolocation_dispatcher_host.h:19: class
GeolocationDispatcherHost : public WebContentsObserver {
heheh, this class is showing its age :)
does it make sense to keep as a "dispatcher host"?
would it be more descriptive nowadays as GeolocationWebContentsObserver?

to be clear, it's probably best to do such rename as a follow up, just want to
highlight it..

https://codereview.chromium.org/273523007/diff/120001/content/public/browser/...
File content/public/browser/geolocation_provider.h (right):

https://codereview.chromium.org/273523007/diff/120001/content/public/browser/...
content/public/browser/geolocation_provider.h:22: // least one registered
observer or pending callback. The arbitrator and the
perhaps: ...or pending callback (and only after UserDidOptIntoLocationServices).

[jam, 2014-05-08 19:09:52]

https://codereview.chromium.org/273523007/diff/120001/content/browser/geoloca...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/273523007/diff/120001/content/browser/geoloca...
content/browser/geolocation/geolocation_dispatcher_host.cc:90: // initialization
here, defer to OnRegisterBridge which is triggered whenever
On 2014/05/08 17:29:26, bulach wrote:
> nit: while at it if you don't mind: OnStartUpdating (RegisterBridge is long
> gone...)

Done.

https://codereview.chromium.org/273523007/diff/120001/content/browser/geoloca...
content/browser/geolocation/geolocation_dispatcher_host.cc:121:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2014/05/08 17:29:26, bulach wrote:
> probably a spurious optimization, but anyways...
> we could potentially register the callback and tell the GeolocationProvider
> which thread we want to be dispatched?
> it'd avoid hopping here from the geolocation thread to ui to io..

In general, I think code should live as much as possible on the UI thread.
Developers shouldn't have to worry about the extra thread hop cost when sending
an IPC from the UI thread (for almost all code). i.e. in this case, a
geolocation update is pretty rare, and expensive one to get, so the thread hop
cost is trivial in comparison. Also, to do this from geolocation to IO would
mean needing to keep a reference to the BrowserMessageFilter since this class
isn't thread safe; adding more complexity. Finally, this wouldn't work with out
of process iframes where this class will probably send the update to multiple
processes.

https://codereview.chromium.org/273523007/diff/120001/content/browser/geoloca...
content/browser/geolocation/geolocation_dispatcher_host.cc:191: if
(watching_requested_ & !paused_) {
On 2014/05/08 17:29:26, bulach wrote:
> nit: s/&/&&/

oops, nice catch, thanks.

https://codereview.chromium.org/273523007/diff/120001/content/browser/geoloca...
File content/browser/geolocation/geolocation_dispatcher_host.h (right):

https://codereview.chromium.org/273523007/diff/120001/content/browser/geoloca...
content/browser/geolocation/geolocation_dispatcher_host.h:19: class
GeolocationDispatcherHost : public WebContentsObserver {
On 2014/05/08 17:29:26, bulach wrote:
> heheh, this class is showing its age :)
> does it make sense to keep as a "dispatcher host"?
> would it be more descriptive nowadays as GeolocationWebContentsObserver?
> 
> to be clear, it's probably best to do such rename as a follow up, just want to
> highlight it..

We have various naming conventions for this stuff. I don't feel strongly one way
or another for this name, but I agree that it should be in a followup. If you
care about it, I can do it after this

https://codereview.chromium.org/273523007/diff/120001/content/public/browser/...
File content/public/browser/geolocation_provider.h (right):

https://codereview.chromium.org/273523007/diff/120001/content/public/browser/...
content/public/browser/geolocation_provider.h:22: // least one registered
observer or pending callback. The arbitrator and the
On 2014/05/08 17:29:26, bulach wrote:
> perhaps: ...or pending callback (and only after
UserDidOptIntoLocationServices).

Done.

[jam, 2014-05-08 19:11:02]

+nasko for geolocation_messages.h

[Michael van Ouwerkerk, 2014-05-08 19:27:03]

Thanks! lgtm

[benm (inactive), 2014-05-08 20:23:32]

lgtm, one question but it's late here and i'm probably just missing something
obvious ;-)

https://codereview.chromium.org/273523007/diff/150001/content/browser/geoloca...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/273523007/diff/150001/content/browser/geoloca...
content/browser/geolocation/geolocation_dispatcher_host.cc:102:
geolocation_subscription_.reset();
sorry if it's a silly question, but why do we clear these and stop updates when
RVH changes? The new RVH will send another StartUpdating message should it
desire geolocation?

[dcheng, 2014-05-08 21:28:04]

https://codereview.chromium.org/273523007/diff/150001/base/callback_list.h.pump
File base/callback_list.h.pump (right):

https://codereview.chromium.org/273523007/diff/150001/base/callback_list.h.pu...
base/callback_list.h.pump:127: bool empty() { return callbacks_.empty(); }
Super duper minor driveby nit: I think this can be const-ified.

[nasko, 2014-05-08 21:53:44]

IPC LGTM

[jam, 2014-05-08 23:22:07]

https://codereview.chromium.org/273523007/diff/150001/content/browser/geoloca...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/273523007/diff/150001/content/browser/geoloca...
content/browser/geolocation/geolocation_dispatcher_host.cc:102:
geolocation_subscription_.reset();
On 2014/05/08 20:23:33, benm wrote:
> sorry if it's a silly question, but why do we clear these and stop updates
when
> RVH changes? The new RVH will send another StartUpdating message should it
> desire geolocation?

not a silly question, it's a bit cryptic. I was trying to maintain the exact
same behavior as before this change, since when the RVH changed then there would
have been a new render_process_id/render_view_id pair in the map.

this also prevents a bug where a renderer crashes and a new one is malicious and
gets geolocation permission from the old RVH

[jam, 2014-05-08 23:50:51]

+brettw for base

[bulach, 2014-05-09 09:50:15]

still lgtm, just clarifications:

https://codereview.chromium.org/273523007/diff/120001/content/browser/geoloca...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/273523007/diff/120001/content/browser/geoloca...
content/browser/geolocation/geolocation_dispatcher_host.cc:121:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2014/05/08 19:09:53, jam wrote:
> On 2014/05/08 17:29:26, bulach wrote:
> > probably a spurious optimization, but anyways...
> > we could potentially register the callback and tell the GeolocationProvider
> > which thread we want to be dispatched?
> > it'd avoid hopping here from the geolocation thread to ui to io..
> 
> In general, I think code should live as much as possible on the UI thread.
> Developers shouldn't have to worry about the extra thread hop cost when
sending
> an IPC from the UI thread (for almost all code). i.e. in this case, a
> geolocation update is pretty rare, and expensive one to get, so the thread hop
> cost is trivial in comparison. Also, to do this from geolocation to IO would
> mean needing to keep a reference to the BrowserMessageFilter since this class
> isn't thread safe; adding more complexity. Finally, this wouldn't work with
out
> of process iframes where this class will probably send the update to multiple
> processes.

thanks for the detailed explanation! I agree it's better as you have it, thanks!

https://codereview.chromium.org/273523007/diff/120001/content/browser/geoloca...
File content/browser/geolocation/geolocation_dispatcher_host.h (right):

https://codereview.chromium.org/273523007/diff/120001/content/browser/geoloca...
content/browser/geolocation/geolocation_dispatcher_host.h:19: class
GeolocationDispatcherHost : public WebContentsObserver {
On 2014/05/08 19:09:53, jam wrote:
> On 2014/05/08 17:29:26, bulach wrote:
> > heheh, this class is showing its age :)
> > does it make sense to keep as a "dispatcher host"?
> > would it be more descriptive nowadays as GeolocationWebContentsObserver?
> > 
> > to be clear, it's probably best to do such rename as a follow up, just want
to
> > highlight it..
> 
> We have various naming conventions for this stuff. I don't feel strongly one
way
> or another for this name, but I agree that it should be in a followup. If you
> care about it, I can do it after this

I think we have a few other FooWebContentsObserver, but I'm not quite sure how
strong that pattern is... 
you certainly know more than I do, so I'm happy whatever you decide.
(and yes, not on this patch anyways...)

[jam, 2014-05-09 16:43:32]

base->thakis

[Nico, 2014-05-09 16:51:44]

s/thakis/ajwong/ for callback stuff

[Cait (Slow), 2014-05-09 20:11:11]

base/callback_list* lgtm, modulo my comment re: the empty() function.

https://codereview.chromium.org/273523007/diff/230001/base/callback_list.h
File base/callback_list.h (right):

https://codereview.chromium.org/273523007/diff/230001/base/callback_list.h#ne...
base/callback_list.h:122: bool empty() { return callbacks_.empty(); }
Note that this will only return true when the active_iterator_count has gone to
0 and Compact() has run. 

In your case I think this is fine, b/c it gets called by your removal callback,
which only runs at those times anyways, but maybe add a comment to this effect,
to avoid confusion in future.

[jam, 2014-05-09 20:30:51]

https://codereview.chromium.org/273523007/diff/230001/base/callback_list.h
File base/callback_list.h (right):

https://codereview.chromium.org/273523007/diff/230001/base/callback_list.h#ne...
base/callback_list.h:122: bool empty() { return callbacks_.empty(); }
On 2014/05/09 20:11:13, Cait Phillips wrote:
> Note that this will only return true when the active_iterator_count has gone
to
> 0 and Compact() has run. 
> 
> In your case I think this is fine, b/c it gets called by your removal
callback,
> which only runs at those times anyways, but maybe add a comment to this
effect,
> to avoid confusion in future.

good point, thanks. i added a dcheck to ensure this. if someone later wants to
use it during iteration, which seems unlikely, we can make it support that case
then.

[jam, 2014-05-09 20:32:31]

back to brettw for owners lgtm. ajwong said caitp should review, which she did,
but i still need owner approval there

[brettw, 2014-05-09 20:36:21]

lgtm

[Ilya Sherman, 2014-05-09 20:39:30]

autofill changes LGTM (much nicer!).  Thanks!