Bug 60683 – [chromium] Make openFileSystem check for permission first

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[John Abd-El-Malek]

[chromium] Make openFileSystem check for permission first

[John Abd-El-Malek]

Created attachment 93232 [details]
Patch

[John Abd-El-Malek]

Created attachment 93233 [details]
Patch

[John Abd-El-Malek]

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 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]

Created attachment 93292 [details]
Patch

[John Abd-El-Malek]

(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 on attachment 93292 [details]
Patch

R=me

[John Abd-El-Malek]

Committed r86356: <http://trac.webkit.org/changeset/86356>

[Michael Nordman]

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]

(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]

http://trac.webkit.org/changeset/86356 might have broken SnowLeopard Intel Release (WebKit2 Tests)
The following tests are not passing:
fast/lists/inlineBoxWrapperNullCheck.html