Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(3)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 289793002: Rename sync and async file system access methods in blink. [blink] (1/4) (Closed)

Created:
6 years, 7 months ago by Xi Han
Modified:
6 years, 7 months ago
CC:
blink-reviews, jamesr, tzik, nhiroki, abarth-chromium, dglazkov+blink
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

1. Add requestFileSystemAccessSync(), which will replace allowFileSystem() in the future. 2. Rename requestFileSystemAccess to requestFileSystemAccessAsync. This is the first patch for user story of "file system permission in webview implementation". BUG=343382 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174223

Patch Set 1 : Initial patch #

Total comments: 2

Patch Set 2 : Changes are made. #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats (+47 lines, -17 lines) Patch
M Source/modules/filesystem/FileSystemClient.h View 1 chunk +2 lines, -1 line 0 comments Download
M Source/web/LocalFileSystemClient.h View 1 chunk +2 lines, -1 line 0 comments Download
M Source/web/LocalFileSystemClient.cpp View 1 1 chunk +22 lines, -10 lines 0 comments Download
M Source/web/WorkerPermissionClient.h View 1 chunk +2 lines, -1 line 3 comments Download
M Source/web/WorkerPermissionClient.cpp View 1 chunk +9 lines, -2 lines 0 comments Download
M public/web/WebPermissionClient.h View 1 chunk +4 lines, -1 line 3 comments Download
M public/web/WebWorkerPermissionClientProxy.h View 1 chunk +6 lines, -1 line 1 comment Download

Messages

Total messages: 22 (0 generated)
Xi Han
6 years, 7 months ago (2014-05-14 18:54:55 UTC) #1
Xi Han
Revert the calls that will block blink. Please take another look at. Thanks.
6 years, 7 months ago (2014-05-14 19:54:51 UTC) #2
Fady Samuel
lgtm
6 years, 7 months ago (2014-05-14 20:00:14 UTC) #3
Xi Han
This is the blink's part of rename (related to review https://codereview.chromium.org/273513005/). Thank you for your ...
6 years, 7 months ago (2014-05-14 20:23:23 UTC) #4
jochen (gone - plz use gerrit)
https://codereview.chromium.org/289793002/diff/10001/Source/web/LocalFileSystemClient.cpp File Source/web/LocalFileSystemClient.cpp (right): https://codereview.chromium.org/289793002/diff/10001/Source/web/LocalFileSystemClient.cpp#newcode73 Source/web/LocalFileSystemClient.cpp:73: Document* document = toDocument(context); this should not be reachable, ...
6 years, 7 months ago (2014-05-15 14:35:20 UTC) #5
Xi Han
Cl is updated, and please take another look at. Thanks a lot. https://codereview.chromium.org/289793002/diff/10001/Source/web/LocalFileSystemClient.cpp File Source/web/LocalFileSystemClient.cpp ...
6 years, 7 months ago (2014-05-15 15:33:25 UTC) #6
jochen (gone - plz use gerrit)
lgtm
6 years, 7 months ago (2014-05-15 15:34:30 UTC) #7
kinuko
lgtm2
6 years, 7 months ago (2014-05-16 04:04:33 UTC) #8
kinuko
Wait, I think I got a bit confused. For worker's interface we don't need Async ...
6 years, 7 months ago (2014-05-16 10:18:26 UTC) #9
Xi Han
Yes, you are right. For worker's thread, we will use sync api; while for document's ...
6 years, 7 months ago (2014-05-16 13:05:35 UTC) #10
Xi Han
https://codereview.chromium.org/289793002/diff/40001/Source/web/WorkerPermissionClient.h File Source/web/WorkerPermissionClient.h (right): https://codereview.chromium.org/289793002/diff/40001/Source/web/WorkerPermissionClient.h#newcode58 Source/web/WorkerPermissionClient.h:58: void requestFileSystemAccessAsync(const WebPermissionCallbacks&); In this CL, I only do ...
6 years, 7 months ago (2014-05-16 13:05:51 UTC) #11
Xi Han
The CQ bit was checked by hanxi@chromium.org
6 years, 7 months ago (2014-05-16 13:16:57 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hanxi@chromium.org/289793002/40001
6 years, 7 months ago (2014-05-16 13:17:09 UTC) #13
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are ...
6 years, 7 months ago (2014-05-16 14:34:12 UTC) #14
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 7 months ago (2014-05-16 15:12:32 UTC) #15
commit-bot: I haz the power
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6908)
6 years, 7 months ago (2014-05-16 15:12:32 UTC) #16
Xi Han
The CQ bit was checked by hanxi@chromium.org
6 years, 7 months ago (2014-05-16 21:02:41 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hanxi@chromium.org/289793002/40001
6 years, 7 months ago (2014-05-16 21:03:16 UTC) #18
commit-bot: I haz the power
Change committed as 174223
6 years, 7 months ago (2014-05-17 02:09:36 UTC) #19
kinuko
https://codereview.chromium.org/289793002/diff/40001/Source/web/WorkerPermissionClient.h File Source/web/WorkerPermissionClient.h (right): https://codereview.chromium.org/289793002/diff/40001/Source/web/WorkerPermissionClient.h#newcode58 Source/web/WorkerPermissionClient.h:58: void requestFileSystemAccessAsync(const WebPermissionCallbacks&); On 2014/05/16 13:05:51, hanxi wrote: > ...
6 years, 7 months ago (2014-05-19 08:38:03 UTC) #20
kinuko
On 2014/05/19 08:38:03, kinuko wrote: > https://codereview.chromium.org/289793002/diff/40001/Source/web/WorkerPermissionClient.h > File Source/web/WorkerPermissionClient.h (right): > > https://codereview.chromium.org/289793002/diff/40001/Source/web/WorkerPermissionClient.h#newcode58 > ...
6 years, 7 months ago (2014-05-19 08:44:38 UTC) #21
Xi Han
6 years, 7 months ago (2014-05-20 13:32:26 UTC) #22
Message was sent while issue was closed. 
On 2014/05/19 08:44:38, kinuko wrote:
> On 2014/05/19 08:38:03, kinuko wrote:
> >
>
https://codereview.chromium.org/289793002/diff/40001/Source/web/WorkerPermiss...
> > File Source/web/WorkerPermissionClient.h (right):
> > 
> >
>
https://codereview.chromium.org/289793002/diff/40001/Source/web/WorkerPermiss...
> > Source/web/WorkerPermissionClient.h:58: void
> requestFileSystemAccessAsync(const
> > WebPermissionCallbacks&);
> > On 2014/05/16 13:05:51, hanxi wrote:
> > > In this CL, I only do renaming, since this function is still used in
> > > LocalFileSystemClient.cpp now. It will be removed in the following CL
(issue
> > > 277353002).
> > 
> > It's fine since now it's landed, but I don't think we call this method in
> > LocalFileSystemClient.cpp any more, even without waiting for issue
277353002.
> > (Line 78 in the file, where we call
> >
>
WorkerPermissionClient::from(*toWorkerGlobalScope(context))->requestFileSystemAccessSync()
> > seems to be the only code LocalFileSystemClient calls into this)
> > 

Thank you for the correction.

>
https://codereview.chromium.org/289793002/diff/40001/public/web/WebPermission...
> > File public/web/WebPermissionClient.h (right):
> > 
> >
>
https://codereview.chromium.org/289793002/diff/40001/public/web/WebPermission...
> > public/web/WebPermissionClient.h:53: virtual void
> > requestFileSystemAccessAsync(const WebPermissionCallbacks& callbacks) { }
> > This can be also done in the other CL, but I suspect you'll need default
> > implementation for this to allow the request by default, or will need to
> > implement this method in WebPermissions object so that layout tests can run.
> > (You can look for the callsites of setPermissionClient() and see what we do
> > differently for full chrome / aw case and content_shell case)
> 
> And for this one I think doing the latter will be cleaner, i.e. give mock impl
> to WebPermissions::requestFileSystemAccessAsync for tests.

This change will appear in review 277353002. Thank you for looking into it.

Powered by Google App Engine
This is Rietveld 408576698