Plumb file system permission into WebviewGuest.

Plumb file system permission into WebviewGuest.

Two remaining issues:
1. It seems that WebviewGuest doesn't support SharedWorker yet, so this CL
   doesn't include the changes for SharedWorker.
2. To request file system access for operations like getFile(), removeFile() etc., I have to set "unlimitedstorage"
   permission in manifest.json, otherwise will get QuotaExceededError (request persistent storage without "unlimitedstorage" permission). Any idea?  


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

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

[Fady Samuel, 2014-05-29 19:04:26]

This looks reasonable to me so far.

https://codereview.chromium.org/306473012/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/306473012/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:291:
SendMessage(
This appears to be only called once. This doesn't seem like a very useful helper
method. Move the code into here directly.

[Fady Samuel, 2014-05-29 19:04:26]

This looks reasonable to me so far.

https://codereview.chromium.org/306473012/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/306473012/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:291:
SendMessage(
This appears to be only called once. This doesn't seem like a very useful helper
method. Move the code into here directly.

[Xi Han, 2014-05-29 21:10:20]

https://codereview.chromium.org/306473012/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/306473012/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/tab_specific_content_settings.cc:291:
SendMessage(
On 2014/05/29 19:04:32, Fady Samuel wrote:
> This appears to be only called once. This doesn't seem like a very useful
helper
> method. Move the code into here directly.

Done.

[Fady Samuel, 2014-05-29 21:36:24]

Same cleanup suggestions.

https://codereview.chromium.org/306473012/diff/60001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/306473012/diff/60001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1851:
base::Bind(&TabSpecificContentSettings::FileSystemAccessedSync,
TabSpecificContentSettings::FileSystemAccessed

https://codereview.chromium.org/306473012/diff/60001/chrome/browser/renderer_...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (left):

https://codereview.chromium.org/306473012/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_render_message_filter.cc:461: // Record
access to file system for potential display in UI.
Detect WebView up here, and call WebViewGuest::FileSystemAccessedAsync.

https://codereview.chromium.org/306473012/diff/60001/chrome/browser/renderer_...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/306473012/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_render_message_filter.cc:460:
base::Bind(&TabSpecificContentSettings::FileSystemAccessedSync,
TabSpecificContentSettings::FileSystemAccessed

https://codereview.chromium.org/306473012/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_render_message_filter.cc:470:
base::Bind(&TabSpecificContentSettings::FileSystemAccessedSyncDelayReply,
Why not go directly to WebViewGuest?

Implement this in WebViewGuest::FileSystemAccessSync

https://codereview.chromium.org/306473012/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_render_message_filter.cc:491:
base::Bind(&TabSpecificContentSettings::FileSystemAccessedAsync,
TabSpecificContetSettings::FileSystemAccessed

[Xi Han, 2014-05-30 14:38:35]

https://codereview.chromium.org/306473012/diff/60001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/306473012/diff/60001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1851:
base::Bind(&TabSpecificContentSettings::FileSystemAccessedSync,
On 2014/05/29 21:36:25, Fady Samuel wrote:
> TabSpecificContentSettings::FileSystemAccessed

Done.

https://codereview.chromium.org/306473012/diff/60001/chrome/browser/renderer_...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (left):

https://codereview.chromium.org/306473012/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_render_message_filter.cc:461: // Record
access to file system for potential display in UI.
On 2014/05/29 21:36:25, Fady Samuel wrote:
> Detect WebView up here, and call WebViewGuest::FileSystemAccessedAsync.

Done.

https://codereview.chromium.org/306473012/diff/60001/chrome/browser/renderer_...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/306473012/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_render_message_filter.cc:460:
base::Bind(&TabSpecificContentSettings::FileSystemAccessedSync,
On 2014/05/29 21:36:25, Fady Samuel wrote:
> TabSpecificContentSettings::FileSystemAccessed

Done.

https://codereview.chromium.org/306473012/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_render_message_filter.cc:470:
base::Bind(&TabSpecificContentSettings::FileSystemAccessedSyncDelayReply,
On 2014/05/29 21:36:25, Fady Samuel wrote:
> Why not go directly to WebViewGuest?
> 
> Implement this in WebViewGuest::FileSystemAccessSync

Done.

https://codereview.chromium.org/306473012/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_host/chrome_render_message_filter.cc:491:
base::Bind(&TabSpecificContentSettings::FileSystemAccessedAsync,
On 2014/05/29 21:36:25, Fady Samuel wrote:
> TabSpecificContetSettings::FileSystemAccessed

Done.

[fsamuel, 2014-05-30 15:13:21]

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (left):

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1851: i->first, i->second, url,
!allow));
Revert this change.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/content_...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/tab_specific_content_settings.cc:42: using
content::RenderFrameHost;
revert this change.

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

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:137: case
WEB_VIEW_PERMISSION_TYPE_FILESYSTEM:
Order alphabetically.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:262: case
WEB_VIEW_PERMISSION_TYPE_FILESYSTEM:
Order alphabetically

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:297: case
WEB_VIEW_PERMISSION_TYPE_FILESYSTEM:
Order alphabetically.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:825: bool
blocked_by_policy) {
This should be passing into allowed_by_default

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:826:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
Drop this.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:839: void
WebViewGuest::FileSystemAccessedAsyncInternal(int render_process_id,
WebViewGuest::FileSystemAccessedAsyncResponse

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:843: bool
blocked_by_policy) {
This should be passed into allowed_by_default.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:844:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
Drop this.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:855: WebViewGuest*
WebViewGuest::GetWebViewGuest(int render_process_id,
Call it from WebViewGuest::FromFrame, and make it public. Move it near
FromWebContents and From

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:858:
content::RenderFrameHost::FromID(render_process_id, render_frame_id);
if (!render_frame_host)
  return NULL;

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:865: void
WebViewGuest::FileSystemAccessedSyncDelayReply(int render_process_id,
WebViewGuest::FileSystemAccessedSync

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:870:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
Drop this. Everything in WebViewGuest is assumed UI unless specified otherwise.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:883: void
WebViewGuest::FileSystemAccessedSyncDelayReplyInternal(
WebViewGuest::FileSystemAccessedSyncResponse

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:888: bool
blocked_by_policy) {
This is not blocked_by_policy. This is allowed.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:889:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
Drop this. Everything in WebViewGuest is assumed UI thread unless specified
otherwise.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:893: reply_msg,
!blocked_by_policy);
Do you have the logic reversed here?

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:974: bool
WebViewGuest::OnMessageReceived(
Revert this part. This seems unnecessary.

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

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:469:
base::Bind(&WebViewGuest::FileSystemAccessedSyncDelayReply,
WebViewGuest::FileSystemAccessedSync is sufficient to distinguish it from the
other method.

[Xi Han, 2014-05-30 18:15:56]

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (left):

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1851: i->first, i->second, url,
!allow));
On 2014/05/30 15:13:21, fsamuel wrote:
> Revert this change.

Done.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/content_...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/tab_specific_content_settings.cc:42: using
content::RenderFrameHost;
On 2014/05/30 15:13:21, fsamuel wrote:
> revert this change.

Done.

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

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:137: case
WEB_VIEW_PERMISSION_TYPE_FILESYSTEM:
On 2014/05/30 15:13:21, fsamuel wrote:
> Order alphabetically.

Done.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:262: case
WEB_VIEW_PERMISSION_TYPE_FILESYSTEM:
On 2014/05/30 15:13:21, fsamuel wrote:
> Order alphabetically

Done.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:297: case
WEB_VIEW_PERMISSION_TYPE_FILESYSTEM:
On 2014/05/30 15:13:21, fsamuel wrote:
> Order alphabetically. 

Done.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:825: bool
blocked_by_policy) {
On 2014/05/30 15:13:21, fsamuel wrote:
> This should be passing into allowed_by_default

Done.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:826:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2014/05/30 15:13:21, fsamuel wrote:
> Drop this.

Done.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:839: void
WebViewGuest::FileSystemAccessedAsyncInternal(int render_process_id,
On 2014/05/30 15:13:21, fsamuel wrote:
> WebViewGuest::FileSystemAccessedAsyncResponse

Done.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:843: bool
blocked_by_policy) {
On 2014/05/30 15:13:21, fsamuel wrote:
> This should be passed into allowed_by_default.

Not this one, but the one in
WebViewGuest::FileSystemAccessedSyncDelayReply（...）. Thanks for pointing it out
anyway.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:844:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2014/05/30 15:13:21, fsamuel wrote:
> Drop this.

Done.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:855: WebViewGuest*
WebViewGuest::GetWebViewGuest(int render_process_id,
On 2014/05/30 15:13:21, fsamuel wrote:
> Call it from WebViewGuest::FromFrame, and make it public. Move it near
> FromWebContents and From

Done.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:858:
content::RenderFrameHost::FromID(render_process_id, render_frame_id);
On 2014/05/30 15:13:21, fsamuel wrote:
> if (!render_frame_host)
>   return NULL;

Done.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:865: void
WebViewGuest::FileSystemAccessedSyncDelayReply(int render_process_id,
On 2014/05/30 15:13:21, fsamuel wrote:
> WebViewGuest::FileSystemAccessedSync

Done.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:870:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2014/05/30 15:13:21, fsamuel wrote:
> Drop this. Everything in WebViewGuest is assumed UI unless specified
otherwise.

Done.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:883: void
WebViewGuest::FileSystemAccessedSyncDelayReplyInternal(
On 2014/05/30 15:13:21, fsamuel wrote:
> WebViewGuest::FileSystemAccessedSyncResponse

Done.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:888: bool
blocked_by_policy) {
On 2014/05/30 15:13:21, fsamuel wrote:
> This is not blocked_by_policy. This is allowed.

Done.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:889:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2014/05/30 15:13:21, fsamuel wrote:
> Drop this. Everything in WebViewGuest is assumed UI thread unless specified
> otherwise.

Done.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:889:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2014/05/30 15:13:21, fsamuel wrote:
> Drop this. Everything in WebViewGuest is assumed UI thread unless specified
> otherwise.

Done.

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:974: bool
WebViewGuest::OnMessageReceived(
On 2014/05/30 15:13:21, fsamuel wrote:
> Revert this part. This seems unnecessary.

This is necessary to add "content::" before RenderFrameHost, because in
WebviewGuest.cc I uses RenderFrameHost within my changes, so I include
"render_frame_host.h" and remove its forward declaration.

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

https://codereview.chromium.org/306473012/diff/100001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:469:
base::Bind(&WebViewGuest::FileSystemAccessedSyncDelayReply,
On 2014/05/30 15:13:21, fsamuel wrote:
> WebViewGuest::FileSystemAccessedSync is sufficient to distinguish it from the
> other method.

Done.

[Fady Samuel, 2014-05-30 18:26:12]

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_guest.cc (left):

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:72: using
content::RenderFrameHost;
Put this back.

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

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:850:
content::RenderFrameHost::FromID(render_process_id, render_frame_id);
It is possible for the frame to go away between the time the request is made and
the time we receive a response.

if (!render_frame_host)
  return;

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:886:
content::RenderFrameHost::FromID(render_process_id, render_frame_id);
if (!render_frame_host)
  return;

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:965: bool
WebViewGuest::OnMessageReceived(
Restore this.

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

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.h:190: void
OnWebViewFileSystemPermissionResponse(
This should actually be private. I misplaced these responses here. They should
all be private I think.

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.h:256: static void
FileSystemAccessedAsync(int render_process_id,
Description of what this is and what it does.

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.h:262: static void
FileSystemAccessedSync(int render_process_id,
Description of what this is and what it does.

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_permission_types.h (right):

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_permission_types.h:30:
WEB_VIEW_PERMISSION_TYPE_FILESYSTEM
alphabetical order please.

[Xi Han, 2014-05-30 19:35:29]

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_guest.cc (left):

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:72: using
content::RenderFrameHost;
On 2014/05/30 18:26:13, Fady Samuel wrote:
> Put this back.

Done.

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

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:850:
content::RenderFrameHost::FromID(render_process_id, render_frame_id);
On 2014/05/30 18:26:13, Fady Samuel wrote:
> It is possible for the frame to go away between the time the request is made
and
> the time we receive a response.
> 
> if (!render_frame_host)
>   return;

Done.

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:886:
content::RenderFrameHost::FromID(render_process_id, render_frame_id);
On 2014/05/30 18:26:13, Fady Samuel wrote:
> if (!render_frame_host)
>   return;

Done.

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:965: bool
WebViewGuest::OnMessageReceived(
On 2014/05/30 18:26:13, Fady Samuel wrote:
> Restore this.

Done.

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

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.h:190: void
OnWebViewFileSystemPermissionResponse(
On 2014/05/30 18:26:13, Fady Samuel wrote:
> This should actually be private. I misplaced these responses here. They should
> all be private I think.

Move all of them to private.

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.h:256: static void
FileSystemAccessedAsync(int render_process_id,
On 2014/05/30 18:26:13, Fady Samuel wrote:
> Description of what this is and what it does.

Done.

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.h:262: static void
FileSystemAccessedSync(int render_process_id,
On 2014/05/30 18:26:13, Fady Samuel wrote:
> Description of what this is and what it does.

Done.

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_permission_types.h (right):

https://codereview.chromium.org/306473012/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_permission_types.h:30:
WEB_VIEW_PERMISSION_TYPE_FILESYSTEM
On 2014/05/30 18:26:13, Fady Samuel wrote:
> alphabetical order please.

Sorry for missing this one.

[fsamuel, 2014-05-30 19:46:51]

lgtm with nits

https://codereview.chromium.org/306473012/diff/160001/chrome/browser/guest_vi...
File chrome/browser/guest_view/guest_view.h (right):

https://codereview.chromium.org/306473012/diff/160001/chrome/browser/guest_vi...
chrome/browser/guest_view/guest_view.h:27: static T* FromFrame(int
render_process_id, int render_frame_id) {
Rename to FromFrameID?

https://codereview.chromium.org/306473012/diff/160001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_guest.h (left):

https://codereview.chromium.org/306473012/diff/160001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.h:266: const
ui::SimpleMenuModel& menu_model);
space between this and the next method.

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

https://codereview.chromium.org/306473012/diff/160001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.h:223: // was accessed. It
will ask the embbedder whether the guest has the
Called when file system access is requested by the guest content using the
asynchronous HTML5 file system API. The request is plumbed through the <webview>
permission request API. The request will be:
- Allowed if the embedder explicitly allowed it.
- Denied if the embedder explicitly denied.
- Determined by the guest's content settings if the embedder does not perform an
explicit action.
  If access was blocked due to the page's content settings, |blocked_by_policy|
should be true, and this function should invoke OnContentBlocked.

https://codereview.chromium.org/306473012/diff/160001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.h:247: // OnContentBlocked.
Called when file system access is requested by the guest content using the
synchronous HTML5 file system API in a worker thread or shared worker. The
request is plumbed through the <webview> permission request API. The request
will be:
- Allowed if the embedder explicitly allowed it.
- Denied if the embedder explicitly denied.
- Determined by the guest's content settings if the embedder does not perform an
explicit action.
  If access was blocked due to the page's content settings, |blocked_by_policy|
should be true, and this function should invoke OnContentBlocked.

[Xi Han, 2014-05-30 20:27:45]

https://codereview.chromium.org/306473012/diff/160001/chrome/browser/guest_vi...
File chrome/browser/guest_view/guest_view.h (right):

https://codereview.chromium.org/306473012/diff/160001/chrome/browser/guest_vi...
chrome/browser/guest_view/guest_view.h:27: static T* FromFrame(int
render_process_id, int render_frame_id) {
On 2014/05/30 19:46:51, fsamuel wrote:
> Rename to FromFrameID?

Done.

https://codereview.chromium.org/306473012/diff/160001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_guest.h (left):

https://codereview.chromium.org/306473012/diff/160001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.h:266: const
ui::SimpleMenuModel& menu_model);
On 2014/05/30 19:46:51, fsamuel wrote:
> space between this and the next method.

Done.

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

https://codereview.chromium.org/306473012/diff/160001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.h:223: // was accessed. It
will ask the embbedder whether the guest has the
On 2014/05/30 19:46:51, fsamuel wrote:
> Called when file system access is requested by the guest content using the
> asynchronous HTML5 file system API. The request is plumbed through the
<webview>
> permission request API. The request will be:
> - Allowed if the embedder explicitly allowed it.
> - Denied if the embedder explicitly denied.
> - Determined by the guest's content settings if the embedder does not perform
an
> explicit action.
>   If access was blocked due to the page's content settings,
|blocked_by_policy|
> should be true, and this function should invoke OnContentBlocked.

Done.

https://codereview.chromium.org/306473012/diff/160001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.h:247: // OnContentBlocked.
On 2014/05/30 19:46:51, fsamuel wrote:
> Called when file system access is requested by the guest content using the
> synchronous HTML5 file system API in a worker thread or shared worker. The
> request is plumbed through the <webview> permission request API. The request
> will be:
> - Allowed if the embedder explicitly allowed it.
> - Denied if the embedder explicitly denied.
> - Determined by the guest's content settings if the embedder does not perform
an
> explicit action.
>   If access was blocked due to the page's content settings,
|blocked_by_policy|
> should be true, and this function should invoke OnContentBlocked.

Done.

[jochen (gone - plz use gerrit), 2014-06-02 07:14:07]

what bits do you want me to look at?

[Xi Han, 2014-06-02 12:51:18]

On 2014/06/02 07:14:07, jochen wrote:
> what bits do you want me to look at?

I found you are one of the owners of these files that I changes, so I would like
you to review the major parts of the changes, such as:
 - chrome/browser/renderer_host/chrome_render_message_filter.h
 - chrome/browser/renderer_host/chrome_render_message_filter.cc
 - chrome/browser/guest_view/web_view/web_view_guest.h
 - chrome/browser/guest_view/web_view/web_view_guest.cc

Thanks a lot for your help.

[jochen (gone - plz use gerrit), 2014-06-02 13:20:22]

the webview stuff should be covered by Fady? I don't really know about the
plumbing

chrome message filter lgtm

[Xi Han, 2014-06-03 13:29:54]

Alexei: 
would you please review file: tools/metrics/actions/actions.xml?

Thanks a lot.

[Alexei Svitkine (slow), 2014-06-03 14:18:35]

https://codereview.chromium.org/306473012/diff/180001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/306473012/diff/180001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:10839: <owner>Please list the metric's owners.
Add more owner tags as needed.</owner>
Please fill out these fields by setting yourself as owner and providing a
description. Thanks!

[Xi Han, 2014-06-03 15:25:50]

Alexei： I updated my CL, please take another look. Thanks!

https://codereview.chromium.org/306473012/diff/180001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/306473012/diff/180001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:10839: <owner>Please list the metric's owners.
Add more owner tags as needed.</owner>
On 2014/06/03 14:18:35, Alexei Svitkine wrote:
> Please fill out these fields by setting yourself as owner and providing a
> description. Thanks!

Done.

[Alexei Svitkine (slow), 2014-06-03 15:35:06]

lgtm

[Xi Han, 2014-06-03 19:54:52]

The test for DefaultDeny isn't included in this CL. I submit changes for other
tests first, and will upload DefaultDeny part later. Thanks you for reviewing my
changes.

[Fady Samuel, 2014-06-03 20:46:54]

Some additional comments.

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

https://codereview.chromium.org/306473012/diff/270001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:494: !allowed));
Make reply_msg the last parameter here. See the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/embedder.html
(right):

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/embedder.html:2:
* Copyright 2013 The Chromium Authors. All rights reserved.  Use of this
2014

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/embedder.js
(right):

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/embedder.js:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/embedder.js:143:
// denies the access to filesystem for the guest.
denies access

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/guest_main.html
(right):

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/guest_main.html:27:
console.log('maybeNotifyEmbedder' +
Be consistent: window.console.log

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/guest_main.html:45:
console.log('set totalCallbackCount to 0');
Be consistent: window.console.log

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/guest_main.html:48:
console.log('Call requestFileSystemAccess');
Be consistent: window.console.log

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/guest_main.html:55:
console.log('request Quota granted.');
Be consistent: window.console.log

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/guest_main.html:66:
console.log('onFileSystemSuccess, successCallbackCount: ' + successCallbackCount
+
Be consistent: window.console.log

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/guest_main.html:74:
console.log('onFileSystemFailure, totalCallbackCount: ' +
Be consistent: window.console.log

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/guest_main.html:80:
console.log('guest.onPostMessageReceived');
Be consistent: window.console.log

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/guest_main.html:95:
console.log('Guest loaded');
Be consistent: window.console.log

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/web_view/filesystem/main/test.js
(right):

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/test.js:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/embedder.html
(right):

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/embedder.html:2:
* Copyright 2013 The Chromium Authors. All rights reserved.  Use of this
2014

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/embedder.js
(right):

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/embedder.js:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/guest_worker.html
(right):

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/guest_worker.html:2:
* Copyright 2013 The Chromium Authors. All rights reserved.  Use of this
2014

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/guest_worker.html:23:
console.log('maybeNotifyEmbedder' +
Be consistent: window.console.log

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/guest_worker.html:74:
console.log('guest.onPostMessageReceived');
Be consistent: window.console.log

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/guest_worker.html:89:
console.log('Guest loaded');
Be consistent: window.console.log

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/test.js
(right):

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/test.js:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/worker.js
(right):

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/worker.js:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
2014

[Xi Han, 2014-06-04 19:08:20]

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

https://codereview.chromium.org/306473012/diff/270001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:494: !allowed));
On 2014/06/03 20:46:52, Fady Samuel wrote:
> Make reply_msg the last parameter here. See the style guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/embedder.html
(right):

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/embedder.html:2:
* Copyright 2013 The Chromium Authors. All rights reserved.  Use of this
On 2014/06/03 20:46:52, Fady Samuel wrote:
> 2014

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/embedder.js
(right):

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/embedder.js:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
On 2014/06/03 20:46:52, Fady Samuel wrote:
> 2014

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/embedder.js:143:
// denies the access to filesystem for the guest.
On 2014/06/03 20:46:52, Fady Samuel wrote:
> denies access

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/guest_main.html
(right):

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/guest_main.html:27:
console.log('maybeNotifyEmbedder' +
On 2014/06/03 20:46:52, Fady Samuel wrote:
> Be consistent: window.console.log

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/guest_main.html:45:
console.log('set totalCallbackCount to 0');
On 2014/06/03 20:46:52, Fady Samuel wrote:
> Be consistent: window.console.log

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/guest_main.html:48:
console.log('Call requestFileSystemAccess');
On 2014/06/03 20:46:53, Fady Samuel wrote:
> Be consistent: window.console.log

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/guest_main.html:55:
console.log('request Quota granted.');
On 2014/06/03 20:46:52, Fady Samuel wrote:
> Be consistent: window.console.log

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/guest_main.html:66:
console.log('onFileSystemSuccess, successCallbackCount: ' + successCallbackCount
+
On 2014/06/03 20:46:52, Fady Samuel wrote:
> Be consistent: window.console.log

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/guest_main.html:74:
console.log('onFileSystemFailure, totalCallbackCount: ' +
On 2014/06/03 20:46:52, Fady Samuel wrote:
> Be consistent: window.console.log

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/guest_main.html:80:
console.log('guest.onPostMessageReceived');
On 2014/06/03 20:46:52, Fady Samuel wrote:
> Be consistent: window.console.log

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/guest_main.html:95:
console.log('Guest loaded');
On 2014/06/03 20:46:52, Fady Samuel wrote:
> Be consistent: window.console.log

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/web_view/filesystem/main/test.js
(right):

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/main/test.js:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
On 2014/06/03 20:46:53, Fady Samuel wrote:
> 2014

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/embedder.html
(right):

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/embedder.html:2:
* Copyright 2013 The Chromium Authors. All rights reserved.  Use of this
On 2014/06/03 20:46:53, Fady Samuel wrote:
> 2014

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/embedder.js
(right):

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/embedder.js:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
On 2014/06/03 20:46:53, Fady Samuel wrote:
> 2014

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/guest_worker.html
(right):

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/guest_worker.html:2:
* Copyright 2013 The Chromium Authors. All rights reserved.  Use of this
On 2014/06/03 20:46:53, Fady Samuel wrote:
> 2014

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/guest_worker.html:23:
console.log('maybeNotifyEmbedder' +
On 2014/06/03 20:46:53, Fady Samuel wrote:
> Be consistent: window.console.log

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/guest_worker.html:74:
console.log('guest.onPostMessageReceived');
On 2014/06/03 20:46:53, Fady Samuel wrote:
> Be consistent: window.console.log

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/guest_worker.html:89:
console.log('Guest loaded');
On 2014/06/03 20:46:53, Fady Samuel wrote:
> Be consistent: window.console.log

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/test.js
(right):

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/test.js:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
On 2014/06/03 20:46:53, Fady Samuel wrote:
> 2014

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/test.js:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
On 2014/06/03 20:46:53, Fady Samuel wrote:
> 2014

Done.

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/worker.js
(right):

https://codereview.chromium.org/306473012/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/filesystem/worker/worker.js:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
On 2014/06/03 20:46:54, Fady Samuel wrote:
> 2014

Done.

[Xi Han, 2014-06-05 13:08:49]

Benjamin: would you please review the following file:
-chrome/browser/apps/web_view_browsertest.cc
Thanks a lot!

[not at google - send to devlin, 2014-06-05 16:57:03]

https://codereview.chromium.org/306473012/diff/330001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/306473012/diff/330001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:695: 'filesystem'];
what does it mean for a webview to have the 'filesystem' permission?

[Xi Han, 2014-06-05 17:00:00]

https://codereview.chromium.org/306473012/diff/330001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/306473012/diff/330001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:695: 'filesystem'];
On 2014/06/05 16:57:04, kalman wrote:
> what does it mean for a webview to have the 'filesystem' permission?

From my understanding, it means the embedder can change the default permission
of file system access request from a guest, by explicitly allow or deny.

[not at google - send to devlin, 2014-06-05 17:03:17]

https://codereview.chromium.org/306473012/diff/330001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/306473012/diff/330001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:695: 'filesystem'];
On 2014/06/05 16:59:59, hanxi wrote:
> On 2014/06/05 16:57:04, kalman wrote:
> > what does it mean for a webview to have the 'filesystem' permission?
> 
> From my understanding, it means the embedder can change the default permission
> of file system access request from a guest, by explicitly allow or deny.

what kind of file system access? like access outside the sandbox? this isn't
related to chrome.fileSystem, right..?

[Xi Han, 2014-06-05 17:09:37]

https://codereview.chromium.org/306473012/diff/330001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/306473012/diff/330001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:695: 'filesystem'];
On 2014/06/05 17:03:17, kalman wrote:
> On 2014/06/05 16:59:59, hanxi wrote:
> > On 2014/06/05 16:57:04, kalman wrote:
> > > what does it mean for a webview to have the 'filesystem' permission?
> > 
> > From my understanding, it means the embedder can change the default
permission
> > of file system access request from a guest, by explicitly allow or deny.

It is html file system access, starting from window.requestFileSystem(..). No,
this isn't related to chrome.filesystem.
> 
> what kind of file system access? like access outside the sandbox? this isn't
> related to chrome.fileSystem, right..?

[not at google - send to devlin, 2014-06-05 17:12:50]

https://codereview.chromium.org/306473012/diff/330001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/306473012/diff/330001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:695: 'filesystem'];
On 2014/06/05 17:09:37, hanxi wrote:
> On 2014/06/05 17:03:17, kalman wrote:
> > On 2014/06/05 16:59:59, hanxi wrote:
> > > On 2014/06/05 16:57:04, kalman wrote:
> > > > what does it mean for a webview to have the 'filesystem' permission?
> > > 
> > > From my understanding, it means the embedder can change the default
> permission
> > > of file system access request from a guest, by explicitly allow or deny.
> 
> It is html file system access, starting from window.requestFileSystem(..). No,
> this isn't related to chrome.filesystem.
> > 
> > what kind of file system access? like access outside the sandbox? this isn't
> > related to chrome.fileSystem, right..?
> 

Ok cool.

I'm all for explicitly enabling features of webviews, but why is access to the
sandboxed file system prevented? seems pretty harmless.

[Xi Han, 2014-06-05 17:24:00]

https://codereview.chromium.org/306473012/diff/330001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/306473012/diff/330001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:695: 'filesystem'];
On 2014/06/05 17:12:50, kalman wrote:
> On 2014/06/05 17:09:37, hanxi wrote:
> > On 2014/06/05 17:03:17, kalman wrote:
> > > On 2014/06/05 16:59:59, hanxi wrote:
> > > > On 2014/06/05 16:57:04, kalman wrote:
> > > > > what does it mean for a webview to have the 'filesystem' permission?
> > > > 
> > > > From my understanding, it means the embedder can change the default
> > permission
> > > > of file system access request from a guest, by explicitly allow or deny.
> > 
> > It is html file system access, starting from window.requestFileSystem(..).
No,
> > this isn't related to chrome.filesystem.
> > > 
> > > what kind of file system access? like access outside the sandbox? this
isn't
> > > related to chrome.fileSystem, right..?
> > 
> 
> Ok cool.
> 
> I'm all for explicitly enabling features of webviews, but why is access to the
> sandboxed file system prevented? seems pretty harmless.

Honestly, I don't know exactly why, but maybe because there are some cases that
the embedder don't want the guest to access local file systems, such as create a
new file, or delete a file.

[not at google - send to devlin, 2014-06-05 17:30:39]

^ fady?

https://codereview.chromium.org/306473012/diff/330001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/306473012/diff/330001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:695: 'filesystem'];
> 
> Honestly, I don't know exactly why, but maybe because there are some cases
that
> the embedder don't want the guest to access local file systems, such as create
a
> new file, or delete a file.

That would make sense, but I thought requestFileSystem was for the sandboxed
file system and not related to <input type=file> or whatever (i.e.
out-of-sandbox file system).

and there's nothing dangerous about sandboxed file system access.

[Xi Han, 2014-06-05 17:45:07]

https://codereview.chromium.org/306473012/diff/330001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/306473012/diff/330001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view.js:695: 'filesystem'];
On 2014/06/05 17:30:39, kalman wrote:
> > 
> > Honestly, I don't know exactly why, but maybe because there are some cases
> that
> > the embedder don't want the guest to access local file systems, such as
create
> a
> > new file, or delete a file.
> 
> That would make sense, but I thought requestFileSystem was for the sandboxed
> file system and not related to <input type=file> or whatever (i.e.
> out-of-sandbox file system).
> 
> and there's nothing dangerous about sandboxed file system access.

The guest can request both "Temporary" or "Persistent" file system access, maybe
the "persistent" access sometime is considered not safe. This is just from my
guess, and maybe we just want to implement this permission as the same way as
other permissions.

[Fady Samuel, 2014-06-09 14:44:48]

On 2014/06/05 17:45:07, hanxi wrote:
>
https://codereview.chromium.org/306473012/diff/330001/chrome/renderer/resourc...
> File chrome/renderer/resources/extensions/web_view.js (right):
> 
>
https://codereview.chromium.org/306473012/diff/330001/chrome/renderer/resourc...
> chrome/renderer/resources/extensions/web_view.js:695: 'filesystem'];
> On 2014/06/05 17:30:39, kalman wrote:
> > > 
> > > Honestly, I don't know exactly why, but maybe because there are some cases
> > that
> > > the embedder don't want the guest to access local file systems, such as
> create
> > a
> > > new file, or delete a file.
> > 
> > That would make sense, but I thought requestFileSystem was for the sandboxed
> > file system and not related to <input type=file> or whatever (i.e.
> > out-of-sandbox file system).
> > 
> > and there's nothing dangerous about sandboxed file system access.
> 
> The guest can request both "Temporary" or "Persistent" file system access,
maybe
> the "persistent" access sometime is considered not safe. This is just from my
> guess, and maybe we just want to implement this permission as the same way as
> other permissions.

lgtm for tests

[not at google - send to devlin, 2014-06-09 21:30:54]

lgtm

[Xi Han, 2014-06-09 21:36:18]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-06-09 21:38:47]

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

[commit-bot: I haz the power, 2014-06-10 09:46:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  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/19...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-06-10 11:20:42]

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

[commit-bot: I haz the power, 2014-06-10 11:20:43]

Try jobs failed on following builders:
  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...)

[Xi Han, 2014-06-10 13:11:25]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-06-10 13:13:11]

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

[commit-bot: I haz the power, 2014-06-10 21:29:04]

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

[commit-bot: I haz the power, 2014-06-10 22:20:11]

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

[commit-bot: I haz the power, 2014-06-10 22:20:12]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[Xi Han, 2014-06-11 03:26:19]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-06-11 03:29:44]

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

[commit-bot: I haz the power, 2014-06-11 04:26:58]

Change committed as 276251

[Xi Han, 2014-06-11 15:10:44]

Please take another look at. Thanks.

[fsamuel, 2014-06-11 15:38:37]

lgtm with two nits.

https://codereview.chromium.org/306473012/diff/410001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (left):

https://codereview.chromium.org/306473012/diff/410001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:198: // Record
access to file system for potential display in UI.
You lost this comment. Add it back.

https://codereview.chromium.org/306473012/diff/410001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:214: // Record
access to file system for potential display in UI.
Add this comment back.

[Xi Han, 2014-06-11 15:46:32]

https://codereview.chromium.org/306473012/diff/410001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (left):

https://codereview.chromium.org/306473012/diff/410001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:198: // Record
access to file system for potential display in UI.
On 2014/06/11 15:38:36, fsamuel wrote:
> You lost this comment. Add it back.

Done.

https://codereview.chromium.org/306473012/diff/410001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:214: // Record
access to file system for potential display in UI.
On 2014/06/11 15:38:36, fsamuel wrote:
> Add this comment back.

Done.

[fsamuel, 2014-06-11 15:48:01]

lgtm

[Xi Han, 2014-06-11 15:48:36]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-06-11 15:51:31]

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

[commit-bot: I haz the power, 2014-06-11 21:14:14]

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

[commit-bot: I haz the power, 2014-06-11 21:28:16]

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

[commit-bot: I haz the power, 2014-06-11 21:28:18]

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

[Xi Han, 2014-06-11 21:48:07]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-06-11 21:50:23]

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

[commit-bot: I haz the power, 2014-06-11 22:22:36]

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

[commit-bot: I haz the power, 2014-06-11 22:27:09]

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

[commit-bot: I haz the power, 2014-06-11 22:27:10]

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

[Xi Han, 2014-06-12 03:06:12]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-06-12 03:07:39]

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

[commit-bot: I haz the power, 2014-06-12 08:28:08]

Change committed as 276584