Client-based geolocation support.

Client-based geolocation support.

Add in support for client-based geolocation in WebKit.
Default to disabled (ENABLE_CLIENT_BASED_GEOLOCATION=0) in features_override.gypi
until all the WebKit patches (see https://bugs.webkit.org/show_bug.cgi?id=45752)
have landed. When we switch over to client-based geolocation, we should remove
the old non-client-based geolocation code.

BUG=55907
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69156

[joth, 2010-12-07 10:24:51]

Epic! Thanks for guiding this refactoring in...

http://codereview.chromium.org/5612005/diff/1/build/features_override.gypi
File build/features_override.gypi (right):

http://codereview.chromium.org/5612005/diff/1/build/features_override.gypi#ne...
build/features_override.gypi:28: 'ENABLE_CLIENT_BASED_GEOLOCATION=0',
Probably as well to keep this list in alpha order

http://codereview.chromium.org/5612005/diff/1/chrome/browser/geolocation/geol...
File chrome/browser/geolocation/geolocation_dispatcher_host_old.cc (right):

http://codereview.chromium.org/5612005/diff/1/chrome/browser/geolocation/geol...
chrome/browser/geolocation/geolocation_dispatcher_host_old.cc:174: #if
defined(ENABLE_CLIENT_BASED_GEOLOCATION)
just to check, in the gypi you have set ENABLE_CLIENT_BASED_GEOLOCATION=0. Does
this not mean defined(ENABLE_CLIENT_BASED_GEOLOCATION) will be true?

http://codereview.chromium.org/5612005/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/5612005/diff/1/chrome/chrome_browser.gypi#newc...
chrome/chrome_browser.gypi:4277: ['"ENABLE_CLIENT_BASED_GEOLOCATION=1" in
feature_defines', {
this looks unusual, not least because it is = rather than == like the other
conditions.
Maybe put a comment on this saying it's an transitory thing whilst bug X is in
progress and it will be removed real soon.

The other option is to unconditionally set the ENABLE_CLIENT_BASED_GEOLOCATION
define here, and have a comment saying it should be kept in sync with the value
in feature defines.

http://codereview.chromium.org/5612005/diff/1/chrome/chrome_browser.gypi#newc...
chrome/chrome_browser.gypi:4278: 'defines': [
'ENABLE_CLIENT_BASED_GEOLOCATION=1' ]
ah, I see that explains it, this is a different ENABLE_CLIENT_BASED_GEOLOCATION
macro to the feature_defines one.
As they carry different values ([0,1] vs [undefined,1]) maybe give them
different names?

http://codereview.chromium.org/5612005/diff/1/chrome/chrome_renderer.gypi
File chrome/chrome_renderer.gypi (right):

http://codereview.chromium.org/5612005/diff/1/chrome/chrome_renderer.gypi#new...
chrome/chrome_renderer.gypi:317: 'defines': [
'ENABLE_CLIENT_BASED_GEOLOCATION=1' ]
ditto

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
File chrome/renderer/geolocation_dispatcher.cc (right):

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:13: #include
"chrome/renderer/geolocation_dispatcher.h"
alpha order

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:24: {
move { to previous line

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:29: }
you are allowed to do this all on one line:
Foo::~Foo() {}

Anyway, put { on previous line

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:64: enable_high_accuracy_ =
enable_high_accuracy;
do we not need to send IPC to the GeolocationDispatcherHost (if it changed)?
TODO?

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:69: controller_ = controller;
this does a deep copy which feels a bit funky as normally a BlahController is an
identity object (i.e. DISALLOW_COPY_AND_ASSIGN or equiv) but presumably this is
all as per the WebKit design.

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:73: return false;
maybe comment why? (we don't want to fetch it synchronously from the browser,
and no need for yet-another layer of caching)

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:82:
permissionRequest.securityOrigin().toString()));
this cast looks suspect. ah I see you're forcing it to invoke the conversion
operator.
Suggest using implicit_cast<> (defined in base) or make a temporary on the
stack:
string16 origin = permissionRequest.securityOrigin().toString();
rv_->Send(blah, blah, GURL(origin));

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:84: render_view_->routing_id(),
bridge_id, GURL(url)));
nit: no need for the GURL() around url (modulo my previous comment)

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:97: render_view_->routing_id(),
bridge_id, GURL(url)));
dittos

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:112: DCHECK(updating_);
suggest adding
DCHECK(geoposition.IsInitialized());

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:134: DCHECK(false);
NOTREACHED() << geoposition.error_code;

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
File chrome/renderer/geolocation_dispatcher.h (right):

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.h:12: #include "base/scoped_ptr.h"
base before third_party

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.h:22: class WebGeolocationController;
alpha order

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.h:27: };
nit: no ;

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.h:31: // It's the complement of
GeolocationDispatcherOld (owned by RenderViewHost).
isn't this the complement of GeolocationDispatcherHost? It's the replacement for
GeolocationDispatcherOld

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.h:32: 
nit: remove \n

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.h:35: explicit
GeolocationDispatcher(RenderView*);
In chrome we do put the parameter names in method declarations. (not sure about
in the implementations of the WebKit methods below, but my instinct would be to
include them all)

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.h:54: RenderView* render_view_;
comment ownership

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.h:61: void
OnGeolocationPositionUpdated(const Geoposition& geoposition);
put all methods before member data within each section.

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/render_view.h#n...
chrome/renderer/render_view.h:1377: #endif
ISTM we could have reduced churn by leaving the class named as
GeolocationDispatcher through out, but just giving it a different definition and
.cc impl based on the build flags.

Probably not worth changing tack on this now though.

http://codereview.chromium.org/5612005/diff/1/webkit/tools/test_shell/test_sh...
File webkit/tools/test_shell/test_shell.cc (right):

http://codereview.chromium.org/5612005/diff/1/webkit/tools/test_shell/test_sh...
webkit/tools/test_shell/test_shell.cc:653: #if ENABLE_CLIENT_BASED_GEOLOCATION
if defined() ?

http://codereview.chromium.org/5612005/diff/1/webkit/tools/test_shell/test_we...
File webkit/tools/test_shell/test_webview_delegate.cc (right):

http://codereview.chromium.org/5612005/diff/1/webkit/tools/test_shell/test_we...
webkit/tools/test_shell/test_webview_delegate.cc:662: {
{ on prev line

[bulach, 2010-12-07 11:18:42]

great stuff, thanks for such a massive refactoring and this many-sided changes!

some nits below:

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
File chrome/renderer/geolocation_dispatcher.cc (right):

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:21: pending_permissions_(new
WebGeolocationPermissionRequestContainer),
we normally add "()", as new Foo()

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:57: // TODO(jknotten): Remove url and
bridge_id from StartUpdating message
nit: StopUpdating message

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
File chrome/renderer/geolocation_dispatcher.h (right):

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.h:3: // found in the LICENSE file.
\n

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.h:69: #endif //
ENABLE_CLIENT_BASED_GEOLOCATION
\n

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
File chrome/renderer/geolocation_dispatcher_old.h (right):

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher_old.h:64: #endif //
ENABLE_CLIENT_BASED_GEOLOCATION
\n

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/render_view.cc#...
chrome/renderer/render_view.cc:5601: #endif
\n

http://codereview.chromium.org/5612005/diff/1/webkit/tools/test_shell/test_sh...
File webkit/tools/test_shell/test_shell.cc (right):

http://codereview.chromium.org/5612005/diff/1/webkit/tools/test_shell/test_sh...
webkit/tools/test_shell/test_shell.cc:654: // Ensure a fresh (unset) state for
next test run
comment seems a bit unnecessary.

http://codereview.chromium.org/5612005/diff/1/webkit/tools/test_shell/test_sh...
File webkit/tools/test_shell/test_shell.h (right):

http://codereview.chromium.org/5612005/diff/1/webkit/tools/test_shell/test_sh...
webkit/tools/test_shell/test_shell.h:72: class WebGeolocationClientMock;
sort order

[John Knottenbelt, 2010-12-07 12:30:04]

Thanks for the review. I'll make the changes and publish a patch soon. Also,
will change WebGeolocationController from being a copyable value type to a
non-copyable wrapper, allocated on the heap and tracked via scoped_ptr.

http://codereview.chromium.org/5612005/diff/1/build/features_override.gypi
File build/features_override.gypi (right):

http://codereview.chromium.org/5612005/diff/1/build/features_override.gypi#ne...
build/features_override.gypi:28: 'ENABLE_CLIENT_BASED_GEOLOCATION=0',
On 2010/12/07 10:24:51, joth wrote:
> Probably as well to keep this list in alpha order

Done.

http://codereview.chromium.org/5612005/diff/1/chrome/browser/geolocation/geol...
File chrome/browser/geolocation/geolocation_dispatcher_host_old.cc (right):

http://codereview.chromium.org/5612005/diff/1/chrome/browser/geolocation/geol...
chrome/browser/geolocation/geolocation_dispatcher_host_old.cc:174: #if
defined(ENABLE_CLIENT_BASED_GEOLOCATION)
On 2010/12/07 10:24:51, joth wrote:
> just to check, in the gypi you have set ENABLE_CLIENT_BASED_GEOLOCATION=0.
Does
> this not mean defined(ENABLE_CLIENT_BASED_GEOLOCATION) will be true?

No, see comments in chrome_browser.gypi

http://codereview.chromium.org/5612005/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/5612005/diff/1/chrome/chrome_browser.gypi#newc...
chrome/chrome_browser.gypi:4277: ['"ENABLE_CLIENT_BASED_GEOLOCATION=1" in
feature_defines', {
I'll add a comment in to say that it will disappear once we have completed a
migration to client-based geolocation (together with some bug references).

The defines in feature_override.gypi are only automatically exposed as
preprocessor macros when compiling the WebKit source. The above exposes
ENABLE_CLIENT_BASED_GEOLOCATION as a preprocessor macro when compiling Chromium
code.

http://codereview.chromium.org/5612005/diff/1/chrome/chrome_browser.gypi#newc...
chrome/chrome_browser.gypi:4278: 'defines': [
'ENABLE_CLIENT_BASED_GEOLOCATION=1' ]
On 2010/12/07 10:24:51, joth wrote:
> ah, I see that explains it, this is a different
ENABLE_CLIENT_BASED_GEOLOCATION
> macro to the feature_defines one.
> As they carry different values ([0,1] vs [undefined,1]) maybe give them
> different names?

The feature_defines array is purely a gyp variable. In WebKit, these are exposed
as ENABLE_XXX if ENABLE_XXX=1 is present in the features_defines and undefined
otherwise, which is why I have adopted the same practice on this end.

http://codereview.chromium.org/5612005/diff/1/chrome/chrome_renderer.gypi
File chrome/chrome_renderer.gypi (right):

http://codereview.chromium.org/5612005/diff/1/chrome/chrome_renderer.gypi#new...
chrome/chrome_renderer.gypi:317: 'defines': [
'ENABLE_CLIENT_BASED_GEOLOCATION=1' ]
Will add comment.

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
File chrome/renderer/geolocation_dispatcher.cc (right):

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:82:
permissionRequest.securityOrigin().toString()));
On 2010/12/07 10:24:51, joth wrote:
> this cast looks suspect. ah I see you're forcing it to invoke the conversion
> operator.
> Suggest using implicit_cast<> (defined in base) or make a temporary on the
> stack:
> string16 origin = permissionRequest.securityOrigin().toString();
> rv_->Send(blah, blah, GURL(origin));
> 

Done.

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:84: render_view_->routing_id(),
bridge_id, GURL(url)));
On 2010/12/07 10:24:51, joth wrote:
> nit: no need for the GURL() around url (modulo my previous comment)

Done.

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:97: render_view_->routing_id(),
bridge_id, GURL(url)));
On 2010/12/07 10:24:51, joth wrote:
> dittos

Done.

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:112: DCHECK(updating_);
On 2010/12/07 10:24:51, joth wrote:
> suggest adding
> DCHECK(geoposition.IsInitialized());

Done.

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/render_view.h#n...
chrome/renderer/render_view.h:1377: #endif
Agree. Once we switch over, I plan to rename things back and remove the _old
suffix on dispatcher_host_old, which actually serves both the client and
non-client-based dispatchers at the moment.

http://codereview.chromium.org/5612005/diff/1/webkit/tools/test_shell/test_sh...
File webkit/tools/test_shell/test_shell.cc (right):

http://codereview.chromium.org/5612005/diff/1/webkit/tools/test_shell/test_sh...
webkit/tools/test_shell/test_shell.cc:653: #if ENABLE_CLIENT_BASED_GEOLOCATION
Good spot!

http://codereview.chromium.org/5612005/diff/1/webkit/tools/test_shell/test_sh...
webkit/tools/test_shell/test_shell.cc:654: // Ensure a fresh (unset) state for
next test run
On 2010/12/07 11:18:42, bulach wrote:
> comment seems a bit unnecessary.

Removed.

http://codereview.chromium.org/5612005/diff/1/webkit/tools/test_shell/test_we...
File webkit/tools/test_shell/test_webview_delegate.cc (right):

http://codereview.chromium.org/5612005/diff/1/webkit/tools/test_shell/test_we...
webkit/tools/test_shell/test_webview_delegate.cc:662: {
On 2010/12/07 10:24:51, joth wrote:
> { on prev line

Done.

[joth, 2010-12-08 10:47:40]

Looks good, but not sure I think you might have missed a couple of my comments
(resurfaced below)
Cheers,

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
File chrome/renderer/geolocation_dispatcher.cc (right):

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:64: enable_high_accuracy_ =
enable_high_accuracy;
On 2010/12/07 10:24:51, joth wrote:
> do we not need to send IPC to the GeolocationDispatcherHost (if it changed)?
> TODO?

ping

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:134: DCHECK(false);
On 2010/12/07 10:24:51, joth wrote:
> NOTREACHED() << geoposition.error_code;

ping

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
File chrome/renderer/geolocation_dispatcher.h (right):

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.h:31: // It's the complement of
GeolocationDispatcherOld (owned by RenderViewHost).
On 2010/12/07 10:24:51, joth wrote:
> isn't this the complement of GeolocationDispatcherHost? It's the replacement
for
> GeolocationDispatcherOld

ping

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.h:54: RenderView* render_view_;
On 2010/12/07 10:24:51, joth wrote:
> comment ownership

ping

[John Knottenbelt, 2010-12-08 12:28:34]

Sorry I missed those earlier comments. I have now addressed them in the latest
patch.

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
File chrome/renderer/geolocation_dispatcher.cc (right):

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:64: enable_high_accuracy_ =
enable_high_accuracy;
Sorry, missed this one. We don't need to send an IPC because
setEnableHighAccuracy is called by the GeolocationController:
 - with true, before it calls startUpdating in response to the first
high-accuracy Geolocation subscription.
 - with false, and after having called stopUpdating when the last high-accuracy
Geolocation unsubscribes.
I will add a comment about this.
On 2010/12/08 10:47:40, joth wrote:
> On 2010/12/07 10:24:51, joth wrote:
> > do we not need to send IPC to the GeolocationDispatcherHost (if it changed)?
> > TODO?
> 
> ping

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:134: DCHECK(false);
On 2010/12/08 10:47:40, joth wrote:
> On 2010/12/07 10:24:51, joth wrote:
> > NOTREACHED() << geoposition.error_code;
> 
> ping

Done.

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
File chrome/renderer/geolocation_dispatcher.h (right):

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.h:31: // It's the complement of
GeolocationDispatcherOld (owned by RenderViewHost).
On 2010/12/08 10:47:40, joth wrote:
> On 2010/12/07 10:24:51, joth wrote:
> > isn't this the complement of GeolocationDispatcherHost? It's the replacement
> for
> > GeolocationDispatcherOld
> 
> ping

Done.

[joth, 2010-12-08 12:54:29]

Thanks. Still not quite clear on the contract for this method... (the code could
well be correct, just trying to understand it)

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
File chrome/renderer/geolocation_dispatcher.cc (right):

http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
chrome/renderer/geolocation_dispatcher.cc:64: enable_high_accuracy_ =
enable_high_accuracy;
On 2010/12/08 12:28:34, John Knottenbelt wrote:
> Sorry, missed this one. We don't need to send an IPC because
> setEnableHighAccuracy is called by the GeolocationController:
>  - with true, before it calls startUpdating in response to the first
> high-accuracy Geolocation subscription.
>  - with false, and after having called stopUpdating when the last
high-accuracy
> Geolocation unsubscribes.
what if there are still low-accuracy subscriber when the last high accuracy one
leaves us? It can't call stopUpdating, so does it re-call startUpdating? Or does
it do a stop/start?

[John Knottenbelt, 2010-12-08 15:18:48]

Good spot. I've put in a call to startUpdating() if we are already updating so
that the browser can update the high-accuracy requirements. 

I've also filed http://code.google.com/p/chromium/issues/detail?id=65885 so that
we can get some coverage of the high-accuracy mode at the browser level.

On 2010/12/08 12:54:29, joth wrote:
> Thanks. Still not quite clear on the contract for this method... (the code
could
> well be correct, just trying to understand it)
> 
>
http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
> File chrome/renderer/geolocation_dispatcher.cc (right):
> 
>
http://codereview.chromium.org/5612005/diff/1/chrome/renderer/geolocation_dis...
> chrome/renderer/geolocation_dispatcher.cc:64: enable_high_accuracy_ =
> enable_high_accuracy;
> On 2010/12/08 12:28:34, John Knottenbelt wrote:
> > Sorry, missed this one. We don't need to send an IPC because
> > setEnableHighAccuracy is called by the GeolocationController:
> >  - with true, before it calls startUpdating in response to the first
> > high-accuracy Geolocation subscription.
> >  - with false, and after having called stopUpdating when the last
> high-accuracy
> > Geolocation unsubscribes.
> what if there are still low-accuracy subscriber when the last high accuracy
one
> leaves us? It can't call stopUpdating, so does it re-call startUpdating? Or
does
> it do a stop/start?

[joth, 2010-12-09 12:00:20]

LGTM

[jam, 2010-12-11 19:01:10]

hi, some minor feedback regarding naming, please do this before checkin.

per the thread on chromium-dev ("heads-up: message filters in the browser"),
GeolocationDispatcher should be named GeolocationMessageFilter and should derive
from BrowserMessageFilter.  I don't see how you add this new class to the list
of filters, but the old one was a member of ResourceMessageFilter.  The new way
of doing this is to add it in BrowserRenderProcessHost::CreateMessageFilters().

Thanks

[joth, 2010-12-12 18:31:08]

Excellent point, thanks very much for the timely reminder, and for making
that refactoring in the first place.
I meant to mention this when I saw your heads-up email but don't think I
ever did.


On 11 December 2010 19:01, <jam@chromium.org> wrote:

> hi, some minor feedback regarding naming, please do this before checkin.
>
> per the thread on chromium-dev ("heads-up: message filters in the
> browser"),
> GeolocationDispatcher should be named GeolocationMessageFilter and should
> derive
> from BrowserMessageFilter.  I don't see how you add this new class to the
> list
> of filters, but the old one was a member of ResourceMessageFilter.  The new
> way
> of doing this is to add it in
> BrowserRenderProcessHost::CreateMessageFilters().
>
> Thanks
>
>
> http://codereview.chromium.org/5612005/
>

[bulach, 2010-12-13 14:49:50]

LGTM, but just double checking with jam:

on your comment above you mentioned "should be named GeolocationMessageFilter
and should derive
from BrowserMessageFilter", but on the announcement email says that it should be
FeatureXDispatcherHost. I guess the current patch is correct, as in, it's
"GeolocationDispatcherHost : public BrowserMessageFilter" since it's got a
corresponding renderer object? would you mind taking a quick look, please?

http://codereview.chromium.org/5612005/diff/59001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_dispatcher_host.cc (right):

http://codereview.chromium.org/5612005/diff/59001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_dispatcher_host.cc:31: // handled. Called
in the browser process.
ditto

http://codereview.chromium.org/5612005/diff/59001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_dispatcher_host.h (right):

http://codereview.chromium.org/5612005/diff/59001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_dispatcher_host.h:16: 
remove \n

http://codereview.chromium.org/5612005/diff/59001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_dispatcher_host.h:24: // handled. Called
in the browser process.
remove 23-24 and just use:
// BrowserMessageFilter implementation.

but I guess this decl is not needed, right? it's already declared pure virtual
in the base class.

http://codereview.chromium.org/5612005/diff/59001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

http://codereview.chromium.org/5612005/diff/59001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_permission_context.cc:445: // requests
the client to start updating.
s/requests requests/requests/

http://codereview.chromium.org/5612005/diff/59001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/5612005/diff/59001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/browser_render_process_host.cc:39: #include
"chrome/browser/geolocation/geolocation_permission_context.h"
I guess you can remove the geolocation_permission_context.h

http://codereview.chromium.org/5612005/diff/59001/chrome/renderer/geolocation...
File chrome/renderer/geolocation_dispatcher.cc (right):

http://codereview.chromium.org/5612005/diff/59001/chrome/renderer/geolocation...
chrome/renderer/geolocation_dispatcher.cc:7: #include
"chrome/renderer/geolocation_dispatcher.h"
\n

http://codereview.chromium.org/5612005/diff/59001/chrome/renderer/geolocation...
chrome/renderer/geolocation_dispatcher.cc:70: // accuracy requirements.
maybe:

bool has_changed = enable_high_accuracy_ != enable_high_accuracy;
enable_high_accuracy_ = enable_high_accuracy;
// We have a different accuracy requirement, request browser
// to update.
if (updating_ && has_changed)
  startUpdating();

http://codereview.chromium.org/5612005/diff/59001/chrome/renderer/geolocation...
File chrome/renderer/geolocation_dispatcher.h (right):

http://codereview.chromium.org/5612005/diff/59001/chrome/renderer/geolocation...
chrome/renderer/geolocation_dispatcher.h:32: // It's the complement of
GeolocationDispatcher (owned by RenderViewHost).
s/GeolocationDispatcher/GeolocationDispatcherHost/

[jam, 2010-12-13 20:17:46]

lgtm with these nits.  and yep, sorry i didn't see the renderer-side
corresponding object, so GeolocationDispatcherHost is right

http://codereview.chromium.org/5612005/diff/52002/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_dispatcher_host.h (right):

http://codereview.chromium.org/5612005/diff/52002/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_dispatcher_host.h:12: namespace IPC {
class Message; }
don't need this

http://codereview.chromium.org/5612005/diff/52002/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_dispatcher_host.h:20: int
resource_message_filter_process_id,
nit: please just call this "render_process_id" for consistency

http://codereview.chromium.org/5612005/diff/52002/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_dispatcher_host.h:24: // handled. Called
in the browser process.
nit: we usually don't repeat comments from parent classes

[John Knottenbelt, 2010-12-14 15:01:09]

http://codereview.chromium.org/5612005/diff/52002/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_dispatcher_host.h (right):

http://codereview.chromium.org/5612005/diff/52002/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_dispatcher_host.h:12: namespace IPC {
class Message; }
On 2010/12/13 20:17:46, John Abd-El-Malek wrote:
> don't need this

Done.

http://codereview.chromium.org/5612005/diff/52002/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_dispatcher_host.h:20: int
resource_message_filter_process_id,
On 2010/12/13 20:17:46, John Abd-El-Malek wrote:
> nit: please just call this "render_process_id" for consistency

Done.

http://codereview.chromium.org/5612005/diff/52002/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_dispatcher_host.h:24: // handled. Called
in the browser process.
On 2010/12/13 20:17:46, John Abd-El-Malek wrote:
> nit: we usually don't repeat comments from parent classes

Done.

[jam, 2010-12-14 23:27:01]

http://codereview.chromium.org/5612005/diff/72002/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/5612005/diff/72002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_message_filter.cc:24: #include
"chrome/browser/file_system/file_system_dispatcher_host.h"
this got merged in by accident, this include shouldn't be added

[John Knottenbelt, 2010-12-15 10:39:22]

Thanks. I've opened a new issue for this change:
http://codereview.chromium.org/5890001/