Introduce WebViewPermissionHelperDelegate.

Introduce WebViewPermissionHelperDelegate in extensions and ChromeWebViewPermissionHelperDelegate in chrome\browser.

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

[Fady Samuel, 2014-08-13 20:07:26]

Some initial comments.

https://codereview.chromium.org/468323002/diff/40001/chrome/browser/guest_vie...
File
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h
(right):

https://codereview.chromium.org/468323002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h:19:
extensions::WebViewPermissionHelper& web_view_permission_helper);
Pass by pointer (not reference) if we expect to change this (we do).

https://codereview.chromium.org/468323002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h:35:

nit: Remove line.

https://codereview.chromium.org/468323002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h:42:

nit: Remove line.

https://codereview.chromium.org/468323002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h:47:

nit: Remove line.

https://codereview.chromium.org/468323002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h:54:

nit: Remove line.

https://codereview.chromium.org/468323002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h:89:
#endif  // defiend(ENABLE_PLUGINS)
s/defiend/defined

https://codereview.chromium.org/468323002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h:136:
extensions::WebViewPermissionHelper& web_view_permission_helper_;
Make this a pointer.

https://codereview.chromium.org/468323002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h:140:
extensions::WebViewGuest* web_view_guest() {
Move this up. All member variables should be at the bottom.

https://codereview.chromium.org/468323002/diff/40001/extensions/browser/guest...
File
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h
(right):

https://codereview.chromium.org/468323002/diff/40001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h:8:
#include "base/memory/weak_ptr.h"
Is this necessary?

https://codereview.chromium.org/468323002/diff/40001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h:23:
const content::MediaResponseCallback& callback) {}
I think the usual convention is for interfaces, there's an empty line between
each method, and for implementations, there's no empty line. In this case, you
should have an empty line between each method.

[Xi Han, 2014-08-13 21:30:57]

https://codereview.chromium.org/468323002/diff/40001/chrome/browser/guest_vie...
File
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h
(right):

https://codereview.chromium.org/468323002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h:19:
extensions::WebViewPermissionHelper& web_view_permission_helper);
On 2014/08/13 20:07:25, Fady Samuel wrote:
> Pass by pointer (not reference) if we expect to change this (we do).

Done.

https://codereview.chromium.org/468323002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h:35:

On 2014/08/13 20:07:25, Fady Samuel wrote:
> nit: Remove line.

Done.

https://codereview.chromium.org/468323002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h:42:

On 2014/08/13 20:07:25, Fady Samuel wrote:
> nit: Remove line.

Done.

https://codereview.chromium.org/468323002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h:47:

On 2014/08/13 20:07:25, Fady Samuel wrote:
> nit: Remove line.

Done.

https://codereview.chromium.org/468323002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h:54:

On 2014/08/13 20:07:25, Fady Samuel wrote:
> nit: Remove line.

Done.

https://codereview.chromium.org/468323002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h:89:
#endif  // defiend(ENABLE_PLUGINS)
On 2014/08/13 20:07:25, Fady Samuel wrote:
> s/defiend/defined

Done.

https://codereview.chromium.org/468323002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h:136:
extensions::WebViewPermissionHelper& web_view_permission_helper_;
On 2014/08/13 20:07:25, Fady Samuel wrote:
> Make this a pointer.

Done.

https://codereview.chromium.org/468323002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h:140:
extensions::WebViewGuest* web_view_guest() {
On 2014/08/13 20:07:25, Fady Samuel wrote:
> Move this up. All member variables should be at the bottom.

Done.

https://codereview.chromium.org/468323002/diff/40001/extensions/browser/guest...
File
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h
(right):

https://codereview.chromium.org/468323002/diff/40001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h:8:
#include "base/memory/weak_ptr.h"
On 2014/08/13 20:07:25, Fady Samuel wrote:
> Is this necessary?

Removed.

https://codereview.chromium.org/468323002/diff/40001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h:23:
const content::MediaResponseCallback& callback) {}
On 2014/08/13 20:07:25, Fady Samuel wrote:
> I think the usual convention is for interfaces, there's an empty line between
> each method, and for implementations, there's no empty line. In this case, you
> should have an empty line between each method.

Thank you for pointing out this.

[Fady Samuel, 2014-08-13 23:24:50]

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
File
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc
(right):

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:136:
OnMediaPermissionResponse,
nit: try to keep this as one line.

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:204:
::OnPointerLockPermissionResponse,
Try to keep this as one line.

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:233:
::OnGeolocationPermissionResponse,
nit: try to keep this as one line.

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:305:
::OnFileSystemPermissionResponse,
nit: try to keep this as one line.

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:329:
::FileSystemAccessedAsyncResponse,
nit: Try to keep this as one line.

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:359:
::FileSystemAccessedSyncResponse,
nit: Try to keep this as one line.

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
File
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h
(right):

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h:22:
virtual void RequestMediaAccessPermission(
Add the comment above:

WebViewPermissionHelperDelegate implementation.

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/web_view_permission_helper.cc (right):

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:321: void
WebViewPermissionHelper::ErasePendingPermissionRequest(int request_id) {
nit: CancelPendingPermissionRequest.

https://codereview.chromium.org/468323002/diff/60001/extensions/browser/guest...
File
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h
(right):

https://codereview.chromium.org/468323002/diff/60001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h:14:
class WebViewPermissionHelperDelegate : public content::WebContentsObserver {
Short comment describing what this is and what this does.

[Xi Han, 2014-08-14 13:59:55]

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
File
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc
(right):

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:136:
OnMediaPermissionResponse,
On 2014/08/13 23:24:50, Fady Samuel wrote:
> nit: try to keep this as one line.

Done.

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:204:
::OnPointerLockPermissionResponse,
On 2014/08/13 23:24:50, Fady Samuel wrote:
> Try to keep this as one line.


It doesn't fit in one line, but I change its format.

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:233:
::OnGeolocationPermissionResponse,
On 2014/08/13 23:24:50, Fady Samuel wrote:
> nit: try to keep this as one line.

It doesn't fit in one line, but I change its format.

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:305:
::OnFileSystemPermissionResponse,
On 2014/08/13 23:24:50, Fady Samuel wrote:
> nit: try to keep this as one line.

It doesn't fit in one line, but I change its format.

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:329:
::FileSystemAccessedAsyncResponse,
On 2014/08/13 23:24:50, Fady Samuel wrote:
> nit: Try to keep this as one line.

It doesn't fit in one line, but I change its format.

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:359:
::FileSystemAccessedSyncResponse,
On 2014/08/13 23:24:50, Fady Samuel wrote:
> nit: Try to keep this as one line.

It doesn't fit in one line, but I change its format.

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
File
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h
(right):

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h:22:
virtual void RequestMediaAccessPermission(
On 2014/08/13 23:24:50, Fady Samuel wrote:
> Add the comment above:
> 
> WebViewPermissionHelperDelegate implementation.

Done.

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/web_view_permission_helper.cc (right):

https://codereview.chromium.org/468323002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:321: void
WebViewPermissionHelper::ErasePendingPermissionRequest(int request_id) {
On 2014/08/13 23:24:50, Fady Samuel wrote:
> nit: CancelPendingPermissionRequest.

Done.

https://codereview.chromium.org/468323002/diff/60001/extensions/browser/guest...
File
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h
(right):

https://codereview.chromium.org/468323002/diff/60001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h:14:
class WebViewPermissionHelperDelegate : public content::WebContentsObserver {
On 2014/08/13 23:24:50, Fady Samuel wrote:
> Short comment describing what this is and what this does.

Done.

[Fady Samuel, 2014-08-14 15:01:04]

https://codereview.chromium.org/468323002/diff/140001/chrome/browser/guest_vi...
File
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc
(right):

https://codereview.chromium.org/468323002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:231:
OnGeolocationPermissionResponse,
nit: indent 4 spaces.

https://codereview.chromium.org/468323002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:325:
FileSystemAccessedAsyncResponse,
nit: 4 spaces indent.

[Xi Han, 2014-08-14 15:14:21]

Fady: PTAL;

+Ken: Please review changes in;
- extensions/extensions.gyp

https://codereview.chromium.org/468323002/diff/140001/chrome/browser/guest_vi...
File
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc
(right):

https://codereview.chromium.org/468323002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:231:
OnGeolocationPermissionResponse,
On 2014/08/14 15:01:04, Fady Samuel wrote:
> nit: indent 4 spaces.

Done.

https://codereview.chromium.org/468323002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc:325:
FileSystemAccessedAsyncResponse,
On 2014/08/14 15:01:04, Fady Samuel wrote:
> nit: 4 spaces indent.

Done.

[Fady Samuel, 2014-08-14 15:17:39]

lgtm

[Ken Rockot(use gerrit already), 2014-08-14 16:17:05]

On 2014/08/14 15:17:39, Fady Samuel wrote:
> lgtm

lgtm!

[Xi Han, 2014-08-14 17:04:11]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-08-14 17:07:18]

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

[commit-bot: I haz the power, 2014-08-15 00:17:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-15 06:18:11]

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

[commit-bot: I haz the power, 2014-08-15 06:18:13]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[Xi Han, 2014-08-15 13:32:23]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-08-15 13:33:55]

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

[commit-bot: I haz the power, 2014-08-15 15:17:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[commit-bot: I haz the power, 2014-08-15 15:38:14]

Committed patchset #7 (180001) as 289873