Fix Android strict-mode violation in GeoLocation info bar.

The CL breaks up some of the work done in GeoLocationInfoBar.

Work that needs to be done in UI thread are kept there while other heavy lifing is done by workers.

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

[acleung1, 2013-08-20 22:53:11]

Hey Jonathan, would you mind taking a look at this for me? Thanks!

[joth, 2013-08-20 23:07:21]

This looks suspeciously non thread safe. I only really read the first file.

+cramya,bulch FYI as they probably know
geolocation_confirm_infobar_delegate_android better than me. (I've not dealt in
anger with chrome/ layer code for 12 months so a bit rusty on some of this... :)

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:109:
base::WorkerPool::PostTask(FROM_HERE,
WorkerPool is a bit funny about non-joinable threads and what you can do on it.

It think the general preference is to use
BrowserThread::PostBlockingPoolTaskAndReply() these days.

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:132:
profile_->GetHostContentSettingsMap()->GetContentSetting(
This looks bad. Pretty sure profile_ is not thread safe, and I don't think
content settings map is either.

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:148:
base::Unretained(this), id, requesting_frame, embedder,callback));
why use Unretained() here but not on line 112?

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:183:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
you call this from  ChromeGeolocationPermissionContext::PermissionDecided()
which is called from ChromeGeolocationPermissionContext::DecidePermission()
which is not on UI thread. How can this not assert?

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:207: return
new PermissionQueueController(profile(),
again using profile on non-UI thread?

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
File chrome/browser/geolocation/geolocation_infobar_delegate_android.h (right):

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/geolocation_infobar_delegate_android.h:14: public
base::RefCountedThreadSafe<GeolocationInfoBarDelegateAndroid> {
you can't just mix in ref-counting. The root base class (InfoBarDelegate)
already declares a public destructor so anyone might just delete these objects
outside of the ref-counting protocol

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/geolocation_infobar_delegate_android.h:25: virtual
~GeolocationInfoBarDelegateAndroid();
(likewise you won't want a public d'tor here if you're making this ref-counted)

nit: \n before private:

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/geolocation_infobar_delegate_android.h:36: bool
is_allow_label;
trailing _

[joth, 2013-08-20 23:07:26]

This looks suspiciously non thread safe. I only really read the first file.

+cramya,bulch FYI as they probably know
geolocation_confirm_infobar_delegate_android better than me. (I've not dealt in
anger with chrome/ layer code for 12 months so a bit rusty on some of this... :)

[acleung1, 2013-08-20 23:58:38]

That IS bad! May be I was too aggressive with moving stuff out of the UI thread.

Another approach I had in mind was to move the

IsMasterLocationSettingEnabled() later so the control flow goes like this:



UI:
 ChromeGeolocationPermissionContext::RequestGeolocationPermission
 ...
 All the way to ChromeGeolocationPermissionContext::DecidePermission

Worker:
 Call the two functions that requires database queries in
google_location_settings_helper_, remember the result.

UI:
 CreateInfoBars using those informations.

Do you think that would be better than trying to make this CL thread safe?

[bulach, 2013-08-21 09:28:29]

+miguelg who's doing some infobar refactorings to bring in the translate infobar
and maybe interested in this case..

[Michael van Ouwerkerk, 2013-08-21 10:24:44]

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc (left):

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:181:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
Instead of deleting, could you modify such checks to check for the thread you
are actually expecting? Or at least assert which thread it is not on?

[Miguel Garcia, 2013-08-21 11:46:46]

Just a pass by review on the delegate piece.

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
File chrome/browser/geolocation/geolocation_infobar_delegate.cc (right):

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/geolocation_infobar_delegate.cc:36: DelegateType *
const delegate =
nit: DelegateType* (no space)

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/geolocation_infobar_delegate.cc:37: new
DelegateType(infobar_service, controller, id, requesting_frame,
I thin you can push the Initialize/Show thing to the android delegate only and
extend the ifdef that already exist so the right code is applied per platform,
this is similar to what we do in the auto login infobar delegate
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

That way you avoid the virtual method and polluting the platform agnostic
delegate with extra methods that do nothing.


In any case it seems that these two new methods could be protected right?

[joth, 2013-08-21 19:36:13]

On 21 August 2013 03:24, <mvanouwerkerk@chromium.org> wrote:

>
> https://codereview.chromium.**org/23345004/diff/4001/chrome/**
>
browser/geolocation/chrome_**geolocation_permission_**context.cc<https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation/chrome_geolocation_permission_context.cc>
> File chrome/browser/geolocation/**chrome_geolocation_permission_**
> context.cc
> (left):
>
> https://codereview.chromium.**org/23345004/diff/4001/chrome/**
> browser/geolocation/chrome_**geolocation_permission_**
>
context.cc#oldcode181<https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation/chrome_geolocation_permission_context.cc#oldcode181>
> chrome/browser/geolocation/**chrome_geolocation_permission_**
> context.cc:181:
>
> DCHECK(content::BrowserThread:**:CurrentlyOn(content::**
> BrowserThread::UI));
> Instead of deleting, could you modify such checks to check for the
> thread you are actually expecting? Or at least assert which thread it is
> not on?
>
>
FWIW  Checking you're not on a certain thread is considered a testability
antipattern now (see recent ajwong G+ posting)


>
https://codereview.chromium.**org/23345004/<https://codereview.chromium.org/2...
>

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

[Michael van Ouwerkerk, 2013-08-22 09:52:55]

On 2013/08/21 19:36:13, joth wrote:
> On 21 August 2013 03:24, <mailto:mvanouwerkerk@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.**org/23345004/diff/4001/chrome/**
> >
>
browser/geolocation/chrome_**geolocation_permission_**context.cc<https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation/chrome_geolocation_permission_context.cc>
> > File chrome/browser/geolocation/**chrome_geolocation_permission_**
> > context.cc
> > (left):
> >
> > https://codereview.chromium.**org/23345004/diff/4001/chrome/**
> > browser/geolocation/chrome_**geolocation_permission_**
> >
>
context.cc#oldcode181<https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation/chrome_geolocation_permission_context.cc#oldcode181>
> > chrome/browser/geolocation/**chrome_geolocation_permission_**
> > context.cc:181:
> >
> > DCHECK(content::BrowserThread:**:CurrentlyOn(content::**
> > BrowserThread::UI));
> > Instead of deleting, could you modify such checks to check for the
> > thread you are actually expecting? Or at least assert which thread it is
> > not on?
> >
> >
> FWIW  Checking you're not on a certain thread is considered a testability
> antipattern now (see recent ajwong G+ posting)
> 
> 
> >
>
https://codereview.chromium.**org/23345004/%3Chttps://codereview.chromium.org...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Ok I found the post on DCHECK for threads. Thanks Jonathan :-)

[acleung1, 2013-08-23 23:13:09]

Hi, thanks for the suggestions!

I re-arranged the control flow so it is mostly back the UI thread unless we
really have to.

Please take another look.

Thanks,

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc (left):

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:181:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2013/08/21 10:24:45, Michael van Ouwerkerk wrote:
> Instead of deleting, could you modify such checks to check for the thread you
> are actually expecting? Or at least assert which thread it is not on?

I am moving this back to the UI thread as well.

As for asserting which thread this is not on, I am not sure if we should that
after Jonathan's comment on that being anti-pattern.

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:109:
base::WorkerPool::PostTask(FROM_HERE,
On 2013/08/20 23:07:21, joth wrote:
> WorkerPool is a bit funny about non-joinable threads and what you can do on
it.
> 
> It think the general preference is to use
> BrowserThread::PostBlockingPoolTaskAndReply() these days.

Ok. I removed this and keep the execution of these in the UI thread until later
to avoid the possible race conditions.

I'll use PostBlockingPoolTask later in the code.

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:132:
profile_->GetHostContentSettingsMap()->GetContentSetting(
On 2013/08/20 23:07:21, joth wrote:
> This looks bad. Pretty sure profile_ is not thread safe, and I don't think
> content settings map is either.

Fixed. Kept in UI.

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:148:
base::Unretained(this), id, requesting_frame, embedder,callback));
On 2013/08/20 23:07:21, joth wrote:
> why use Unretained() here but not on line 112?

Removed the early call and kept using unretained here as well.

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:183:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2013/08/20 23:07:21, joth wrote:
> you call this from  ChromeGeolocationPermissionContext::PermissionDecided()
> which is called from ChromeGeolocationPermissionContext::DecidePermission()
> which is not on UI thread. How can this not assert?

Good catch! This should have assert if master location is not enabled.

I changed this back to UI thread as well.

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:207: return
new PermissionQueueController(profile(),
On 2013/08/20 23:07:21, joth wrote:
> again using profile on non-UI thread?

fixed.

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
File chrome/browser/geolocation/geolocation_infobar_delegate.cc (right):

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/geolocation_infobar_delegate.cc:36: DelegateType *
const delegate =
On 2013/08/21 11:46:46, Miguel Garcia wrote:
> nit: DelegateType* (no space)

Done.

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/geolocation_infobar_delegate.cc:37: new
DelegateType(infobar_service, controller, id, requesting_frame,
On 2013/08/21 11:46:46, Miguel Garcia wrote:
> I thin you can push the Initialize/Show thing to the android delegate only and
> extend the ifdef that already exist so the right code is applied per platform,
> this is similar to what we do in the auto login infobar delegate
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> That way you avoid the virtual method and polluting the platform agnostic
> delegate with extra methods that do nothing.
> 
> 
> In any case it seems that these two new methods could be protected right?

It difficult to have a sort of polymorphic behavior that the subclass of
GeolocationInfoBarDelegate can override because users of that class just call
the constructor and nothing else.

I limited the added method to a single one as suggested. I also made Show()
protected as well.

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
File chrome/browser/geolocation/geolocation_infobar_delegate_android.h (right):

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/geolocation_infobar_delegate_android.h:14: public
base::RefCountedThreadSafe<GeolocationInfoBarDelegateAndroid> {
On 2013/08/20 23:07:21, joth wrote:
> you can't just mix in ref-counting. The root base class (InfoBarDelegate)
> already declares a public destructor so anyone might just delete these objects
> outside of the ref-counting protocol

removed.

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/geolocation_infobar_delegate_android.h:25: virtual
~GeolocationInfoBarDelegateAndroid();
On 2013/08/20 23:07:21, joth wrote:
> (likewise you won't want a public d'tor here if you're making this
ref-counted)
> 
> nit: \n before private:


removed instead.

https://codereview.chromium.org/23345004/diff/4001/chrome/browser/geolocation...
chrome/browser/geolocation/geolocation_infobar_delegate_android.h:36: bool
is_allow_label;
On 2013/08/20 23:07:21, joth wrote:
> trailing _

Done.

[bulach, 2013-09-04 18:48:53]

thanks alan! sorry for the delay, I haven't managed to get through all files but
here's some general nits and small suggestions..

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:138: //
setting == ask. Prompt the user in the UI thread now.
hmm, isn't this already on the UI thread as per 124 above? not quite clear why
this is necessary..

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:152:
QueueController()->CreateInfoBarRequest(
nit: indentation

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context.h (right):

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context.h:91:
base::Callback<void(bool)> callback);
nit: indentation

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc
(right):

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:54:
CallSuperDecidePermission,
is it possible to bind to DecidePermission directly?

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:61:
requesting_frame, embedder, callback, false)); }
nit: move } to the next line..

it maybe slightly more readable as:

base::Closure ui_closure;
if (enabled) {
  ui_closure = base::Bind(...);
} else {
  ui_closure = base::Bind(...);
}

// This method is executed from the BlockingPool, post the result
// back to the UI thread.
content::BrowserThread::PostTask(
    content::BrowserThread::UI, FROM_HERE, ui_closure);

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:71:
// strict mode violations.
rather than "the super class implementation requires", probably simplest to:

// Called on the UI thread. However, do the work on a separate thread
// to avoid strict mode violation.
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:75:
base::Unretained(this), id, requesting_frame, embedder, callback));
nit: indentation

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
File chrome/browser/geolocation/geolocation_infobar_delegate.h (right):

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
chrome/browser/geolocation/geolocation_infobar_delegate.h:44: // Make infobar
visible to the user.
not quite clear what this comment mean :)

perhaps something like:

// Override to do some work right before the infobar is
// visible to the user.
virtual BeforeShow(InfoBarService* infobar_service) {} 

then the implementation would call this before the AddInfoBar?

[acleung1, 2013-09-05 21:50:15]

thanks! PTAL.

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:138: //
setting == ask. Prompt the user in the UI thread now.
On 2013/09/04 18:48:53, bulach wrote:
> hmm, isn't this already on the UI thread as per 124 above? not quite clear why
> this is necessary..

Good point. Done.

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:152:
QueueController()->CreateInfoBarRequest(
On 2013/09/04 18:48:53, bulach wrote:
> nit: indentation

Done.

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context.h (right):

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context.h:91:
base::Callback<void(bool)> callback);
On 2013/09/04 18:48:53, bulach wrote:
> nit: indentation

Done.

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc
(right):

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:54:
CallSuperDecidePermission,
On 2013/09/04 18:48:53, bulach wrote:
> is it possible to bind to DecidePermission directly?

Yeah. I was struggling with this and couldn't find a good solution.

Binding DecidePermission will send it through the virtual dispatcher, calling
the child implementation again and creating a infinite loop.

I am happy to take any suggestions to avoid this.

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:61:
requesting_frame, embedder, callback, false)); }
On 2013/09/04 18:48:53, bulach wrote:
> nit: move } to the next line..
> 
> it maybe slightly more readable as:
> 
> base::Closure ui_closure;
> if (enabled) {
>   ui_closure = base::Bind(...);
> } else {
>   ui_closure = base::Bind(...);
> }
> 
> // This method is executed from the BlockingPool, post the result
> // back to the UI thread.
> content::BrowserThread::PostTask(
>     content::BrowserThread::UI, FROM_HERE, ui_closure);

Done.

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:71:
// strict mode violations.
On 2013/09/04 18:48:53, bulach wrote:
> rather than "the super class implementation requires", probably simplest to:
> 
> // Called on the UI thread. However, do the work on a separate thread
> // to avoid strict mode violation.
> DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

Done.

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:75:
base::Unretained(this), id, requesting_frame, embedder, callback));
On 2013/09/04 18:48:53, bulach wrote:
> nit: indentation

Done.

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
File chrome/browser/geolocation/geolocation_infobar_delegate.h (right):

https://codereview.chromium.org/23345004/diff/36001/chrome/browser/geolocatio...
chrome/browser/geolocation/geolocation_infobar_delegate.h:44: // Make infobar
visible to the user.
On 2013/09/04 18:48:53, bulach wrote:
> not quite clear what this comment mean :)
> 
> perhaps something like:
> 
> // Override to do some work right before the infobar is
> // visible to the user.
> virtual BeforeShow(InfoBarService* infobar_service) {} 
> 
> then the implementation would call this before the AddInfoBar?

Oops. Strayed comments from a delete.

I think this addresses the issue Miguel mentioned earlier about introducing too
many functions. Constructor -> show() seems simpiler

[bulach, 2013-09-12 09:18:29]

thanks alan! I'm afraid the rabbit hole goes a bit deeper in the infobar side,
sorry... :-( some nits and suggestions below, hope it'll help..

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/android/go...
File chrome/browser/android/google_location_settings_helper.h (right):

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/android/go...
chrome/browser/android/google_location_settings_helper.h:20: virtual std::string
GetAcceptButtonLabel(bool allow) = 0;
this would break downstream, need to do a two-sided patch (i.e, add a new method
with the param, non-virtual, a TODO to remove this method, then after it rolls
downstream, remove the the version without param...).

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:151:
QueueController()->CreateInfoBarRequest(
nit: wrong indent

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc
(right):

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:25:
void ChromeGeolocationPermissionContextAndroid::CallSuperDecidePermission(
thanks for the explanation!
how about "ProceedDecidePermissionFlow"? more below..

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:48:
// go back to the UI thread for the rest of the bookkeeping.
Perhaps a more detailed explanation would help clarify:

// The flow for geolocation permission on android is:
// - ChromeGeolocationPermissionContextAndroid::DecidePermission
// intercepts the flow in the UI thread, and posts task
// to the blocking pool to CheckMasterLocation (in order to
// avoid strict-mode violation).
// - At this point the master location permission is either:
// -- enabled, in which we case it proceeds the normal flow
// via ChromeGeolocationPermissionContext (which may create infobars, etc.).
// -- disabled, in which case the permission is already decided.
//
// In either case, ChromeGeolocationPermissionContext expects these
// in the UI thread.

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:52:
&ChromeGeolocationPermissionContextAndroid::CallSuperDecidePermission,
so if you agree with the comment above, I think "ProceedDecidePermissionFlow"
would be clearer..

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:64:
content::BrowserThread::UI, FROM_HERE,ui_closure);
nit: space after comma, s/,ui_clo/, ui_clo/

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:79:
base::Unretained(this), id, requesting_frame, embedder, callback));
nit: indent

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
File chrome/browser/geolocation/geolocation_infobar_delegate_android.cc (right):

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
chrome/browser/geolocation/geolocation_infobar_delegate_android.cc:42:
base::Unretained(this),
this won't work... :(
the infobar could be deleted before the worker pool executed, and then this
callback would deref a dangling pointer.. the infobar is mostly a UI object and
ideally wouldn't know anything about the underlying thread model.. also, I think
the "Accept()" would have a strict-mode violation too, since it's calling on the
google_location_settings_helper_ as well...

at the end of the day, this needs one string and one boolean :)
we should pick those up in ChromeGeolocationPermissionContextAndroid and then
plumb them through here..

[acleung1, 2013-09-17 00:08:44]

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/android/go...
File chrome/browser/android/google_location_settings_helper.h (right):

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/android/go...
chrome/browser/android/google_location_settings_helper.h:20: virtual std::string
GetAcceptButtonLabel(bool allow) = 0;
On 2013/09/12 09:18:29, bulach wrote:
> this would break downstream, need to do a two-sided patch (i.e, add a new
method
> with the param, non-virtual, a TODO to remove this method, then after it rolls
> downstream, remove the the version without param...).

Ya. I have a downstream patch that I'll be tagging the merger to merge. That
change is rather small so I wanted to get this one in a good state before
sending the other out for review.

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:151:
QueueController()->CreateInfoBarRequest(
On 2013/09/12 09:18:29, bulach wrote:
> nit: wrong indent

Done.

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc
(right):

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:25:
void ChromeGeolocationPermissionContextAndroid::CallSuperDecidePermission(
On 2013/09/12 09:18:29, bulach wrote:
> thanks for the explanation!
> how about "ProceedDecidePermissionFlow"? more below..

Done.

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:48:
// go back to the UI thread for the rest of the bookkeeping.
On 2013/09/12 09:18:29, bulach wrote:
> Perhaps a more detailed explanation would help clarify:
> 
> // The flow for geolocation permission on android is:
> // - ChromeGeolocationPermissionContextAndroid::DecidePermission
> // intercepts the flow in the UI thread, and posts task
> // to the blocking pool to CheckMasterLocation (in order to
> // avoid strict-mode violation).
> // - At this point the master location permission is either:
> // -- enabled, in which we case it proceeds the normal flow
> // via ChromeGeolocationPermissionContext (which may create infobars, etc.).
> // -- disabled, in which case the permission is already decided.
> //
> // In either case, ChromeGeolocationPermissionContext expects these
> // in the UI thread.

Done.

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:52:
&ChromeGeolocationPermissionContextAndroid::CallSuperDecidePermission,
On 2013/09/12 09:18:29, bulach wrote:
> so if you agree with the comment above, I think "ProceedDecidePermissionFlow"
> would be clearer..

Done.

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:64:
content::BrowserThread::UI, FROM_HERE,ui_closure);
On 2013/09/12 09:18:29, bulach wrote:
> nit: space after comma, s/,ui_clo/, ui_clo/

Done.

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:79:
base::Unretained(this), id, requesting_frame, embedder, callback));
On 2013/09/12 09:18:29, bulach wrote:
> nit: indent

Done.

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
File chrome/browser/geolocation/geolocation_infobar_delegate_android.cc (right):

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
chrome/browser/geolocation/geolocation_infobar_delegate_android.cc:42:
base::Unretained(this),
On 2013/09/12 09:18:29, bulach wrote:
> this won't work... :(
> the infobar could be deleted before the worker pool executed, and then this
> callback would deref a dangling pointer.. the infobar is mostly a UI object
and
> ideally wouldn't know anything about the underlying thread model.. also, I
think
> the "Accept()" would have a strict-mode violation too, since it's calling on
the
> google_location_settings_helper_ as well...
> 
> at the end of the day, this needs one string and one boolean :)
> we should pick those up in ChromeGeolocationPermissionContextAndroid and then
> plumb them through here..

Hmm I understood the concern but I don't really follow your suggestions.

What you are suggesting that is we let ChromeGeolocationPermissionContextAndroid
do that work of CheckLocationSetting and pass these down to
GeolocationInfoBarDelegateAndroid? WOuld that introduce a dependency between
those two class that wasn't there before?

Or am I misunderstand your suggestion?

[bulach, 2013-10-16 10:47:10]

very sorry for the delay, went under my radar... :( feel free to ping me if it
takes more than a couple of days!!

some clarifications below, but the general idea is that
chrome_geolocation_permission_context_android.cc is the one orchestrating all
the threads, and the infobar will get all the information it needs at creation
time, and live entirely within the UI thread.. please let me know your thoughts!

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc
(right):

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:94:
callback);
here, I think this could pass the boolean and string obtained above down to the
infobar... that way, the infobar is entirely in the UI thread, and all the
orchestration happens here.

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
File chrome/browser/geolocation/geolocation_infobar_delegate_android.cc (right):

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
chrome/browser/geolocation/geolocation_infobar_delegate_android.cc:42:
base::Unretained(this),
On 2013/09/17 00:08:45, acleung1 wrote:
> On 2013/09/12 09:18:29, bulach wrote:
> > this won't work... :(
> > the infobar could be deleted before the worker pool executed, and then this
> > callback would deref a dangling pointer.. the infobar is mostly a UI object
> and
> > ideally wouldn't know anything about the underlying thread model.. also, I
> think
> > the "Accept()" would have a strict-mode violation too, since it's calling on
> the
> > google_location_settings_helper_ as well...
> > 
> > at the end of the day, this needs one string and one boolean :)
> > we should pick those up in ChromeGeolocationPermissionContextAndroid and
then
> > plumb them through here..
> 
> Hmm I understood the concern but I don't really follow your suggestions.
> 
> What you are suggesting that is we let
ChromeGeolocationPermissionContextAndroid
> do that work of CheckLocationSetting and pass these down to
> GeolocationInfoBarDelegateAndroid? WOuld that introduce a dependency between
> those two class that wasn't there before?
> 
> Or am I misunderstand your suggestion?

see above..

[acleung1, 2013-12-12 23:38:30]

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc
(right):

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:94:
callback);
On 2013/10/16 10:47:11, bulach wrote:
> here, I think this could pass the boolean and string obtained above down to
the
> infobar... that way, the infobar is entirely in the UI thread, and all the
> orchestration happens here.

I don't think that's going to solve the issue we have. There are two strict mode
violations. The first one  involves the "Allow Label" which the info bar needs
to know when it is being constructed.

https://codereview.chromium.org/23345004/diff/40001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:94:
callback);
On 2013/10/16 10:47:11, bulach wrote:
> here, I think this could pass the boolean and string obtained above down to
the
> infobar... that way, the infobar is entirely in the UI thread, and all the
> orchestration happens here.

I dont' think this will solve our problem because there are two strict mode
violations this is trying to fix. One of them being the Allow Label that is
needed when the infobar is being constructed. I don't really see a good way to
delay it to here.

[bulach, 2014-01-15 16:22:07]

hey alan, terribly sorry for the delay... I can't see patchset #9, so some
comments may already have been addressed...
I'm clarifying my previous comment, I think it'd be great to avoid making
geolocation_infobar_delegate multi-threaded, I think we can capture all the
information it needs right at the permission_context level (which is already
multi-threaded)..

wdyt?

https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:13: #include
"base/threading/worker_pool.h"
nit: not needed?

https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:151:
QueueController()->CreateInfoBarRequest(
nit: wrong indent, should be just 2

https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc
(right):

https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:45:
google_location_settings_helper_->IsMasterLocationSettingEnabled();
I think my previous comment was that we could also check it here for:

"is_allow_label = google_location_settings_helper_->IsAllowLabel();" and 
google_location_settings_helper_>GetAcceptButtonLabel(is_allow_label_)

then pass to the ui_closure and then pass it all the way down to the infobar... 
that would remove the changes to infobar_delegate, that object should be just on
the UI thread, and any tasks / orchestration should be done before that object
is created...

wdyt?

[acleung1, 2014-01-24 05:48:15]

On 2014/01/15 16:22:07, bulach_ooo_until_feb_2 wrote:
> hey alan, terribly sorry for the delay... I can't see patchset #9, so some
> comments may already have been addressed...
> I'm clarifying my previous comment, I think it'd be great to avoid making
> geolocation_infobar_delegate multi-threaded, I think we can capture all the
> information it needs right at the permission_context level (which is already
> multi-threaded)..
> 
> wdyt?
> 
>
https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
> File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
> (right):
> 
>
https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
> chrome/browser/geolocation/chrome_geolocation_permission_context.cc:13:
#include
> "base/threading/worker_pool.h"
> nit: not needed?
> 
>
https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
> chrome/browser/geolocation/chrome_geolocation_permission_context.cc:151:
> QueueController()->CreateInfoBarRequest(
> nit: wrong indent, should be just 2
> 
>
https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
> File
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc
> (right):
> 
>
https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
>
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:45:
> google_location_settings_helper_->IsMasterLocationSettingEnabled();
> I think my previous comment was that we could also check it here for:
> 
> "is_allow_label = google_location_settings_helper_->IsAllowLabel();" and 
> google_location_settings_helper_>GetAcceptButtonLabel(is_allow_label_)
> 
> then pass to the ui_closure and then pass it all the way down to the
infobar... 
> that would remove the changes to infobar_delegate, that object should be just
on
> the UI thread, and any tasks / orchestration should be done before that object
> is created...
> 
> wdyt?

Ok. I understand the idea and I think it make sense.

However, I am not sure how the AcceptButtonLabel can be passed all the way back
to the infobar_delegate.

The InfobarDelegate is created by the PermissionQueueController. That is the
point where the InfoBarDelegate and
the PermissionContext "meet up" in the call stack. The best place to pass the
string to the InfoBarDelegat will be
adding an extra string value to CreateInfoBarRequest. Which means we have to add
that to PermissionQueueController.

This doesn't feel right considered that PermissionQueueController should be a
high level controller that isn't aware of
stuff like the OK label on it's infobar buttons.

Or I am not seeing something that can make the connection easiler?

-Alan

[bulach, 2014-01-24 06:08:00]

On 2014/01/24 05:48:15, acleung1 wrote:
> On 2014/01/15 16:22:07, bulach_ooo_until_feb_2 wrote:
> > hey alan, terribly sorry for the delay... I can't see patchset #9, so some
> > comments may already have been addressed...
> > I'm clarifying my previous comment, I think it'd be great to avoid making
> > geolocation_infobar_delegate multi-threaded, I think we can capture all the
> > information it needs right at the permission_context level (which is already
> > multi-threaded)..
> > 
> > wdyt?
> > 
> >
>
https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
> > File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
> > chrome/browser/geolocation/chrome_geolocation_permission_context.cc:13:
> #include
> > "base/threading/worker_pool.h"
> > nit: not needed?
> > 
> >
>
https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
> > chrome/browser/geolocation/chrome_geolocation_permission_context.cc:151:
> > QueueController()->CreateInfoBarRequest(
> > nit: wrong indent, should be just 2
> > 
> >
>
https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
> > File
> chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
> >
>
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:45:
> > google_location_settings_helper_->IsMasterLocationSettingEnabled();
> > I think my previous comment was that we could also check it here for:
> > 
> > "is_allow_label = google_location_settings_helper_->IsAllowLabel();" and 
> > google_location_settings_helper_>GetAcceptButtonLabel(is_allow_label_)
> > 
> > then pass to the ui_closure and then pass it all the way down to the
> infobar... 
> > that would remove the changes to infobar_delegate, that object should be
just
> on
> > the UI thread, and any tasks / orchestration should be done before that
object
> > is created...
> > 
> > wdyt?
> 
> Ok. I understand the idea and I think it make sense.
> 
> However, I am not sure how the AcceptButtonLabel can be passed all the way
back
> to the infobar_delegate.
> 
> The InfobarDelegate is created by the PermissionQueueController. That is the
> point where the InfoBarDelegate and
> the PermissionContext "meet up" in the call stack. The best place to pass the
> string to the InfoBarDelegat will be
> adding an extra string value to CreateInfoBarRequest. Which means we have to
add
> that to PermissionQueueController.
> 
> This doesn't feel right considered that PermissionQueueController should be a
> high level controller that isn't aware of
> stuff like the OK label on it's infobar buttons.
> 
> Or I am not seeing something that can make the connection easiler?
> 
> -Alan

I suppose we have to pick our poison here :)
The options are:
1) Make PermissionQueueController more aware of UI elements.
- ideally, it shouldn't
+ it already calls GeolocationInfoBarDelegate::Create, so passing through extra
arguments to that function via its  CreateInfoBarRequest doesn't look too
terrible...
we may make it more generic, so potentially other modules would be able to have
such finer grained control over the UI, i.e., make
PermissionController::CreateInfoBarRequest take an extra struct, say, UiElements
{ std::string ok_button_label; std::string cancel_button_label;}, etc...

2) Make the geolocation infobar more thread-aware
- the UI objects normally live strictly in the UI thread, so this would make
that object more complicated
+ you already have the code :) (but the base::Unretained(this) won't work there,
so we'd still need to make it correct).

miguel / michael have played with infobars more recently than I did, so I'd like
to hear their opinions..
I think going with (1) would be slightly simpler and more extensible over
time... wdyt?

[acleung1, 2014-02-14 05:52:55]

Sounds good. I have taken approach #1. Please take a look.

On 2014/01/24 06:08:00, bulach wrote:
> On 2014/01/24 05:48:15, acleung1 wrote:
> > On 2014/01/15 16:22:07, bulach_ooo_until_feb_2 wrote:
> > > hey alan, terribly sorry for the delay... I can't see patchset #9, so some
> > > comments may already have been addressed...
> > > I'm clarifying my previous comment, I think it'd be great to avoid making
> > > geolocation_infobar_delegate multi-threaded, I think we can capture all
the
> > > information it needs right at the permission_context level (which is
already
> > > multi-threaded)..
> > > 
> > > wdyt?
> > > 
> > >
> >
>
https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
> > > File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
> > > chrome/browser/geolocation/chrome_geolocation_permission_context.cc:13:
> > #include
> > > "base/threading/worker_pool.h"
> > > nit: not needed?
> > > 
> > >
> >
>
https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
> > > chrome/browser/geolocation/chrome_geolocation_permission_context.cc:151:
> > > QueueController()->CreateInfoBarRequest(
> > > nit: wrong indent, should be just 2
> > > 
> > >
> >
>
https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
> > > File
> > chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
> > >
> >
>
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:45:
> > > google_location_settings_helper_->IsMasterLocationSettingEnabled();
> > > I think my previous comment was that we could also check it here for:
> > > 
> > > "is_allow_label = google_location_settings_helper_->IsAllowLabel();" and 
> > > google_location_settings_helper_>GetAcceptButtonLabel(is_allow_label_)
> > > 
> > > then pass to the ui_closure and then pass it all the way down to the
> > infobar... 
> > > that would remove the changes to infobar_delegate, that object should be
> just
> > on
> > > the UI thread, and any tasks / orchestration should be done before that
> object
> > > is created...
> > > 
> > > wdyt?
> > 
> > Ok. I understand the idea and I think it make sense.
> > 
> > However, I am not sure how the AcceptButtonLabel can be passed all the way
> back
> > to the infobar_delegate.
> > 
> > The InfobarDelegate is created by the PermissionQueueController. That is the
> > point where the InfoBarDelegate and
> > the PermissionContext "meet up" in the call stack. The best place to pass
the
> > string to the InfoBarDelegat will be
> > adding an extra string value to CreateInfoBarRequest. Which means we have to
> add
> > that to PermissionQueueController.
> > 
> > This doesn't feel right considered that PermissionQueueController should be
a
> > high level controller that isn't aware of
> > stuff like the OK label on it's infobar buttons.
> > 
> > Or I am not seeing something that can make the connection easiler?
> > 
> > -Alan
> 
> I suppose we have to pick our poison here :)
> The options are:
> 1) Make PermissionQueueController more aware of UI elements.
> - ideally, it shouldn't
> + it already calls GeolocationInfoBarDelegate::Create, so passing through
extra
> arguments to that function via its  CreateInfoBarRequest doesn't look too
> terrible...
> we may make it more generic, so potentially other modules would be able to
have
> such finer grained control over the UI, i.e., make
> PermissionController::CreateInfoBarRequest take an extra struct, say,
UiElements
> { std::string ok_button_label; std::string cancel_button_label;}, etc...
> 
> 2) Make the geolocation infobar more thread-aware
> - the UI objects normally live strictly in the UI thread, so this would make
> that object more complicated
> + you already have the code :) (but the base::Unretained(this) won't work
there,
> so we'd still need to make it correct).
> 
> miguel / michael have played with infobars more recently than I did, so I'd
like
> to hear their opinions..
> I think going with (1) would be slightly simpler and more extensible over
> time... wdyt?

[bulach, 2014-02-14 11:36:47]

lgtm, thanks a ton putting up with my slow comments!

just some nits and small suggestions below.

I added michael/tim as the new owners for geolocation..

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_queue_controller.cc (right):

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/content_s...
chrome/browser/content_settings/permission_queue_controller.cc:68: std::string
accept_button_label_;
nit: move this after embedded, to keep the same order as the ctor

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_queue_controller.h (right):

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/content_s...
chrome/browser/content_settings/permission_queue_controller.h:38: const
std::string accept_button_label,
nit: &

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:235:
base::Bind(&ChromeGeolocationPermissionContext::NotifyPermissionSet,
nit: >80cols

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:235:
base::Bind(&ChromeGeolocationPermissionContext::NotifyPermissionSet,
nit: >80cols

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:247:
QueueController()->CreateInfoBarRequest(
nit: unindent

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
File chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc
(right):

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:67:
std::string accept_button_label =
nit: unindent here and above

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:71:
base::Unretained(this), web_contents, id, requesting_frame, embedder,
nit: I suppose the base class is RefCountedThreadSafe, so could probably pass
just "this" without the Unretained? same for 76 and 100 below..

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
File chrome/browser/geolocation/chrome_geolocation_permission_context_android.h
(right):

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.h:33:
const std::string accept_button_label,
nit: &

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.h:46:
const std::string accept_button_label,
nit: &

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_infobar_delegate.cc (right):

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_infobar_delegate.cc:75:
GeolocationInfoBarDelegate* const delegate = new DelegateType(
nit: is it possible to keep everything inline?

return infobar_service->AddInfoBar(
    ConfirmInfoBarDelegate::CreateInfoBar(
        scoped_ptr<ConfirmInfoBarDelegate>(
            new DelegateType(....)));

the .release in 81 just to put in another scoped_ptr in 82 looks a bit
complicated..

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_infobar_delegate_android.h (right):

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_infobar_delegate_android.h:21: const
std::string accept_button_label);
nit: &

[acleung1, 2014-02-21 21:18:00]

https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:13: #include
"base/threading/worker_pool.h"
On 2014/01/15 16:22:07, bulach_ooo_until_feb_2 wrote:
> nit: not needed?

Done.

https://codereview.chromium.org/23345004/diff/49001/chrome/browser/geolocatio...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:151:
QueueController()->CreateInfoBarRequest(
On 2014/01/15 16:22:07, bulach_ooo_until_feb_2 wrote:
> nit: wrong indent, should be just 2

Isn't this part of an expression that was divided into two lines?

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_queue_controller.cc (right):

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/content_s...
chrome/browser/content_settings/permission_queue_controller.cc:68: std::string
accept_button_label_;
On 2014/02/14 11:36:48, bulach wrote:
> nit: move this after embedded, to keep the same order as the ctor

Done.

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_queue_controller.h (right):

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/content_s...
chrome/browser/content_settings/permission_queue_controller.h:38: const
std::string accept_button_label,
On 2014/02/14 11:36:48, bulach wrote:
> nit: &

Done.

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:235:
base::Bind(&ChromeGeolocationPermissionContext::NotifyPermissionSet,
On 2014/02/14 11:36:48, bulach wrote:
> nit: >80cols

Done.

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:235:
base::Bind(&ChromeGeolocationPermissionContext::NotifyPermissionSet,
On 2014/02/14 11:36:48, bulach wrote:
> nit: >80cols

Done.

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:247:
QueueController()->CreateInfoBarRequest(
On 2014/02/14 11:36:48, bulach wrote:
> nit: unindent

Done.

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
File chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc
(right):

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:67:
std::string accept_button_label =
On 2014/02/14 11:36:48, bulach wrote:
> nit: unindent here and above

Done.

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.cc:71:
base::Unretained(this), web_contents, id, requesting_frame, embedder,
On 2014/02/14 11:36:48, bulach wrote:
> nit: I suppose the base class is RefCountedThreadSafe, so could probably pass
> just "this" without the Unretained? same for 76 and 100 below..

Done.

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
File chrome/browser/geolocation/chrome_geolocation_permission_context_android.h
(right):

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.h:33:
const std::string accept_button_label,
On 2014/02/14 11:36:48, bulach wrote:
> nit: &

Done.

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
chrome/browser/geolocation/chrome_geolocation_permission_context_android.h:46:
const std::string accept_button_label,
On 2014/02/14 11:36:48, bulach wrote:
> nit: &

Done.

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_infobar_delegate.cc (right):

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_infobar_delegate.cc:75:
GeolocationInfoBarDelegate* const delegate = new DelegateType(
On 2014/02/14 11:36:48, bulach wrote:
> nit: is it possible to keep everything inline?
> 
> return infobar_service->AddInfoBar(
>     ConfirmInfoBarDelegate::CreateInfoBar(
>         scoped_ptr<ConfirmInfoBarDelegate>(
>             new DelegateType(....)));
> 
> the .release in 81 just to put in another scoped_ptr in 82 looks a bit
> complicated..

My thoughts were that it was a bit too long as one line. I think I'll keep it as
its.

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_infobar_delegate_android.h (right):

https://codereview.chromium.org/23345004/diff/409001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_infobar_delegate_android.h:21: const
std::string accept_button_label);
On 2014/02/14 11:36:48, bulach wrote:
> nit: &

Done.

[acleung1, 2014-02-21 21:30:03]

Hi markusheintz fischman:

I am missing OWNER approval from the following files:

    chrome/browser/content_settings/permission_queue_controller.cc
    chrome/browser/content_settings/permission_queue_controller.h
    chrome/browser/media/chrome_midi_permission_context.cc
    chrome/browser/media/protected_media_identifier_permission_context.cc

Would you mind taking a look for me?

[acleung1, 2014-02-21 21:33:02]

xians: Would you mind taking a look at the changes in:

    chrome/browser/media/chrome_midi_permission_context.cc
    chrome/browser/media/protected_media_identifier_permission_context.cc

Thanks.

On 2014/02/21 21:30:03, acleung1 wrote:
> Hi markusheintz fischman:
> 
> I am missing OWNER approval from the following files:
> 
>     chrome/browser/content_settings/permission_queue_controller.cc
>     chrome/browser/content_settings/permission_queue_controller.h
>     chrome/browser/media/chrome_midi_permission_context.cc
>     chrome/browser/media/protected_media_identifier_permission_context.cc
> 
> Would you mind taking a look for me?

[no longer working on chromium, 2014-02-23 21:12:34]

lgtm with nits.

[Ami GONE FROM CHROMIUM, 2014-02-24 22:37:06]

chrome/browser/media RS LGTM

[no longer working on chromium, 2014-02-25 11:21:33]

ah, I forgot to publish the nits. Sorry for the delay.

https://codereview.chromium.org/23345004/diff/1229001/chrome/browser/media/ch...
File chrome/browser/media/chrome_midi_permission_context.cc (right):

https://codereview.chromium.org/23345004/diff/1229001/chrome/browser/media/ch...
chrome/browser/media/chrome_midi_permission_context.cc:199: id,
requesting_frame, embedder, "", base::Bind(
nit, use std::string()

https://codereview.chromium.org/23345004/diff/1229001/chrome/browser/media/pr...
File chrome/browser/media/protected_media_identifier_permission_context.cc
(right):

https://codereview.chromium.org/23345004/diff/1229001/chrome/browser/media/pr...
chrome/browser/media/protected_media_identifier_permission_context.cc:124: "",
ditto.

[acleung1, 2014-02-25 23:15:44]

Thanks!

https://codereview.chromium.org/23345004/diff/1229001/chrome/browser/media/ch...
File chrome/browser/media/chrome_midi_permission_context.cc (right):

https://codereview.chromium.org/23345004/diff/1229001/chrome/browser/media/ch...
chrome/browser/media/chrome_midi_permission_context.cc:199: id,
requesting_frame, embedder, "", base::Bind(
On 2014/02/25 11:21:34, xians1 wrote:
> nit, use std::string()

Done.

https://codereview.chromium.org/23345004/diff/1229001/chrome/browser/media/pr...
File chrome/browser/media/protected_media_identifier_permission_context.cc
(right):

https://codereview.chromium.org/23345004/diff/1229001/chrome/browser/media/pr...
chrome/browser/media/protected_media_identifier_permission_context.cc:124: "",
On 2014/02/25 11:21:34, xians1 wrote:
> ditto.

Done.

[acleung1, 2014-02-26 21:07:07]

The CQ bit was checked by acleung@chromium.org

[commit-bot: I haz the power, 2014-02-26 21:08:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acleung@chromium.org/23345004/1469001

[commit-bot: I haz the power, 2014-02-26 21:34:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-26 21:34:40]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[markusheintz_, 2014-02-26 23:45:50]

LGTM

[acleung1, 2014-02-27 00:33:25]

The CQ bit was checked by acleung@chromium.org

[commit-bot: I haz the power, 2014-02-27 00:38:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acleung@chromium.org/23345004/1469001

[acleung1, 2014-02-27 06:02:33]

The CQ bit was unchecked by acleung@chromium.org

[acleung1, 2014-02-27 06:02:37]

The CQ bit was checked by acleung@chromium.org

[commit-bot: I haz the power, 2014-02-27 13:03:06]

Change committed as 253765