Finish implementing the geolocation infobar; adds a Learn more link pointing ...

Finish implementing the geolocation infobar; adds a Learn more link pointing to the (placeholder) help center page.
This extends ConfirmInfoBar to support to have optional link support (simple support, i.e. not the inserted mid-label link that LinkInfoBar sports)

Note 1: this does not exactly match the mock; the allow/deny buttons and link are swapped. I think this looks nicer, is more consistent with other confirm infobars, and happens to be easier to code
Note 2: linux & mac will need follow-up CLs, but will simply ignore the link in the meantime

BUG=11246
TEST=browser_tests.exe --gtest_filter=Geol*



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

[joth, 2010-03-17 19:34:30]

jcampan, I think you're the infobar expert, would you mind taking a look over
this for me? at least with the overall direction, and let me know if you'd
prefer it factored another way.
(rough mocks for the intent here are in the design doc:
http://docs.google.com/View?id=dfbnm49n_0dpc7pxpx)

Cheers!
Joth

[jcampan, 2010-03-17 23:26:20]

http://codereview.chromium.org/1037006/diff/12001/13005
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

http://codereview.chromium.org/1037006/diff/12001/13005#newcode42
chrome/browser/geolocation/geolocation_permission_context.cc:42:
tab_contents_(tab_contents), context_(context),
Nit: I think the Google C++ style guide requires each initializer to be on its
own line.

http://codereview.chromium.org/1037006/diff/12001/13005#newcode99
chrome/browser/geolocation/geolocation_permission_context.cc:99: };
DISALLOW_COPY_...

http://codereview.chromium.org/1037006/diff/12001/13002
File chrome/browser/views/infobars/infobars.cc (right):

http://codereview.chromium.org/1037006/diff/12001/13002#newcode451
chrome/browser/views/infobars/infobars.cc:451: AddChildView(link_);
Should this AddChildView be added to the ConfirmInfoBar::Init() method?

http://codereview.chromium.org/1037006/diff/12001/13002#newcode458
chrome/browser/views/infobars/infobars.cc:458: delete link_;
I think link_ will be deleted twice in that case, since it has been added as a
child view, it gets deleted already when the View destructor is run. Moving
AddChildView(link_) above to init would fix this.

[joth, 2010-03-18 11:10:45]

Thanks for the review. I'll try and get this committed now unless you have
comments Marcus. (Or happy to do follow up change with any other comments
arising)

http://codereview.chromium.org/1037006/diff/12001/13005
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

http://codereview.chromium.org/1037006/diff/12001/13005#newcode42
chrome/browser/geolocation/geolocation_permission_context.cc:42:
tab_contents_(tab_contents), context_(context),
On 2010/03/17 23:26:21, jcampan wrote:
> Nit: I think the Google C++ style guide requires each initializer to be on its
> own line.

Done
Google style guide is vague on how many items allowed per line, but it seems
consistent with Chrome style to do one per line.
(I think the colon had wrong indent too)

http://codereview.chromium.org/1037006/diff/12001/13005#newcode99
chrome/browser/geolocation/geolocation_permission_context.cc:99: };
On 2010/03/17 23:26:21, jcampan wrote:
> DISALLOW_COPY_...

Done.
(DISALLOW_IMPLICIT_... is suitable here)

http://codereview.chromium.org/1037006/diff/12001/13002
File chrome/browser/views/infobars/infobars.cc (right):

http://codereview.chromium.org/1037006/diff/12001/13002#newcode451
chrome/browser/views/infobars/infobars.cc:451: AddChildView(link_);
On 2010/03/17 23:26:21, jcampan wrote:
> Should this AddChildView be added to the ConfirmInfoBar::Init() method?

Done.

http://codereview.chromium.org/1037006/diff/12001/13002#newcode458
chrome/browser/views/infobars/infobars.cc:458: delete link_;
On 2010/03/17 23:26:21, jcampan wrote:
> I think link_ will be deleted twice in that case, since it has been added as a
> child view, it gets deleted already when the View destructor is run. Moving
> AddChildView(link_) above to init would fix this.

Done.

[bulach, 2010-03-18 12:13:23]

LGTM

thanks joth! few nits and general comments below:

http://codereview.chromium.org/1037006/diff/27001/28002
File chrome/browser/views/infobars/infobars.cc (right):

http://codereview.chromium.org/1037006/diff/27001/28002#newcode512
chrome/browser/views/infobars/infobars.cc:512: // Now append the link link to
the label's right edge.
typo, link link

http://codereview.chromium.org/1037006/diff/27001/28002#newcode515
chrome/browser/views/infobars/infobars.cc:515: link_->SetBounds(link_x,
OffsetY(this, link_ps), link_w, link_ps.height());
this is leaking the information about what lays out where, with this
base-base-call, do stuff, base-call, do more stuff.. not sure how to fix though.

http://codereview.chromium.org/1037006/diff/27001/28002#newcode545
chrome/browser/views/infobars/infobars.cc:545: DCHECK(ok_button_);
since this is initialized in the ctor, it seems unnecessary to check here..

[joth, 2010-03-18 12:30:57]

Cheers!
On 2010/03/18 12:13:23, bulach wrote:
> LGTM
> 
> thanks joth! few nits and general comments below:
> 
> http://codereview.chromium.org/1037006/diff/27001/28002
> File chrome/browser/views/infobars/infobars.cc (right):
> 
> http://codereview.chromium.org/1037006/diff/27001/28002#newcode512
> chrome/browser/views/infobars/infobars.cc:512: // Now append the link link to
> the label's right edge.
> typo, link link
> 
> 
DONE

http://codereview.chromium.org/1037006/diff/27001/28002#newcode515
> chrome/browser/views/infobars/infobars.cc:515: link_->SetBounds(link_x,
> OffsetY(this, link_ps), link_w, link_ps.height());
> this is leaking the information about what lays out where, with this
> base-base-call, do stuff, base-call, do more stuff.. not sure how to fix
though.
> 
> 
Added some commentary of the overall flow

http://codereview.chromium.org/1037006/diff/27001/28002#newcode545
> chrome/browser/views/infobars/infobars.cc:545: DCHECK(ok_button_);
> since this is initialized in the ctor, it seems unnecessary to check here..

Done

[jcampan, 2010-03-18 16:00:07]

LGTM