<webview>: Move Geolocation permission to chrome layer.

<webview>: Move Geolocation permission to chrome layer.

This CL also exposes the userGesture field to the geolocation permissionrequest event.

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

[lazyboy, 2014-04-11 19:27:05]

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

https://codereview.chromium.org/235633002/diff/20001/chrome/browser/geolocati...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:162:
WebViewGuest* guest = WebViewGuest::FromWebContents(web_contents);
The guest specialization code lying here seems messy. Can we still keep creating
guest specific geo_permission_context? That seems like a natural way to handle
geo permission.
I know it is not straightfwd anymore cause you're moving this under chrome/

[Fady Samuel, 2014-04-11 19:47:52]

On 2014/04/11 19:27:05, lazyboy wrote:
>
https://codereview.chromium.org/235633002/diff/20001/chrome/browser/geolocati...
> File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
> (right):
> 
>
https://codereview.chromium.org/235633002/diff/20001/chrome/browser/geolocati...
> chrome/browser/geolocation/chrome_geolocation_permission_context.cc:162:
> WebViewGuest* guest = WebViewGuest::FromWebContents(web_contents);
> The guest specialization code lying here seems messy. Can we still keep
creating
> guest specific geo_permission_context? That seems like a natural way to handle
> geo permission.
> I know it is not straightfwd anymore cause you're moving this under chrome/

No, this doesn't work for <appview> in the near future. A single process can
host standalone app windows and appview guests. A process cannot be marked as a
"guest process" anymore.

[Fady Samuel, 2014-04-11 21:20:18]

PTAL

[lazyboy, 2014-04-11 21:47:21]

https://codereview.chromium.org/235633002/diff/40001/chrome/browser/guestview...
File chrome/browser/guestview/guestview_constants.h (right):

https://codereview.chromium.org/235633002/diff/40001/chrome/browser/guestview...
chrome/browser/guestview/guestview_constants.h:13: extern const char
kUserGesture[];
nit: sort within group.

https://codereview.chromium.org/235633002/diff/40001/chrome/browser/guestview...
File chrome/browser/guestview/webview/webview_guest.cc (right):

https://codereview.chromium.org/235633002/diff/40001/chrome/browser/guestview...
chrome/browser/guestview/webview/webview_guest.cc:574: base::Unretained(this),
Same question like the other CL, what if WebViewGuest goes away before the bound
method gets called.

https://codereview.chromium.org/235633002/diff/40001/chrome/browser/guestview...
chrome/browser/guestview/webview/webview_guest.cc:910: 
remove empty line

https://codereview.chromium.org/235633002/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/web_view/geolocation/cancel_request/embedder.js
(left):

https://codereview.chromium.org/235633002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/geolocation/cancel_request/embedder.js:74:

Why do you remove these? Do they start getting defined in |e|?

https://codereview.chromium.org/235633002/diff/40001/content/public/browser/b...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://codereview.chromium.org/235633002/diff/40001/content/public/browser/b...
content/public/browser/browser_plugin_guest_delegate.h:90: // Returns whether
the delegate has, or will handle the permission request.
Returns ... need update.

[Fady Samuel, 2014-04-11 21:57:15]

PTAL

https://codereview.chromium.org/235633002/diff/40001/chrome/browser/guestview...
File chrome/browser/guestview/webview/webview_guest.cc (right):

https://codereview.chromium.org/235633002/diff/40001/chrome/browser/guestview...
chrome/browser/guestview/webview/webview_guest.cc:574: base::Unretained(this),
On 2014/04/11 21:47:21, lazyboy wrote:
> Same question like the other CL, what if WebViewGuest goes away before the
bound
> method gets called.

WebViewGuest::SetPermission calls the bound method so this is not possible.

https://codereview.chromium.org/235633002/diff/40001/chrome/browser/guestview...
chrome/browser/guestview/webview/webview_guest.cc:910: 
On 2014/04/11 21:47:21, lazyboy wrote:
> remove empty line

Done.

https://codereview.chromium.org/235633002/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/web_view/geolocation/cancel_request/embedder.js
(left):

https://codereview.chromium.org/235633002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/geolocation/cancel_request/embedder.js:74:

On 2014/04/11 21:47:21, lazyboy wrote:
> Why do you remove these? Do they start getting defined in |e|?

Geolocation recently got a "userGesture". I now plumb that through. See here:
https://codereview.chromium.org/185813004/

https://codereview.chromium.org/235633002/diff/40001/content/public/browser/b...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://codereview.chromium.org/235633002/diff/40001/content/public/browser/b...
content/public/browser/browser_plugin_guest_delegate.h:90: // Returns whether
the delegate has, or will handle the permission request.
On 2014/04/11 21:47:21, lazyboy wrote:
> Returns ... need update.

Done.

[Fady Samuel, 2014-04-11 21:57:16]

PTAL

https://codereview.chromium.org/235633002/diff/40001/chrome/browser/guestview...
File chrome/browser/guestview/webview/webview_guest.cc (right):

https://codereview.chromium.org/235633002/diff/40001/chrome/browser/guestview...
chrome/browser/guestview/webview/webview_guest.cc:574: base::Unretained(this),
On 2014/04/11 21:47:21, lazyboy wrote:
> Same question like the other CL, what if WebViewGuest goes away before the
bound
> method gets called.

WebViewGuest::SetPermission calls the bound method so this is not possible.

https://codereview.chromium.org/235633002/diff/40001/chrome/browser/guestview...
chrome/browser/guestview/webview/webview_guest.cc:910: 
On 2014/04/11 21:47:21, lazyboy wrote:
> remove empty line

Done.

https://codereview.chromium.org/235633002/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/web_view/geolocation/cancel_request/embedder.js
(left):

https://codereview.chromium.org/235633002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/geolocation/cancel_request/embedder.js:74:

On 2014/04/11 21:47:21, lazyboy wrote:
> Why do you remove these? Do they start getting defined in |e|?

Geolocation recently got a "userGesture". I now plumb that through. See here:
https://codereview.chromium.org/185813004/

https://codereview.chromium.org/235633002/diff/40001/content/public/browser/b...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://codereview.chromium.org/235633002/diff/40001/content/public/browser/b...
content/public/browser/browser_plugin_guest_delegate.h:90: // Returns whether
the delegate has, or will handle the permission request.
On 2014/04/11 21:47:21, lazyboy wrote:
> Returns ... need update.

Done.

[lazyboy, 2014-04-11 22:00:53]

lgtm

https://codereview.chromium.org/235633002/diff/40001/chrome/browser/guestview...
File chrome/browser/guestview/webview/webview_guest.cc (right):

https://codereview.chromium.org/235633002/diff/40001/chrome/browser/guestview...
chrome/browser/guestview/webview/webview_guest.cc:558: 
nit: remove empty line.

https://codereview.chromium.org/235633002/diff/40001/chrome/browser/guestview...
chrome/browser/guestview/webview/webview_guest.cc:574: base::Unretained(this),
On 2014/04/11 21:57:16, Fady Samuel wrote:
> On 2014/04/11 21:47:21, lazyboy wrote:
> > Same question like the other CL, what if WebViewGuest goes away before the
> bound
> > method gets called.
> 
> WebViewGuest::SetPermission calls the bound method so this is not possible.
Cool, can we add a quick comment here then.

https://codereview.chromium.org/235633002/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/web_view/geolocation/cancel_request/embedder.js
(left):

https://codereview.chromium.org/235633002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/geolocation/cancel_request/embedder.js:74:

On 2014/04/11 21:57:16, Fady Samuel wrote:
> On 2014/04/11 21:47:21, lazyboy wrote:
> > Why do you remove these? Do they start getting defined in |e|?
> 
> Geolocation recently got a "userGesture". I now plumb that through. See here:
> https://codereview.chromium.org/185813004/

Please add this to the CL description.

[Fady Samuel, 2014-04-14 17:30:02]

+jam@ for content/public changes.

https://codereview.chromium.org/235633002/diff/40001/chrome/browser/guestview...
File chrome/browser/guestview/webview/webview_guest.cc (right):

https://codereview.chromium.org/235633002/diff/40001/chrome/browser/guestview...
chrome/browser/guestview/webview/webview_guest.cc:574: base::Unretained(this),
On 2014/04/11 22:00:53, lazyboy wrote:
> On 2014/04/11 21:57:16, Fady Samuel wrote:
> > On 2014/04/11 21:47:21, lazyboy wrote:
> > > Same question like the other CL, what if WebViewGuest goes away before the
> > bound
> > > method gets called.
> > 
> > WebViewGuest::SetPermission calls the bound method so this is not possible.
> Cool, can we add a quick comment here then.

Done.

https://codereview.chromium.org/235633002/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/web_view/geolocation/cancel_request/embedder.js
(left):

https://codereview.chromium.org/235633002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/geolocation/cancel_request/embedder.js:74:

On 2014/04/11 22:00:53, lazyboy wrote:
> On 2014/04/11 21:57:16, Fady Samuel wrote:
> > On 2014/04/11 21:47:21, lazyboy wrote:
> > > Why do you remove these? Do they start getting defined in |e|?
> > 
> > Geolocation recently got a "userGesture". I now plumb that through. See
here:
> > https://codereview.chromium.org/185813004/
> 
> Please add this to the CL description.

Done.

[jam, 2014-04-14 18:11:54]

https://codereview.chromium.org/235633002/diff/80001/content/public/browser/b...
File content/public/browser/browser_plugin_guest_delegate.cc (right):

https://codereview.chromium.org/235633002/diff/80001/content/public/browser/b...
content/public/browser/browser_plugin_guest_delegate.cc:35: std::string()));
why not just an empty method? in general we try to keep all the delegate methods
empty in content/public

https://codereview.chromium.org/235633002/diff/80001/content/public/browser/b...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://codereview.chromium.org/235633002/diff/80001/content/public/browser/b...
content/public/browser/browser_plugin_guest_delegate.h:90: // Returns an unique
identifier for the request.
this return value isn't used in content (which i also agree with), so take it
out of here.

[Fady Samuel, 2014-04-14 18:51:13]

PTAL

https://codereview.chromium.org/235633002/diff/80001/content/public/browser/b...
File content/public/browser/browser_plugin_guest_delegate.cc (right):

https://codereview.chromium.org/235633002/diff/80001/content/public/browser/b...
content/public/browser/browser_plugin_guest_delegate.cc:35: std::string()));
On 2014/04/14 18:11:55, jam wrote:
> why not just an empty method? in general we try to keep all the delegate
methods
> empty in content/public

I've made it empty. This code is temporary. We will shortly be getting rid of
this entirely once all permissions are moved into the chrome layer.

https://codereview.chromium.org/235633002/diff/80001/content/public/browser/b...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://codereview.chromium.org/235633002/diff/80001/content/public/browser/b...
content/public/browser/browser_plugin_guest_delegate.h:90: // Returns an unique
identifier for the request.
On 2014/04/14 18:11:55, jam wrote:
> this return value isn't used in content (which i also agree with), so take it
> out of here.

Done.

[jam, 2014-04-14 19:08:19]

lgtm

[Fady Samuel, 2014-04-14 19:08:47]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-04-14 19:10:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/235633002/120001

[commit-bot: I haz the power, 2014-04-14 19:46:52]

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

[commit-bot: I haz the power, 2014-04-14 19:46:52]

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

[Fady Samuel, 2014-04-14 20:24:27]

+asvitkine@ for actions.xml

[Alexei Svitkine (slow), 2014-04-14 20:36:30]

https://codereview.chromium.org/235633002/diff/140001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (left):

https://codereview.chromium.org/235633002/diff/140001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:1746: <action
name="BrowserPlugin.PermissionDeny.Geolocation">
Instead of removing these, please mark them as obsolete.

[Fady Samuel, 2014-04-14 20:50:49]

PTAL Alexei

[Alexei Svitkine (slow), 2014-04-14 21:11:53]

LGTM, thanks

[Fady Samuel, 2014-04-14 21:12:24]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-04-14 21:14:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/235633002/160001

[commit-bot: I haz the power, 2014-04-14 22:00:03]

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

[commit-bot: I haz the power, 2014-04-14 22:00:03]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg
  tryserver.chromium on linux_chromium_chromeos_rel

[Fady Samuel, 2014-04-14 22:17:11]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2014-04-14 22:17:11]

The CQ bit was unchecked by fsamuel@chromium.org

[Fady Samuel, 2014-04-14 22:17:11]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-04-14 22:18:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/235633002/180001

[commit-bot: I haz the power, 2014-04-14 23:22:57]

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

[commit-bot: I haz the power, 2014-04-14 23:22:58]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[Fady Samuel, 2014-04-15 15:39:45]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-04-15 15:40:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/235633002/190001

[commit-bot: I haz the power, 2014-04-15 18:19:55]

Change committed as 263926