Refactor PluginPermissionHelper as WebViewPermissionHelper

WebViewGuest is growing fairly large (~ 1600 lines). This CL aims to move all permission related code to a new helper class WebViewPermissionHelper:

- Rename PluginPermissionHelper to WebViewPermissionHelper;
- Move permission request code:
--- Include permissions: Download, PointerLock, Media, Geolocation, FileSystem, Plugin;
--- Not include: New Window API, Dialog API.

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

[Fady Samuel, 2014-06-20 22:54:34]

https://codereview.chromium.org/347113002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://codereview.chromium.org/347113002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:530:
WebViewPermissionHelper::SET_PERMISSION_INVALID);
This is more readable:

EXTENSION_FUNCTION_VALIDATE(
    result != WebViewPermissionHelper::SET_PERMISSION_INVALID);

https://codereview.chromium.org/347113002/diff/40001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/347113002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_guest.cc:143:
WebViewPermissionHelper::CreateForWebContents(contents);
Let's not manage this here.

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

https://codereview.chromium.org/347113002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_guest.h:404:
WebViewPermissionHelper* web_view_permission_helper_;
Don't make this a pointer. Make it a value.

WebViewPermissionHelper* web_view_permission_helper_;

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

https://codereview.chromium.org/347113002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:51:
DEFINE_WEB_CONTENTS_USER_DATA_KEY(WebViewPermissionHelper);
Don't make this WebContentsUserData. This makes lifetime unnecessarily
complicated.

Instead, you can create a FromWebContents for it as follows:

WebViewGuest* web_view_guest = WebViewGuest::FromWebContents(web_contents);
if (!web_view_guest)
  return NULL;
return web_view_guest->web_view_permission_helper();

https://codereview.chromium.org/347113002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:58:
web_view_guest_(WebViewGuest::FromWebContents(web_contents)) {
Pass this in as a raw pointer from WebViewGuest.

https://codereview.chromium.org/347113002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:161: void
WebViewPermissionHelper::RecordUserInitiatedUMA(
This doesn't need to be public. You can probably put it in an anonymous
namespace.

https://codereview.chromium.org/347113002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:262: void
WebViewPermissionHelper::OnWebViewMediaPermissionResponse(
Replace all instances of OnWebView* to On*

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

https://codereview.chromium.org/347113002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_permission_helper.h:150: bool
allow);
Space

https://codereview.chromium.org/347113002/diff/40001/content/public/browser/w...
File content/public/browser/web_contents_user_data.h (right):

https://codereview.chromium.org/347113002/diff/40001/content/public/browser/w...
content/public/browser/web_contents_user_data.h:53: static T* FromFrameID(int
render_process_id, int render_frame_id) {
Don't modify this file.

[Xi Han, 2014-06-24 13:55:33]

https://codereview.chromium.org/347113002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://codereview.chromium.org/347113002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:530:
WebViewPermissionHelper::SET_PERMISSION_INVALID);
On 2014/06/20 22:54:33, Fady Samuel wrote:
> This is more readable:
> 
> EXTENSION_FUNCTION_VALIDATE(
>     result != WebViewPermissionHelper::SET_PERMISSION_INVALID);

Done.

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

https://codereview.chromium.org/347113002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:51:
DEFINE_WEB_CONTENTS_USER_DATA_KEY(WebViewPermissionHelper);
On 2014/06/20 22:54:33, Fady Samuel wrote:
> Don't make this WebContentsUserData. This makes lifetime unnecessarily
> complicated.
> 
> Instead, you can create a FromWebContents for it as follows:
> 
> WebViewGuest* web_view_guest = WebViewGuest::FromWebContents(web_contents);
> if (!web_view_guest)
>   return NULL;
> return web_view_guest->web_view_permission_helper();

Done.

https://codereview.chromium.org/347113002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:58:
web_view_guest_(WebViewGuest::FromWebContents(web_contents)) {
On 2014/06/20 22:54:33, Fady Samuel wrote:
> Pass this in as a raw pointer from WebViewGuest.

Done.

https://codereview.chromium.org/347113002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:161: void
WebViewPermissionHelper::RecordUserInitiatedUMA(
On 2014/06/20 22:54:33, Fady Samuel wrote:
> This doesn't need to be public. You can probably put it in an anonymous
> namespace.

Done.

https://codereview.chromium.org/347113002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:262: void
WebViewPermissionHelper::OnWebViewMediaPermissionResponse(
On 2014/06/20 22:54:33, Fady Samuel wrote:
> Replace all instances of OnWebView* to On*

Done.

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

https://codereview.chromium.org/347113002/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_permission_helper.h:150: bool
allow);
On 2014/06/20 22:54:33, Fady Samuel wrote:
> Space

Done.

https://codereview.chromium.org/347113002/diff/40001/content/public/browser/w...
File content/public/browser/web_contents_user_data.h (right):

https://codereview.chromium.org/347113002/diff/40001/content/public/browser/w...
content/public/browser/web_contents_user_data.h:53: static T* FromFrameID(int
render_process_id, int render_frame_id) {
On 2014/06/20 22:54:33, Fady Samuel wrote:
> Don't modify this file.

Done.

[Xi Han, 2014-06-26 16:12:47]

I fixed the permission tests failures, please take another look. Thanks!

[Fady Samuel, 2014-06-27 18:46:08]

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
File chrome/browser/guest_view/guest_view_base.h (right):

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/guest_view_base.h:208: // Change this function as a
public function because WebViewGuest will use it
Remove this part of the comment.

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/guest_view_base.h:210: void DispatchEvent(Event*
event);
Couple you please rename this to DispatchEventToEmbedder? Thanks!

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

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:367:
web_view_permission_helper_.reset(new WebViewPermissionHelper(this));
Move this inside AttachWebViewHelpers.

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:627:
WebViewPermissionHelper::SetPermissionResult WebViewGuest::SetPermission(
Do we need this here at all? What if we remove it?

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:632: action,
Fix alignment.

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:683: void
WebViewGuest::FileSystemAccessedAsync(int render_process_id,
Do we need this at all? What if we simply move it to the
WebViewPermissionHelper?

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:690:
DCHECK(web_view_permission_helper);
This DCHECK is not necessary. It'll crash on the next line if
web_view_permission_helper is NULL.

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:699: void
WebViewGuest::FileSystemAccessedSync(int render_process_id,
Do we need this here at all?

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:706:
DCHECK(web_view_permission_helper);
This DCHECK isn't necessary.

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:916: request,
alignment.

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

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:162: return
web_view_guest->GetWebViewPermissionHelper();
Remove this public accessor and make WebviewPermissionHelper a friend of
WebViewGuest.

[Xi Han, 2014-07-03 18:55:59]

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
File chrome/browser/guest_view/guest_view_base.h (right):

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/guest_view_base.h:208: // Change this function as a
public function because WebViewGuest will use it
On 2014/06/27 18:46:07, Fady Samuel wrote:
> Remove this part of the comment.

Done.

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/guest_view_base.h:210: void DispatchEvent(Event*
event);
On 2014/06/27 18:46:07, Fady Samuel wrote:
> Couple you please rename this to DispatchEventToEmbedder? Thanks!

Done.

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

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:367:
web_view_permission_helper_.reset(new WebViewPermissionHelper(this));
On 2014/06/27 18:46:07, Fady Samuel wrote:
> Move this inside AttachWebViewHelpers.

AttachWebViewHelpers is not a member function of WebViewGuest. So if moving the
code to there, we need to obtain the permission helper from webcontent. It will
add extra work to initialize the permission help. What do you think?

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:627:
WebViewPermissionHelper::SetPermissionResult WebViewGuest::SetPermission(
On 2014/06/27 18:46:07, Fady Samuel wrote:
> Do we need this here at all? What if we remove it?

Done.

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:632: action,
On 2014/06/27 18:46:07, Fady Samuel wrote:
> Fix alignment.

Done.

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:683: void
WebViewGuest::FileSystemAccessedAsync(int render_process_id,
On 2014/06/27 18:46:07, Fady Samuel wrote:
> Do we need this at all? What if we simply move it to the
> WebViewPermissionHelper?

I would like to keep this static function defined here, like an static entry to
its following operations. All of the following code (requestPermission and
callback) could be defined as non-static member functions in
WebVierwPermissionHelper.

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:690:
DCHECK(web_view_permission_helper);
On 2014/06/27 18:46:07, Fady Samuel wrote:
> This DCHECK is not necessary. It'll crash on the next line if
> web_view_permission_helper is NULL.

Done.

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:699: void
WebViewGuest::FileSystemAccessedSync(int render_process_id,
On 2014/06/27 18:46:07, Fady Samuel wrote:
> Do we need this here at all?

Same as above.

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:706:
DCHECK(web_view_permission_helper);
On 2014/06/27 18:46:07, Fady Samuel wrote:
> This DCHECK isn't necessary.

Done.

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:916: request,
On 2014/06/27 18:46:07, Fady Samuel wrote:
> alignment.

Done.

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

https://codereview.chromium.org/347113002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:162: return
web_view_guest->GetWebViewPermissionHelper();
On 2014/06/27 18:46:07, Fady Samuel wrote:
> Remove this public accessor and make WebviewPermissionHelper a friend of
> WebViewGuest.

This is a good idea, but I find out that a public accessor is still needed after
removing SetPermission from WebViewGuest
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...).
We need to get the WebViewPermissionHelper from a given guest.

[Fady Samuel, 2014-07-07 15:01:52]

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:366:
AttachWebViewHelpers(guest_web_contents());
Make AttachWebViewHelpers a non-static method and move this line below into it:

web_view_permission_helper_.reset(new WebViewPermissionHelper(this));

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:676: void
WebViewGuest::FileSystemAccessedAsync(int render_process_id,
Keeping this doesn't make sense to me. All it does is access the
WebViewPermissionHelper. Just access it directly?

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:691: void
WebViewGuest::FileSystemAccessedSync(int render_process_id,
Keeping this doesn't make sense to me. All it does is access the
WebViewPermissionHelper, but we can access one directly.

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:909: url,
Fix alignment.

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_permission_helper.cc (right):

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:136:
weak_factory_(this),
This has to be the last thing declared so that it is the first thing destroyed.

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:137:
next_permission_request_id_(0),
Use this constant to initialize:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/gue...

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:153: return
web_view_guest->GetWebViewPermissionHelper();
Again make this private, rather than a public accessor.

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:162: return
web_view_guest->GetWebViewPermissionHelper();
private rather than a public accessor.

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:532:
base::Owned(new PermissionResponseCallback(callback)),
Align parameters here.

[Xi Han, 2014-07-08 15:59:13]

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:366:
AttachWebViewHelpers(guest_web_contents());
On 2014/07/07 15:01:52, Fady Samuel wrote:
> Make AttachWebViewHelpers a non-static method and move this line below into
it:
> 
> web_view_permission_helper_.reset(new WebViewPermissionHelper(this));

Done.

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:676: void
WebViewGuest::FileSystemAccessedAsync(int render_process_id,
On 2014/07/07 15:01:52, Fady Samuel wrote:
> Keeping this doesn't make sense to me. All it does is access the
> WebViewPermissionHelper. Just access it directly?

Done.

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:691: void
WebViewGuest::FileSystemAccessedSync(int render_process_id,
On 2014/07/07 15:01:51, Fady Samuel wrote:
> Keeping this doesn't make sense to me. All it does is access the
> WebViewPermissionHelper, but we can access one directly.

Done.

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:909: url,
On 2014/07/07 15:01:52, Fady Samuel wrote:
> Fix alignment.

Done.

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_permission_helper.cc (right):

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:136:
weak_factory_(this),
On 2014/07/07 15:01:52, Fady Samuel wrote:
> This has to be the last thing declared so that it is the first thing
destroyed.

Done.

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:137:
next_permission_request_id_(0),
On 2014/07/07 15:01:52, Fady Samuel wrote:
> Use this constant to initialize:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/gue...

Done.

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:153: return
web_view_guest->GetWebViewPermissionHelper();
On 2014/07/07 15:01:52, Fady Samuel wrote:
> Again make this private, rather than a public accessor.

Done.

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:162: return
web_view_guest->GetWebViewPermissionHelper();
On 2014/07/07 15:01:52, Fady Samuel wrote:
> private rather than a public accessor.

Done.

https://codereview.chromium.org/347113002/diff/180001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_permission_helper.cc:532:
base::Owned(new PermissionResponseCallback(callback)),
On 2014/07/07 15:01:52, Fady Samuel wrote:
> Align parameters here.

Done.

[Fady Samuel, 2014-07-08 16:31:04]

https://codereview.chromium.org/347113002/diff/200001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/347113002/diff/200001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.h:325:
WebViewPermissionHelper* GetWebViewPermissionHelper() {
Get rid of this?

https://codereview.chromium.org/347113002/diff/200001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/347113002/diff/200001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:17: #include
"chrome/browser/guest_view/web_view/web_view_guest.h"
Can we remove this include?

https://codereview.chromium.org/347113002/diff/200001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:238: void
ChromeRenderMessageFilter::FileSystemAccessedSyncTaskPostedToUIThread(
The name is way too long.

FileSystemAccessedSyncOnUIThread

https://codereview.chromium.org/347113002/diff/200001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:297: void
ChromeRenderMessageFilter::FileSystemAccessedAsyncTaskPostedToUIThread(
FileSystemAccessedAsyncOnUIThread

[Xi Han, 2014-07-08 19:03:34]

https://codereview.chromium.org/347113002/diff/200001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/347113002/diff/200001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.h:325:
WebViewPermissionHelper* GetWebViewPermissionHelper() {
On 2014/07/08 16:31:03, Fady Samuel wrote:
> Get rid of this?

Done.

https://codereview.chromium.org/347113002/diff/200001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/347113002/diff/200001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:17: #include
"chrome/browser/guest_view/web_view/web_view_guest.h"
On 2014/07/08 16:31:03, Fady Samuel wrote:
> Can we remove this include?

Done.

https://codereview.chromium.org/347113002/diff/200001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:238: void
ChromeRenderMessageFilter::FileSystemAccessedSyncTaskPostedToUIThread(
On 2014/07/08 16:31:03, Fady Samuel wrote:
> The name is way too long.
> 
> FileSystemAccessedSyncOnUIThread

Done.

https://codereview.chromium.org/347113002/diff/200001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:297: void
ChromeRenderMessageFilter::FileSystemAccessedAsyncTaskPostedToUIThread(
On 2014/07/08 16:31:03, Fady Samuel wrote:
> FileSystemAccessedAsyncOnUIThread

Done.

[Fady Samuel, 2014-07-08 19:17:08]

https://codereview.chromium.org/347113002/diff/300001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_permission_helper.h (right):

https://codereview.chromium.org/347113002/diff/300001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_permission_helper.h:20: class
WebViewPermissionHelper
Description of class:

WebViewPermissionHelper manages <webview> permission requests. This helper class
is owned by WebViewGuest. Its purpose is to request permission for various
operations from the <webview> embedder, and reply back via callbacks to the
callers on a response from the embedder.

[Xi Han, 2014-07-08 19:41:37]

https://codereview.chromium.org/347113002/diff/300001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_permission_helper.h (right):

https://codereview.chromium.org/347113002/diff/300001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_permission_helper.h:20: class
WebViewPermissionHelper
On 2014/07/08 19:17:08, Fady Samuel wrote:
> Description of class:
> 
> WebViewPermissionHelper manages <webview> permission requests. This helper
class
> is owned by WebViewGuest. Its purpose is to request permission for various
> operations from the <webview> embedder, and reply back via callbacks to the
> callers on a response from the embedder. 

Done.

[Fady Samuel, 2014-07-08 19:57:59]

lgtm. You'll need other OWNERS.

[Xi Han, 2014-07-08 20:09:15]

Jochen: would you please review following files:

-chrome/browser/geolocation/geolocation_permission_context_extensions.cc
- chrome/browser/renderer_host/chrome_render_message_filter.cc
-  chrome/browser/renderer_host/chrome_render_message_filter.h

Thanks!

[jochen (gone - plz use gerrit), 2014-07-09 09:22:44]

please update the CL description to explain why you change this (also consider
linking to a bug)

codewise lgtm

[Xi Han, 2014-07-09 13:13:34]

On 2014/07/09 09:22:44, jochen wrote:
> please update the CL description to explain why you change this (also consider
> linking to a bug)
> 
> codewise lgtm

I update the CL description:)

[Xi Han, 2014-07-09 13:13:39]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-07-09 13:15:17]

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

[commit-bot: I haz the power, 2014-07-09 13:24:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26444)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/30883)

[commit-bot: I haz the power, 2014-07-09 13:26:12]

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

[commit-bot: I haz the power, 2014-07-09 13:26:13]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26449)

[Xi Han, 2014-07-09 14:07:16]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-07-09 14:08:21]

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

[commit-bot: I haz the power, 2014-07-09 17:44:39]

Change committed as 282080