Introduce GeolocationPermissionRequestID, a wrapper struct to contain the (render process ID, rende…

Introduce GeolocationPermissionRequestID, a wrapper class to contain the (render process ID, render view ID, bridge ID) triple that uniquely identifies a geolocation permission request.

This ends up simplifying/saving a noticeable chunk of code.

Also do some other cleanups, e.g.:
* Eliminating using statements when they don't save any lines
* Wrapping per the style guide rules
* Changing PendingInfoBarRequest from a struct to a class since it has more than just a constructor/init method
* Move GeolocationDispatcherHost into content namespace.

BUG=none
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=164475

[Peter Kasting, 2012-10-24 03:32:57]

joi: content/public/browser/ OWNERS
jknotten: everything

It's not clear to me that geolocation.h is the ideal spot for this struct.

[Jói, 2012-10-24 09:48:36]

LGTM with a question.  Adding jam@ who wants to be in the loop on interface
changes.

Cheers,
Jói

https://codereview.chromium.org/11269002/diff/11003/content/public/browser/ge...
File content/public/browser/geolocation.h (right):

https://codereview.chromium.org/11269002/diff/11003/content/public/browser/ge...
content/public/browser/geolocation.h:25: bool Equals(const
GeolocationPermissionRequestID& other) const;
I think our coding style only allows constructor, "Init" and "Reset" type
functions on a struct; should this be a class instead?

Personally I think it's fine this way and I'm not 100% sure on the coding style
question, so will give my LGTM for this set-up.

[John Knottenbelt, 2012-10-24 10:59:10]

Very nice, thanks Peter! lgtm.

CC'ing Ramya, who has a change in flight regarding Android geolocation infobars.

Also adding Bulach as co-owner.

https://codereview.chromium.org/11269002/diff/11003/chrome/browser/geolocatio...
File chrome/browser/geolocation/geolocation_infobar_queue_controller.cc (right):

https://codereview.chromium.org/11269002/diff/11003/chrome/browser/geolocatio...
chrome/browser/geolocation/geolocation_infobar_queue_controller.cc:257:
InfoBarTabHelper* GeolocationInfoBarQueueController::GetInfoBarHelper(
Can this be a non-member function local to this file?

https://codereview.chromium.org/11269002/diff/11003/chrome/browser/geolocatio...
File chrome/browser/geolocation/geolocation_infobar_queue_controller.h (right):

https://codereview.chromium.org/11269002/diff/11003/chrome/browser/geolocatio...
chrome/browser/geolocation/geolocation_infobar_queue_controller.h:78: bool
AlreadyShowingInfoBarForTab(
Can you make this method const?

[John Knottenbelt, 2012-10-24 12:41:55]

Just noticed that GeolocationPermissionContextTests.TabDestroyed unit test
failed on linux_rel, win_rel and maybe some other bots, may be related to your
change?

[jam, 2012-10-24 16:39:43]

given that
-this new struct should be a class
-the content api only has interfaces and not concreate objects
-the benefit for this is in chrome (in content i see conversion back and forth
in geolocation_dispatcher_host.cc with the only time it's not converted back is
to the two GeolocationPermissionContext callbacks).

can we keep this all inside chrome?

https://codereview.chromium.org/11269002/diff/11003/content/public/browser/ge...
File content/public/browser/geolocation.h (right):

https://codereview.chromium.org/11269002/diff/11003/content/public/browser/ge...
content/public/browser/geolocation.h:25: bool Equals(const
GeolocationPermissionRequestID& other) const;
On 2012/10/24 09:48:36, Jói wrote:
> I think our coding style only allows constructor, "Init" and "Reset" type
> functions on a struct; should this be a class instead?
> 
> Personally I think it's fine this way and I'm not 100% sure on the coding
style
> question, so will give my LGTM for this set-up.

you're right, the style guide is specific that if there are functions other than
init/reset it should be a class. personally i think this complicates scenarios
like this, but I think we should follow the style guide.

nit: why Equals instead of operator==?

[Peter Kasting, 2012-10-24 19:48:37]

On 2012/10/24 16:39:43, John Abd-El-Malek wrote:
> -this new struct should be a class

I agree.  I'll change this.

> -the content api only has interfaces and not concreate objects
> -the benefit for this is in chrome (in content i see conversion back and forth
> in geolocation_dispatcher_host.cc with the only time it's not converted back
is
> to the two GeolocationPermissionContext callbacks).
> 
> can we keep this all inside chrome?

The big problem with this is that it blocks me from changing the
GeolocationPermissionContext interface APIs, which in turn means I can't change
any of their implementations' signatures either, which is one (of a couple) of
the main wins of this change.

(I actually could use this more than I did in geolocation_dispatcher_host.cc,
but that's a trivial detail.)

I'm not sure how to comply with the "no concrete objects in content headers"
rule and still keep this class in content/.  Do you have ideas?  Is this the
kind of rule we could bend for a class that just wraps up some data members?  Or
maybe there's an appropriate header location for this kind of thing?

> nit: why Equals instead of operator==?

The style guide bans operator==() overloading except where absolutely necessary
and suggests an explicit Equals() function instead.

[Peter Kasting, 2012-10-24 19:50:47]

https://codereview.chromium.org/11269002/diff/11003/chrome/browser/geolocatio...
File chrome/browser/geolocation/geolocation_infobar_queue_controller.cc (right):

https://codereview.chromium.org/11269002/diff/11003/chrome/browser/geolocatio...
chrome/browser/geolocation/geolocation_infobar_queue_controller.cc:257:
InfoBarTabHelper* GeolocationInfoBarQueueController::GetInfoBarHelper(
On 2012/10/24 10:59:10, John Knottenbelt wrote:
> Can this be a non-member function local to this file?

I think so.  Will change.

https://codereview.chromium.org/11269002/diff/11003/chrome/browser/geolocatio...
File chrome/browser/geolocation/geolocation_infobar_queue_controller.h (right):

https://codereview.chromium.org/11269002/diff/11003/chrome/browser/geolocatio...
chrome/browser/geolocation/geolocation_infobar_queue_controller.h:78: bool
AlreadyShowingInfoBarForTab(
On 2012/10/24 10:59:10, John Knottenbelt wrote:
> Can you make this method const?

Yes.  Will fix.

[jam, 2012-10-24 20:06:03]

On Wed, Oct 24, 2012 at 12:48 PM, <pkasting@chromium.org> wrote:

> On 2012/10/24 16:39:43, John Abd-El-Malek wrote:
>
>> -this new struct should be a class
>>
>
> I agree.  I'll change this.
>
>
>  -the content api only has interfaces and not concreate objects
>> -the benefit for this is in chrome (in content i see conversion back and
>> forth
>> in geolocation_dispatcher_host.cc with the only time it's not converted
>> back
>>
> is
>
>> to the two GeolocationPermissionContext callbacks).
>>
>
>  can we keep this all inside chrome?
>>
>
> The big problem with this is that it blocks me from changing the
> GeolocationPermissionContext interface APIs, which in turn means I can't
> change
> any of their implementations' signatures either, which is one (of a
> couple) of
> the main wins of this change.
>

it seems that the two overridden methods can still use the ints, but the
rest of the file can use the struct which would get most of the win?

>
> (I actually could use this more than I did in geolocation_dispatcher_host.
> **cc,
> but that's a trivial detail.)
>
> I'm not sure how to comply with the "no concrete objects in content
> headers"
> rule and still keep this class in content/.  Do you have ideas?  Is this
> the
> kind of rule we could bend for a class that just wraps up some data
> members?


not really. this is a hard rule that i've stuck to for the content api.
there have been places where it would be nicer to break this, but i think
on the whole this makes the api cleaner.


>  Or
> maybe there's an appropriate header location for this kind of thing?
>
>
>  nit: why Equals instead of operator==?
>>
>
> The style guide bans operator==() overloading except where absolutely
> necessary
> and suggests an explicit Equals() function instead.
>

oh, I didn't know that.


>
https://codereview.chromium.**org/11269002/<https://codereview.chromium.org/1...
>

[Peter Kasting, 2012-10-24 21:01:07]

On 2012/10/24 20:06:03, John Abd-El-Malek wrote:
> > The big problem with this is that it blocks me from changing the
> > GeolocationPermissionContext interface APIs, which in turn means I can't
> > change
> > any of their implementations' signatures either, which is one (of a
> > couple) of
> > the main wins of this change.
> 
> it seems that the two overridden methods can still use the ints, but the
> rest of the file can use the struct which would get most of the win?

There are 24 declarations/definitions/calls of RequestGeolocationPermission and
another 5 for CancelGeolocationPermissionRequest.

If you look at how these are used, I'd have to box and unbox in a lot of places.
 It would destroy the utility of this change.

> not really. this is a hard rule that i've stuck to for the content api.
> there have been places where it would be nicer to break this, but i think
> on the whole this makes the api cleaner.

If there are select places where allowing concrete structs would make life
better, why not allow them selectively?  Say that concrete structs are banned
except with explicit special permission and then use them very sparingly.

I don't understand what holding hard to this rule does for you versus
selectively allowing exceptions where sane.  The only possible reason I could
think of would be that you're worried that if you allow one people will want
more.  But the current system already assumes you're going to review people's
changes before they check in, so that's not an issue, because you can just say
no.  So I'm mystified.

I suppose I could try to work around this by writing a forwarding subclass
header in chrome/ that implements the content-side calls by boxing the args and
passing them on to the "real" implementations, and then making all subclasses
inherit from that.  But it's really ugly and I don't see why it's necessary. 
(And it blocks content/ code, current or future, from using the class.)

PK

[jam, 2012-10-24 21:16:55]

On Wed, Oct 24, 2012 at 2:01 PM, <pkasting@chromium.org> wrote:

> On 2012/10/24 20:06:03, John Abd-El-Malek wrote:
>
>> > The big problem with this is that it blocks me from changing the
>> > GeolocationPermissionContext interface APIs, which in turn means I can't
>> > change
>> > any of their implementations' signatures either, which is one (of a
>> > couple) of
>> > the main wins of this change.
>>
>
>  it seems that the two overridden methods can still use the ints, but the
>> rest of the file can use the struct which would get most of the win?
>>
>
> There are 24 declarations/definitions/calls of
> RequestGeolocationPermission and
> another 5 for CancelGeolocationPermissionReq**uest.
>
> If you look at how these are used, I'd have to box and unbox in a lot of
> places.
>  It would destroy the utility of this change.


The majority of the RequestGeolocationPermission calls are in the unittest,
and those aren't calling the method on GeolocationPermissionContext but
instead a method on GeolocationPermissionContextTests. Seems
that GeolocationPermissionContextTests::RequestGeolocationPermission can
still take the struct, but unpack locally, and you'd still have the benefit.


>
>  not really. this is a hard rule that i've stuck to for the content api.
>> there have been places where it would be nicer to break this, but i think
>> on the whole this makes the api cleaner.
>>
>
> If there are select places where allowing concrete structs would make life
> better, why not allow them selectively?  Say that concrete structs are
> banned
> except with explicit special permission and then use them very sparingly.
>
> I don't understand what holding hard to this rule does for you versus
> selectively allowing exceptions where sane.  The only possible reason I
> could
> think of would be that you're worried that if you allow one people will
> want
> more.  But the current system already assumes you're going to review
> people's
> changes before they check in, so that's not an issue, because you can just
> say
> no.  So I'm mystified.
>

I prefer simple rules. They're easy to enforce, and for people to follow. I
also prefer not having to make judgement calls on when concrete classes are
allowed or not because this is a slippery slope.


>
> I suppose I could try to work around this by writing a forwarding subclass
> header in chrome/ that implements the content-side calls by boxing the
> args and
> passing them on to the "real" implementations, and then making all
> subclasses
> inherit from that.  But it's really ugly and I don't see why it's
> necessary.
> (And it blocks content/ code, current or future, from using the class.)
>
> PK
>
>
https://codereview.chromium.**org/11269002/<https://codereview.chromium.org/1...
>

[Peter Kasting, 2012-10-24 21:27:30]

I appreciate the utility of simple rules.  I also prefer simplicity and would
rather not be inconsistent about what geolocation code does what.

So how about this solution:

I make this a pure struct by removing all the methods.  I'll even remove the
constructor if you like.  Then this is just an aggregate with no functionality
at all.  Surely that's OK to have in a content header?

It's easy to define helper methods external to the struct to handle the couple
functions that are currently members.  As an added bonus this lets me move at
least the functions besides ToString() -- and maybe that function as well if we
want to give up on using it in geolocation_dispatcher_host.cc -- to somewhere in
chrome/.  Maybe even local to some .cc file if they're only used one place
(didn't look).

WDYT?

[jam, 2012-10-24 22:00:08]

On Wed, Oct 24, 2012 at 2:27 PM, <pkasting@chromium.org> wrote:

> I appreciate the utility of simple rules.  I also prefer simplicity and
> would
> rather not be inconsistent about what geolocation code does what.
>
> So how about this solution:
>
> I make this a pure struct by removing all the methods.  I'll even remove
> the
> constructor if you like.


constructor would be fine. btw this should go into
content/public/browser/geolocation_permission_request_id.h per convention
of each struct/interface in a separate file.


>  Then this is just an aggregate with no functionality
> at all.  Surely that's OK to have in a content header?
>
> It's easy to define helper methods external to the struct to handle the
> couple
> functions that are currently members.


while this would keep the content headers consistent, it's unfortunate that
methods which operate on this struct are in a totally different place.

 As an added bonus this lets me move at
> least the functions besides ToString() -- and maybe that function as well
> if we
> want to give up on using it in geolocation_dispatcher_host.cc -- to
> somewhere in
> chrome/.  Maybe even local to some .cc file if they're only used one place
> (didn't look).
>
> WDYT?
>

that sounds reasonable since it would conform to the style guide and the
content api.

i will ask one question: is there enough benefit to keep it in the content
api? i.e. for geolocation_disaptcher_host.cc's
SendGeolocationPermissionResponse the caller creates the struct only for
that function to unpack it.


>
https://codereview.chromium.**org/11269002/<https://codereview.chromium.org/1...
>

[Peter Kasting, 2012-10-24 22:33:59]

On 2012/10/24 22:00:08, John Abd-El-Malek wrote:
> btw this should go into
> content/public/browser/geolocation_permission_request_id.h per convention
> of each struct/interface in a separate file.

Sure, I can do that.

Where would you most prefer functions like the equality check?  Should I put
them in this header file to keep them alongside the struct, since you expressed
some unhappiness at that?  Or should I define them on the chrome side if they're
unnecessary in content?  I probably would prefer the former, but if it feels too
much like a violation of the "no concrete classes" API I can put it elsewhere.

(I'll put a function elsewhere anyway if I find its uses are all confined to a
single consumer; I'll just define in that .cc file in that case.)

> i will ask one question: is there enough benefit to keep it in the content
> api? i.e. for geolocation_disaptcher_host.cc's
> SendGeolocationPermissionResponse the caller creates the struct only for
> that function to unpack it.

I'm not sure precisely what "keep it in the content api" means.  The primary
goal is to keep it in geolocation_permission_context.h.

I will take a closer look at geolocation_dispatcher_host.cc, the IPC methods,
etc., and check into the following alternatives:

* Revert SendGeolocationPermissionResponse()'s signature, which would undo about
half the changes to this file.
* Use the ID struct pervasively in this file and do the boxing and unboxing
closer to where the IPC messages are handled.  This would probably allow us to
nuke the |render_process_id_| member as well.

[jam, 2012-10-24 22:48:34]

On Wed, Oct 24, 2012 at 3:33 PM, <pkasting@chromium.org> wrote:

> On 2012/10/24 22:00:08, John Abd-El-Malek wrote:
>
>> btw this should go into
>> content/public/browser/**geolocation_permission_**request_id.h per
>> convention
>> of each struct/interface in a separate file.
>>
>
> Sure, I can do that.
>
> Where would you most prefer functions like the equality check?  Should I
> put
> them in this header file to keep them alongside the struct, since you
> expressed
> some unhappiness at that?  Or should I define them on the chrome side if
> they're
> unnecessary in content?


they don't really belong in content if they're outside the struct. i.e. to
have a file with this struct, and then the methods right after them, would
only be a less readable form of having a struct with methods


>  I probably would prefer the former, but if it feels too
> much like a violation of the "no concrete classes" API I can put it
> elsewhere.
>
> (I'll put a function elsewhere anyway if I find its uses are all confined
> to a
> single consumer; I'll just define in that .cc file in that case.)
>
>
>  i will ask one question: is there enough benefit to keep it in the content
>> api? i.e. for geolocation_disaptcher_host.**cc's
>> SendGeolocationPermissionRespo**nse the caller creates the struct only
>> for
>> that function to unpack it.
>>
>
> I'm not sure precisely what "keep it in the content api" means.  The
> primary
> goal is to keep it in geolocation_permission_**context.h.
>

I meant not use this in the content api, including
geolocation_permission_context.h.

>
> I will take a closer look at geolocation_dispatcher_host.**cc, the IPC
> methods,
> etc., and check into the following alternatives:
>
> * Revert SendGeolocationPermissionRespo**nse()'s signature, which would
> undo about
> half the changes to this file.
> * Use the ID struct pervasively in this file and do the boxing and unboxing
> closer to where the IPC messages are handled.  This would probably allow
> us to
> nuke the |render_process_id_| member as well.
>
this could be a security issue, because we don't trust the renderer to give
the render process id.

>
>
https://codereview.chromium.**org/11269002/<https://codereview.chromium.org/1...
>

[jam, 2012-10-24 22:49:36]

On Wed, Oct 24, 2012 at 3:48 PM, John Abd-El-Malek <jam@chromium.org> wrote:

>
>
> On Wed, Oct 24, 2012 at 3:33 PM, <pkasting@chromium.org> wrote:
>
>> On 2012/10/24 22:00:08, John Abd-El-Malek wrote:
>>
>>> btw this should go into
>>> content/public/browser/**geolocation_permission_**request_id.h per
>>> convention
>>> of each struct/interface in a separate file.
>>>
>>
>> Sure, I can do that.
>>
>> Where would you most prefer functions like the equality check?  Should I
>> put
>> them in this header file to keep them alongside the struct, since you
>> expressed
>> some unhappiness at that?  Or should I define them on the chrome side if
>> they're
>> unnecessary in content?
>
>
> they don't really belong in content if they're outside the struct. i.e. to
> have a file with this struct, and then the methods right after them, would
> only be a less readable form of having a struct with methods
>
>
>>  I probably would prefer the former, but if it feels too
>> much like a violation of the "no concrete classes" API I can put it
>> elsewhere.
>>
>> (I'll put a function elsewhere anyway if I find its uses are all confined
>> to a
>> single consumer; I'll just define in that .cc file in that case.)
>>
>>
>>  i will ask one question: is there enough benefit to keep it in the
>>> content
>>> api? i.e. for geolocation_disaptcher_host.**cc's
>>> SendGeolocationPermissionRespo**nse the caller creates the struct only
>>> for
>>> that function to unpack it.
>>>
>>
>> I'm not sure precisely what "keep it in the content api" means.  The
>> primary
>> goal is to keep it in geolocation_permission_**context.h.
>>
>
> I meant not use this in the content api, including
> geolocation_permission_context.h.
>

at this point, it honestly doesn't feel worth it to have this structure in
content at all. i.e. it's used in two public methods, if these are left as
is, you can still have most of the benefit to the chrome code.


>
>> I will take a closer look at geolocation_dispatcher_host.**cc, the IPC
>> methods,
>> etc., and check into the following alternatives:
>>
>> * Revert SendGeolocationPermissionRespo**nse()'s signature, which would
>> undo about
>> half the changes to this file.
>> * Use the ID struct pervasively in this file and do the boxing and
>> unboxing
>> closer to where the IPC messages are handled.  This would probably allow
>> us to
>> nuke the |render_process_id_| member as well.
>>
> this could be a security issue, because we don't trust the renderer to
> give the render process id.
>
>>
>>
https://codereview.chromium.**org/11269002/<https://codereview.chromium.org/1...
>>
>
>

[Peter Kasting, 2012-10-24 22:55:25]

On 2012/10/24 22:48:34, John Abd-El-Malek wrote:
> > I'm not sure precisely what "keep it in the content api" means.  The
> > primary
> > goal is to keep it in geolocation_permission_**context.h.
> 
> I meant not use this in the content api, including
> geolocation_permission_context.h.

I'm not willing to accept that outcome, or I wouldn't have proposed the
bare-struct idea in the first place.

I'll put the struct functions somewhere other than a content/ header file,
probably in chrome/.

> > * Use the ID struct pervasively in this file and do the boxing and unboxing
> > closer to where the IPC messages are handled.  This would probably allow
> > us to
> > nuke the |render_process_id_| member as well.
> >
> this could be a security issue, because we don't trust the renderer to give
> the render process id.

I'm not proposing changing the IPC methods themselves.  I won't be passing the
render process ID across IPC.  If the change ends up making sense to do, then
it'll be clearer when you see it in action.  I'm not actually sure yet if it
makes sense.

[Peter Kasting, 2012-10-25 23:01:30]

New snap up.  joi/jknotten, please re-review.

This implements John's suggestion of moving the new class to chrome/ instead of
content/.  The content/ changes are now just misc. leftover cleanups.

This fixes the unit test failure.  One of my new DCHECKs discovered that, during
InfoBarTabHelper teardown, GetInfoBarHelper() was returning NULL but the helper
provided in the "infobar removed" notification was non-NULL.  I changed the code
to just kill all pending requests in this case.

This also does a number of other cleanups, in particular switching
PendingInfoBarRequest to be a class instead of a struct to go along with the
rules we agreed on previously for GeolocationPermissionRequestID.  Normally I'd
break these back down into individual changes but my checkouts are all in a
state of chaos right now; if you review the diffs against patch set 2 I think
most of the changes should be fairly clear.

[jam, 2012-10-26 00:55:31]

content lgtm

[John Knottenbelt, 2012-10-26 12:40:55]

lgtm, thanks again Peter.

[Jói, 2012-10-26 13:13:22]

You already have John's approval for content/, but just for form's sake here is
my LGTM.

Cheers,
Jói