Implementing native chromium GeolocationPermissionContext

Implementing native chromium GeolocationPermissionContext


BUG=


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

[Kristian Monsen, 2013-01-03 08:04:15]

Not ready to submit yet, but it is growing large, and a bit complicated in areas
I am not too familiar with so some early feedback about direction would be nice.

[boliu, 2013-01-03 08:14:37]

https://chromiumcodereview.appspot.com/11763002/diff/4001/android_webview/nat...
File android_webview/native/aw_contents.cc (right):

https://chromiumcodereview.appspot.com/11763002/diff/4001/android_webview/nat...
android_webview/native/aw_contents.cc:179:
aw_browser_context->set_geolocation_permissions_context(
BrowserContext outlives AwContents, it's equivalent to profiles in chrome, or
something like normal vs incognito mode that contains AwContents. From this, it
looks like AwBrowserContext can directly create and own this in the constructor.
(If it's a DEPS/layering violation, you can pass it in from
AwMainDelegate->AwContentBrowserClient->AwBrowerContext)

[Kristian Monsen, 2013-01-03 08:37:58]

https://chromiumcodereview.appspot.com/11763002/diff/4001/android_webview/nat...
File android_webview/native/aw_contents.cc (right):

https://chromiumcodereview.appspot.com/11763002/diff/4001/android_webview/nat...
android_webview/native/aw_contents.cc:179:
aw_browser_context->set_geolocation_permissions_context(
On 2013/01/03 08:14:38, boliu wrote:
> BrowserContext outlives AwContents, it's equivalent to profiles in chrome, or
> something like normal vs incognito mode that contains AwContents. From this,
it
> looks like AwBrowserContext can directly create and own this in the
constructor.
> (If it's a DEPS/layering violation, you can pass it in from
> AwMainDelegate->AwContentBrowserClient->AwBrowerContext)

I just need some place in native/ to construct this. It is owned by
AwBrowserContext, and will only be created once. Is there a better place?

[boliu, 2013-01-03 08:48:35]

https://chromiumcodereview.appspot.com/11763002/diff/4001/android_webview/nat...
File android_webview/native/aw_contents.cc (right):

https://chromiumcodereview.appspot.com/11763002/diff/4001/android_webview/nat...
android_webview/native/aw_contents.cc:179:
aw_browser_context->set_geolocation_permissions_context(
On 2013/01/03 08:37:58, kristianm1 wrote:
> On 2013/01/03 08:14:38, boliu wrote:
> > BrowserContext outlives AwContents, it's equivalent to profiles in chrome,
or
> > something like normal vs incognito mode that contains AwContents. From this,
> it
> > looks like AwBrowserContext can directly create and own this in the
> constructor.
> > (If it's a DEPS/layering violation, you can pass it in from
> > AwMainDelegate->AwContentBrowserClient->AwBrowerContext)
> 
> I just need some place in native/ to construct this. It is owned by
> AwBrowserContext, and will only be created once. Is there a better place?

AwMainDelegate is in lib/ which can include native/. You can following the
existing AwWebContentsViewDelegate::Create example in 
AwMainDelegate::CreateContentBrowserClient to pass in a geo "factory function".
We might just consolidate to an actual factory interface after we get a bunch of
these.

[Kristian Monsen, 2013-01-04 06:14:21]

https://codereview.chromium.org/11763002/diff/4001/android_webview/native/aw_...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/11763002/diff/4001/android_webview/native/aw_...
android_webview/native/aw_contents.cc:179:
aw_browser_context->set_geolocation_permissions_context(
On 2013/01/03 08:48:35, boliu wrote:
> On 2013/01/03 08:37:58, kristianm1 wrote:
> > On 2013/01/03 08:14:38, boliu wrote:
> > > BrowserContext outlives AwContents, it's equivalent to profiles in chrome,
> or
> > > something like normal vs incognito mode that contains AwContents. From
this,
> > it
> > > looks like AwBrowserContext can directly create and own this in the
> > constructor.
> > > (If it's a DEPS/layering violation, you can pass it in from
> > > AwMainDelegate->AwContentBrowserClient->AwBrowerContext)
> > 
> > I just need some place in native/ to construct this. It is owned by
> > AwBrowserContext, and will only be created once. Is there a better place?
> 
> AwMainDelegate is in lib/ which can include native/. You can following the
> existing AwWebContentsViewDelegate::Create example in 
> AwMainDelegate::CreateContentBrowserClient to pass in a geo "factory
function".
> We might just consolidate to an actual factory interface after we get a bunch
of
> these.

Passed it in the ctor to AwContentBrowserClient.

[Kristian Monsen, 2013-01-04 08:27:05]

I have tested this today, and it sends the prompt to java as expected by the cts
test. I think it would be good to submit this. It will be a quite large patch
after plumbing in settings, stored permissions and callbacks back to chromium.

[benm (inactive), 2013-01-04 11:35:52]

https://codereview.chromium.org/11763002/diff/9001/android_webview/browser/aw...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/11763002/diff/9001/android_webview/browser/aw...
android_webview/browser/aw_content_browser_client.cc:48:
content::GeolocationPermissionContext* geolocation_permission_context)
Why does this need to come in as a param here? Couldn't we just new one up
inside AwBrowserContext the first time the getter is called?

[boliu, 2013-01-04 16:42:05]

https://codereview.chromium.org/11763002/diff/9001/android_webview/browser/aw...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/11763002/diff/9001/android_webview/browser/aw...
android_webview/browser/aw_content_browser_client.cc:48:
content::GeolocationPermissionContext* geolocation_permission_context)
On 2013/01/04 11:35:52, benm wrote:
> Why does this need to come in as a param here? Couldn't we just new one up
> inside AwBrowserContext the first time the getter is called?

That would lead to a DEPS violation since AwBrowserContext is in browser/ and
cannot include AwGeolocationPermisssionContext which is in native/.

Although I still vote for passing in a "FactoryFn" so it can be created lazily.
We can refactor into a true factory interface when we collect enough of these.

[Kristian Monsen, 2013-01-04 17:13:46]

https://codereview.chromium.org/11763002/diff/9001/android_webview/browser/aw...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/11763002/diff/9001/android_webview/browser/aw...
android_webview/browser/aw_content_browser_client.cc:48:
content::GeolocationPermissionContext* geolocation_permission_context)
On 2013/01/04 16:42:05, boliu wrote:
> On 2013/01/04 11:35:52, benm wrote:
> > Why does this need to come in as a param here? Couldn't we just new one up
> > inside AwBrowserContext the first time the getter is called?
> 
> That would lead to a DEPS violation since AwBrowserContext is in browser/ and
> cannot include AwGeolocationPermisssionContext which is in native/.
> 
> Although I still vote for passing in a "FactoryFn" so it can be created
lazily.
> We can refactor into a true factory interface when we collect enough of these.

Right now lazy instantiation would not get anything except increased code
complexity as the factory would have to be created at the same time.

Unless you have something else that you want to use it for right now I think it
is best to wait until it is needed. I don't know which other kinds of objects
the factory should create.

[boliu, 2013-01-04 17:17:30]

https://codereview.chromium.org/11763002/diff/9001/android_webview/browser/aw...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/11763002/diff/9001/android_webview/browser/aw...
android_webview/browser/aw_content_browser_client.cc:48:
content::GeolocationPermissionContext* geolocation_permission_context)
On 2013/01/04 17:13:46, Kristian Monsen wrote:
> On 2013/01/04 16:42:05, boliu wrote:
> > On 2013/01/04 11:35:52, benm wrote:
> > > Why does this need to come in as a param here? Couldn't we just new one up
> > > inside AwBrowserContext the first time the getter is called?
> > 
> > That would lead to a DEPS violation since AwBrowserContext is in browser/
and
> > cannot include AwGeolocationPermisssionContext which is in native/.
> > 
> > Although I still vote for passing in a "FactoryFn" so it can be created
> lazily.
> > We can refactor into a true factory interface when we collect enough of
these.
> 
> Right now lazy instantiation would not get anything except increased code
> complexity as the factory would have to be created at the same time.
> 
> Unless you have something else that you want to use it for right now I think
it
> is best to wait until it is needed. I don't know which other kinds of objects
> the factory should create.

I was suggesting FactoryFn (ie function pointer) in this CL, basically just copy
ViewDelegateFactoryFn. Refactoring to a real factory class comes later.

[Kristian Monsen, 2013-01-04 22:54:31]

Updated, PTAL.

One question, can I depend on AwBrowserContext only being used from one thread,
and that is the UI thread?

https://chromiumcodereview.appspot.com/11763002/diff/9001/android_webview/bro...
File android_webview/browser/aw_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/11763002/diff/9001/android_webview/bro...
android_webview/browser/aw_content_browser_client.cc:48:
content::GeolocationPermissionContext* geolocation_permission_context)
On 2013/01/04 17:17:30, boliu wrote:
> On 2013/01/04 17:13:46, Kristian Monsen wrote:
> > On 2013/01/04 16:42:05, boliu wrote:
> > > On 2013/01/04 11:35:52, benm wrote:
> > > > Why does this need to come in as a param here? Couldn't we just new one
up
> > > > inside AwBrowserContext the first time the getter is called?
> > > 
> > > That would lead to a DEPS violation since AwBrowserContext is in browser/
> and
> > > cannot include AwGeolocationPermisssionContext which is in native/.
> > > 
> > > Although I still vote for passing in a "FactoryFn" so it can be created
> > lazily.
> > > We can refactor into a true factory interface when we collect enough of
> these.
> > 
> > Right now lazy instantiation would not get anything except increased code
> > complexity as the factory would have to be created at the same time.
> > 
> > Unless you have something else that you want to use it for right now I think
> it
> > is best to wait until it is needed. I don't know which other kinds of
objects
> > the factory should create.
> 
> I was suggesting FactoryFn (ie function pointer) in this CL, basically just
copy
> ViewDelegateFactoryFn. Refactoring to a real factory class comes later.

Updated to do this, take a look and see if this was what you were thinking.

[boliu_use_chromium_pls, 2013-01-04 23:45:43]

On 2013/01/04 22:54:31, Kristian Monsen wrote:
> Updated, PTAL.
> 
> One question, can I depend on AwBrowserContext only being used from one
thread,
> and that is the UI thread?
> 

Per comment at top of content::BrowserContext, yes.

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/br...
File android_webview/browser/aw_browser_context.cc (right):

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/br...
android_webview/browser/aw_browser_context.cc:16:
geolocation_permission_context_(NULL) {
nit: scoped_refptr defaults constructs to NULL, so no need here

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/br...
android_webview/browser/aw_browser_context.cc:82: if
(geolocation_permission_context_ == NULL) {
nit: I believe you can just do if (!geolocation_permission_context_), but ask
compiler first :)

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/br...
File android_webview/browser/aw_browser_context.h (right):

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/br...
android_webview/browser/aw_browser_context.h:18: typedef
content::GeolocationPermissionContext* GeolocationPermissionGetterFn();
Name it FactoryFn, since the function is also passing ownership of the returned
object.

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/ja...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/ja...
android_webview/java/src/org/chromium/android_webview/AwContents.java:861: new
ChromiumGeolocationCallback(renderProcessId, renderViewId, bridgeId,
The constructor is private, so does this compile? (or maybe I'm learning new
things about java...)

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/na...
File android_webview/native/aw_contents.cc (right):

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/na...
android_webview/native/aw_contents.cc:9: #include
"android_webview/browser/aw_browser_context.h"
all the new includes in this file looks not needed anymore

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/na...
android_webview/native/aw_contents.cc:665: void
AwContents::GeolocationHidePrompt() {
TODO implement?

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/na...
File android_webview/native/aw_contents.h (right):

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/na...
android_webview/native/aw_contents.h:12: #include
"android_webview/native/aw_geolocation_permission_context.h"
doesn't appear to be used

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/na...
android_webview/native/aw_contents.h:108: int bridge_id, const GURL&
requesting_frame);
nit: I think in chromium c++ style guide
(http://dev.chromium.org/developers/coding-style), you have all arguments in one
line or each on different lines (and lined up)

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/na...
File android_webview/native/aw_geolocation_permission_context.cc (right):

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/na...
android_webview/native/aw_geolocation_permission_context.cc:26: if
(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
I know the chrome implementation has this check, but this is only ever called on
from the IO thread, so can we break this into RequestGeolocationPermission which
PostTask to a private RequestGeolocationPermissionOnIOThread, which does the
work.

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/na...
android_webview/native/aw_geolocation_permission_context.cc:60: if
(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
ditto

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/na...
File android_webview/native/aw_geolocation_permission_context.h (right):

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/na...
android_webview/native/aw_geolocation_permission_context.h:30: //
content::GeolocationPermissionContext implementation
nit: usually one of this comment is enough, and group the overriden methods
together without space between them, but since there are only two methods,
doesn't matter that much

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/na...
android_webview/native/aw_geolocation_permission_context.h:43: );
pretty sure the end parenthesis should be on end of previous line.

https://chromiumcodereview.appspot.com/11763002/diff/14001/android_webview/na...
android_webview/native/aw_geolocation_permission_context.h:46: }  // anemspace
android_webview
typo

[Kristian Monsen, 2013-01-05 02:44:58]

Thanks for the review, I think all should be fixed.

https://codereview.chromium.org/11763002/diff/14001/android_webview/browser/a...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/11763002/diff/14001/android_webview/browser/a...
android_webview/browser/aw_browser_context.cc:16:
geolocation_permission_context_(NULL) {
On 2013/01/04 23:45:43, Bo Liu wrote:
> nit: scoped_refptr defaults constructs to NULL, so no need here

Done.

https://codereview.chromium.org/11763002/diff/14001/android_webview/browser/a...
android_webview/browser/aw_browser_context.cc:82: if
(geolocation_permission_context_ == NULL) {
On 2013/01/04 23:45:43, Bo Liu wrote:
> nit: I believe you can just do if (!geolocation_permission_context_), but ask
> compiler first :)

Compiler agrees.

https://codereview.chromium.org/11763002/diff/14001/android_webview/browser/a...
File android_webview/browser/aw_browser_context.h (right):

https://codereview.chromium.org/11763002/diff/14001/android_webview/browser/a...
android_webview/browser/aw_browser_context.h:18: typedef
content::GeolocationPermissionContext* GeolocationPermissionGetterFn();
On 2013/01/04 23:45:43, Bo Liu wrote:
> Name it FactoryFn, since the function is also passing ownership of the
returned
> object.

Done, also added _fn to the end of the variable names so it is clear it is a
function and not an object.

https://codereview.chromium.org/11763002/diff/14001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/11763002/diff/14001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:861: new
ChromiumGeolocationCallback(renderProcessId, renderViewId, bridgeId,
On 2013/01/04 23:45:43, Bo Liu wrote:
> The constructor is private, so does this compile? (or maybe I'm learning new
> things about java...)

Outer classes can use private members of inner classes. It is one of the
"features" of Inner classes:
http://docs.oracle.com/javase/tutorial/java/javaOO/nested.html

Happy to change if it isn't readable. Note that the inner class is private, so
the ctor will be private for all practical purposes anyway.

https://codereview.chromium.org/11763002/diff/14001/android_webview/native/aw...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/11763002/diff/14001/android_webview/native/aw...
android_webview/native/aw_contents.cc:9: #include
"android_webview/browser/aw_browser_context.h"
On 2013/01/04 23:45:43, Bo Liu wrote:
> all the new includes in this file looks not needed anymore

Done.

https://codereview.chromium.org/11763002/diff/14001/android_webview/native/aw...
android_webview/native/aw_contents.cc:665: void
AwContents::GeolocationHidePrompt() {
On 2013/01/04 23:45:43, Bo Liu wrote:
> TODO implement?

Done.

https://codereview.chromium.org/11763002/diff/14001/android_webview/native/aw...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/11763002/diff/14001/android_webview/native/aw...
android_webview/native/aw_contents.h:12: #include
"android_webview/native/aw_geolocation_permission_context.h"
On 2013/01/04 23:45:43, Bo Liu wrote:
> doesn't appear to be used

Done.

https://codereview.chromium.org/11763002/diff/14001/android_webview/native/aw...
android_webview/native/aw_contents.h:108: int bridge_id, const GURL&
requesting_frame);
On 2013/01/04 23:45:43, Bo Liu wrote:
> nit: I think in chromium c++ style guide
> (http://dev.chromium.org/developers/coding-style), you have all arguments in
one
> line or each on different lines (and lined up)

You are right, done. Almost every other function around there breaks that style
rule.

https://codereview.chromium.org/11763002/diff/14001/android_webview/native/aw...
File android_webview/native/aw_geolocation_permission_context.cc (right):

https://codereview.chromium.org/11763002/diff/14001/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.cc:26: if
(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
On 2013/01/04 23:45:43, Bo Liu wrote:
> I know the chrome implementation has this check, but this is only ever called
on
> from the IO thread, so can we break this into RequestGeolocationPermission
which
> PostTask to a private RequestGeolocationPermissionOnIOThread, which does the
> work.

Done.

https://codereview.chromium.org/11763002/diff/14001/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.cc:60: if
(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
On 2013/01/04 23:45:43, Bo Liu wrote:
> ditto

Done.

https://codereview.chromium.org/11763002/diff/14001/android_webview/native/aw...
File android_webview/native/aw_geolocation_permission_context.h (right):

https://codereview.chromium.org/11763002/diff/14001/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.h:30: //
content::GeolocationPermissionContext implementation
On 2013/01/04 23:45:43, Bo Liu wrote:
> nit: usually one of this comment is enough, and group the overriden methods
> together without space between them, but since there are only two methods,
> doesn't matter that much

Done.

https://codereview.chromium.org/11763002/diff/14001/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.h:43: );
On 2013/01/04 23:45:43, Bo Liu wrote:
> pretty sure the end parenthesis should be on end of previous line.

Done.

[boliu, 2013-01-05 04:37:35]

lgtm % tiny things

but also need to wait for one from owner

https://codereview.chromium.org/11763002/diff/14001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/11763002/diff/14001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:861: new
ChromiumGeolocationCallback(renderProcessId, renderViewId, bridgeId,
On 2013/01/05 02:44:58, Kristian Monsen wrote:
> On 2013/01/04 23:45:43, Bo Liu wrote:
> > The constructor is private, so does this compile? (or maybe I'm learning new
> > things about java...)
> 
> Outer classes can use private members of inner classes. It is one of the
> "features" of Inner classes:
> http://docs.oracle.com/javase/tutorial/java/javaOO/nested.html
> 
> Happy to change if it isn't readable. Note that the inner class is private, so
> the ctor will be private for all practical purposes anyway.

In that case, keep it private then. Learning new things about java...

https://codereview.chromium.org/11763002/diff/14001/android_webview/native/aw...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/11763002/diff/14001/android_webview/native/aw...
android_webview/native/aw_contents.h:108: int bridge_id, const GURL&
requesting_frame);
On 2013/01/05 02:44:58, Kristian Monsen wrote:
> On 2013/01/04 23:45:43, Bo Liu wrote:
> > nit: I think in chromium c++ style guide
> > (http://dev.chromium.org/developers/coding-style), you have all arguments in
> one
> > line or each on different lines (and lined up)
> 
> You are right, done. Almost every other function around there breaks that
style
> rule.

True...attribute it to differences between java and c++ style guide

https://codereview.chromium.org/11763002/diff/22001/android_webview/native/aw...
File android_webview/native/aw_geolocation_permission_context.cc (right):

https://codereview.chromium.org/11763002/diff/22001/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.cc:27: const
content::RenderViewHost* host =
DCHECK for ui thread

https://codereview.chromium.org/11763002/diff/22001/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.cc:90: ));
this go on end of previous line

https://codereview.chromium.org/11763002/diff/22001/android_webview/native/aw...
File android_webview/native/aw_geolocation_permission_context.h (right):

https://codereview.chromium.org/11763002/diff/22001/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.h:43: virtual void
RequestGeolocationPermissionOnUIThread(
These don't need to be virtual

[Kristian Monsen, 2013-01-05 04:54:40]

Thank you! Waiting for an owner.

https://codereview.chromium.org/11763002/diff/22001/android_webview/native/aw...
File android_webview/native/aw_geolocation_permission_context.cc (right):

https://codereview.chromium.org/11763002/diff/22001/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.cc:27: const
content::RenderViewHost* host =
On 2013/01/05 04:37:35, boliu wrote:
> DCHECK for ui thread

Done. Added to the other one as well.

https://codereview.chromium.org/11763002/diff/22001/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.cc:90: ));
On 2013/01/05 04:37:35, boliu wrote:
> this go on end of previous line

Done.

https://codereview.chromium.org/11763002/diff/22001/android_webview/native/aw...
File android_webview/native/aw_geolocation_permission_context.h (right):

https://codereview.chromium.org/11763002/diff/22001/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.h:43: virtual void
RequestGeolocationPermissionOnUIThread(
On 2013/01/05 04:37:35, boliu wrote:
> These don't need to be virtual

Done.

[benm (inactive), 2013-01-07 10:45:01]

lgtm, just nits...

https://codereview.chromium.org/11763002/diff/19002/android_webview/native/aw...
File android_webview/native/aw_geolocation_permission_context.cc (right):

https://codereview.chromium.org/11763002/diff/19002/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.cc:30:
content::WebContents* webContents =
nit: web_contents

https://codereview.chromium.org/11763002/diff/19002/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.cc:32: AwContents*
awContents = AwContents::FromWebContents(webContents);
nit: aw_contents

https://codereview.chromium.org/11763002/diff/19002/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.cc:33:
awContents->GeolocationShowPrompt(
would ShowGeolocationPrompt be more readable?

https://codereview.chromium.org/11763002/diff/19002/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.cc:99: bool value) {
nit: TODO?

[joth, 2013-01-07 18:17:39]

OWNERS lgtm mod-nits.

https://codereview.chromium.org/11763002/diff/15012/android_webview/browser/a...
File android_webview/browser/aw_content_browser_client.h (right):

https://codereview.chromium.org/11763002/diff/15012/android_webview/browser/a...
android_webview/browser/aw_content_browser_client.h:22:
GeolocationPermissionFactoryFn* geolocation_permission_factory_fn);
nit: either use _fn suffix on both or neither param names (and associated
members)

[Kristian Monsen, 2013-01-07 18:41:09]

https://codereview.chromium.org/11763002/diff/19002/android_webview/native/aw...
File android_webview/native/aw_geolocation_permission_context.cc (right):

https://codereview.chromium.org/11763002/diff/19002/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.cc:30:
content::WebContents* webContents =
On 2013/01/07 10:45:02, benm wrote:
> nit: web_contents

Done.

https://codereview.chromium.org/11763002/diff/19002/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.cc:32: AwContents*
awContents = AwContents::FromWebContents(webContents);
On 2013/01/07 10:45:02, benm wrote:
> nit: aw_contents

Done.

https://codereview.chromium.org/11763002/diff/19002/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.cc:33:
awContents->GeolocationShowPrompt(
On 2013/01/07 10:45:02, benm wrote:
> would ShowGeolocationPrompt be more readable?

Public API is onGeolocationPermissionsShowPrompt:
http://developer.android.com/reference/android/webkit/WebChromeClient.html#on...,
android.webkit.GeolocationPermissions.Callback)

I wanted it to be as close as possible, but noticed that the On prefix was
frequently dropped in the chromium layer.

https://codereview.chromium.org/11763002/diff/15012/android_webview/browser/a...
File android_webview/browser/aw_content_browser_client.h (right):

https://codereview.chromium.org/11763002/diff/15012/android_webview/browser/a...
android_webview/browser/aw_content_browser_client.h:22:
GeolocationPermissionFactoryFn* geolocation_permission_factory_fn);
On 2013/01/07 18:17:39, joth wrote:
> nit: either use _fn suffix on both or neither param names (and associated
> members)

OK, I will remove the new ones.

[joth, 2013-01-07 20:25:15]

https://codereview.chromium.org/11763002/diff/19002/android_webview/native/aw...
File android_webview/native/aw_geolocation_permission_context.cc (right):

https://codereview.chromium.org/11763002/diff/19002/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.cc:33:
awContents->GeolocationShowPrompt(
On 2013/01/07 18:41:09, Kristian Monsen wrote:
> On 2013/01/07 10:45:02, benm wrote:
> > would ShowGeolocationPrompt be more readable?
> 
> Public API is onGeolocationPermissionsShowPrompt:
>
http://developer.android.com/reference/android/webkit/WebChromeClient.html#on...,
> android.webkit.GeolocationPermissions.Callback)
> 
> I wanted it to be as close as possible, but noticed that the On prefix was
> frequently dropped in the chromium layer.

Yep - onFoo in chromium is normally kept for Foo IPC message handlers.

But in AwContents we already have onReceivedHttpAuthRequest and
OnFindResultReceived and as that class is all about JNI bindings I think
reflecting the (invariant) java names is OK in places where we have a direct
mapping, like this.

(If we did't go that way, we should go more towards the chrome style and put the
verb first as per Ben comment.)

[Kristian Monsen, 2013-01-08 08:03:10]

https://codereview.chromium.org/11763002/diff/19002/android_webview/native/aw...
File android_webview/native/aw_geolocation_permission_context.cc (right):

https://codereview.chromium.org/11763002/diff/19002/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.cc:33:
awContents->GeolocationShowPrompt(
On 2013/01/07 20:25:15, joth wrote:
> On 2013/01/07 18:41:09, Kristian Monsen wrote:
> > On 2013/01/07 10:45:02, benm wrote:
> > > would ShowGeolocationPrompt be more readable?
> > 
> > Public API is onGeolocationPermissionsShowPrompt:
> >
>
http://developer.android.com/reference/android/webkit/WebChromeClient.html#on...,
> > android.webkit.GeolocationPermissions.Callback)
> > 
> > I wanted it to be as close as possible, but noticed that the On prefix was
> > frequently dropped in the chromium layer.
> 
> Yep - onFoo in chromium is normally kept for Foo IPC message handlers.
> 
> But in AwContents we already have onReceivedHttpAuthRequest and
> OnFindResultReceived and as that class is all about JNI bindings I think
> reflecting the (invariant) java names is OK in places where we have a direct
> mapping, like this.
> 
> (If we did't go that way, we should go more towards the chrome style and put
the
> verb first as per Ben comment.)
> 

Added On prefix

https://codereview.chromium.org/11763002/diff/19002/android_webview/native/aw...
android_webview/native/aw_geolocation_permission_context.cc:99: bool value) {
On 2013/01/07 10:45:02, benm wrote:
> nit: TODO?

Done.

[commit-bot: I haz the power, 2013-01-10 01:35:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kristianm@chromium.org/11763002/40001

[commit-bot: I haz the power, 2013-01-10 05:51:47]

Change committed as 176032