Bug 60683

Summary: [chromium] Make openFileSystem check for permission first
Product: WebKit Reporter: John Abd-El-Malek <jam>
Component: New BugsAssignee: John Abd-El-Malek <jam>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ericu, eric, jamesr, kinuko, michaeln, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch jamesr: review+

John Abd-El-Malek
Reported 2011-05-11 18:54:43 PDT
[chromium] Make openFileSystem check for permission first
Attachments
Patch (10.90 KB, patch)
2011-05-11 18:55 PDT, John Abd-El-Malek
no flags
Patch (10.90 KB, patch)
2011-05-11 19:02 PDT, John Abd-El-Malek
no flags
Patch (10.74 KB, patch)
2011-05-12 09:07 PDT, John Abd-El-Malek
jamesr: review+
John Abd-El-Malek
Comment 1 2011-05-11 18:55:07 PDT
John Abd-El-Malek
Comment 2 2011-05-11 19:02:38 PDT
John Abd-El-Malek
Comment 3 2011-05-11 19:16:21 PDT
James/Tony: please review others: FYI since you work on the FS code. I had done this pattern for all the other storage APIs, and just realized I missed FS. The motivation is that the code in src\content shouldn't know about content settings, so splitting the opening into two steps helps that. The chrome side is http://codereview.chromium.org/7012007
Michael Nordman
Comment 4 2011-05-11 19:51:55 PDT
Comment on attachment 93233 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93233&action=review > Source/WebKit/chromium/public/WebCommonWorkerClient.h:90 > + virtual bool allowFileSystem(WebFrame*) The WebFrame param does not belong in this interface. I know it's in the allowDatabase() method, but it doesn't belong there either. The origin of the worker can be determined w/o that parameter just fine since there's a 'client' per worker. > Source/WebKit/chromium/src/WebWorkerBase.cpp:134 > + static PassRefPtr<AllowFileSystemMainThreadBridge> create(WebWorkerBase* worker, const WTF::String& mode, WebCommonWorkerClient* commonClient, WebFrame* frame) frame not needed here either
John Abd-El-Malek
Comment 5 2011-05-12 09:07:23 PDT
John Abd-El-Malek
Comment 6 2011-05-12 09:08:50 PDT
(In reply to comment #4) > (From update of attachment 93233 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93233&action=review > > > Source/WebKit/chromium/public/WebCommonWorkerClient.h:90 > > + virtual bool allowFileSystem(WebFrame*) > > The WebFrame param does not belong in this interface. I know it's in the allowDatabase() method, but it doesn't belong there either. The origin of the worker can be determined w/o that parameter just fine since there's a 'client' per worker. > > > Source/WebKit/chromium/src/WebWorkerBase.cpp:134 > > + static PassRefPtr<AllowFileSystemMainThreadBridge> create(WebWorkerBase* worker, const WTF::String& mode, WebCommonWorkerClient* commonClient, WebFrame* frame) > > frame not needed here either Done for both. I had done it like that to be consistent with allowDatabase, but this is fine as well since that's the same url as openFileSystem uses.
James Robinson
Comment 7 2011-05-12 10:15:44 PDT
Comment on attachment 93292 [details] Patch R=me
John Abd-El-Malek
Comment 8 2011-05-12 10:34:25 PDT
Michael Nordman
Comment 9 2011-05-12 11:14:07 PDT
Comment on attachment 93292 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93292&action=review > Source/WebKit/chromium/src/WebWorkerClientImpl.h:102 > + virtual bool allowFileSystem(WebFrame*) nix WebFrame here too... this wants to be overriding the WebWorkerClient method, right?
John Abd-El-Malek
Comment 10 2011-05-12 11:23:36 PDT
(In reply to comment #9) > (From update of attachment 93292 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93292&action=review > > > Source/WebKit/chromium/src/WebWorkerClientImpl.h:102 > > + virtual bool allowFileSystem(WebFrame*) > > nix WebFrame here too... this wants to be overriding the WebWorkerClient method, right? oops, that was an oversight, will commit in a followup.
WebKit Review Bot
Comment 11 2011-05-12 11:50:30 PDT
http://trac.webkit.org/changeset/86356 might have broken SnowLeopard Intel Release (WebKit2 Tests) The following tests are not passing: fast/lists/inlineBoxWrapperNullCheck.html
Note You need to log in before you can comment on or make changes to this bug.