Bug 44434 – [chromium] Add chromium-side of AsyncFileSystem implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kinuko Yasuda]

Add chromium-side of AsyncFileSystem implementation

[Kinuko Yasuda]

Created attachment 65134 [details]
Patch

[Kinuko Yasuda]

Created attachment 65202 [details]
Patch

[Michael Nordman]

Comment on attachment 65202 [details]
Patch

Some drive by comments.

WebKit/chromium/src/AsyncFileSystemChromium.cpp:55
 +  static PassOwnPtr<AsyncFileSystem> createPlatformAsyncFileSystem()
I don't follow this static function definition, its not called in this module so what purpose does it serve?

WebKit/chromium/src/LocalFileSystemChromium.cpp:70
 +  COMPILE_ASSERT_MATCHING_ENUM(Persistent);
Most of these are in AssertMatchingEnums.cpp, unless there's a compelling reason to have these separated out, I think these should also be in that .cpp file.

WebKit/chromium/src/AsyncFileSystemChromium.cpp:64
 +  {
maybe ASSERT(m_webFileSystem)

WebKit/chromium/src/LocalFileSystemChromium.cpp:60
 +          WebFrameImpl* webFrame = WebFrameImpl::fromFrame(document->frame());
Is there any chance script can be executing in a document that has been detached from its frame? IDK?

WebKit/chromium/src/AsyncFileSystemChromium.h:36
 +  namespace WebKit { class WebFileSystem; }
I would expect in webkit style that the forward decl would go after the includes (not positive about that).

[Kinuko Yasuda]

Created attachment 65209 [details]
Patch

[Kinuko Yasuda]

Thanks!

(In reply to comment #3)
> (From update of attachment 65202 [details])
> Some drive by comments.
> 
> WebKit/chromium/src/AsyncFileSystemChromium.cpp:55
>  +  static PassOwnPtr<AsyncFileSystem> createPlatformAsyncFileSystem()
> I don't follow this static function definition, its not called in this module so what purpose does it serve?

Removed.

> WebKit/chromium/src/LocalFileSystemChromium.cpp:70
>  +  COMPILE_ASSERT_MATCHING_ENUM(Persistent);
> Most of these are in AssertMatchingEnums.cpp, unless there's a compelling reason to have these separated out, I think these should also be in that .cpp file.

I didn't know of the file... fixed.

> WebKit/chromium/src/AsyncFileSystemChromium.cpp:64
>  +  {
> maybe ASSERT(m_webFileSystem)

Added.

> WebKit/chromium/src/LocalFileSystemChromium.cpp:60
>  +          WebFrameImpl* webFrame = WebFrameImpl::fromFrame(document->frame());
> Is there any chance script can be executing in a document that has been detached from its frame? IDK?

In that case openFileSystem's implementation must call errorCallback.

> WebKit/chromium/src/AsyncFileSystemChromium.h:36
>  +  namespace WebKit { class WebFileSystem; }
> I would expect in webkit style that the forward decl would go after the includes (not positive about that).

Fixed.

[WebKit Review Bot]

Attachment 65209 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3731548

[Darin Fisher (:fishd, Google)]

Comment on attachment 65209 [details]
Patch

WebKit/chromium/src/LocalFileSystemChromium.cpp:47
 +  using WebKit::WebFileSystemCallbacksImpl;
nit: since you are in WebKit, I would just put a "using namespace WebKit" here, and
then drop the WebKit:: prefixes down below.


otherwise, R=me

[Michael Nordman]

Comment on attachment 65209 [details]
Patch

WebKit/chromium/src/AsyncFileSystemChromium.h:46
 +  class SecurityOrigin;
super minor nit: is this forward decl needed?

[Kinuko Yasuda]

Created attachment 65483 [details]
Patch

Uploading a patch one more time to include changes made in issue 44433

[Kinuko Yasuda]

Created attachment 65490 [details]
Patch

Removed SecurityOrigin.h includsion from AsyncFileSystemChromium.cpp

[Kinuko Yasuda]

(In reply to comment #7)
> (From update of attachment 65209 [details])
> WebKit/chromium/src/LocalFileSystemChromium.cpp:47
>  +  using WebKit::WebFileSystemCallbacksImpl;
> nit: since you are in WebKit, I would just put a "using namespace WebKit" here, and
> then drop the WebKit:: prefixes down below.

Fixed.

(In reply to comment #8)
> (From update of attachment 65209 [details])
> WebKit/chromium/src/AsyncFileSystemChromium.h:46
>  +  class SecurityOrigin;
> super minor nit: is this forward decl needed?

Removed.

[WebKit Review Bot]

Attachment 65490 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3738626

[Kinuko Yasuda]

Created attachment 65527 [details]
Patch

[WebKit Review Bot]

Attachment 65527 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3726669

[Dumitru Daniliuc]

Comment on attachment 65527 [details]
Patch

re-applying fishd's r+.

[Kinuko Yasuda]

Committed r66280: <http://trac.webkit.org/changeset/66280>

[WebKit Review Bot]

http://trac.webkit.org/changeset/66280 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/66280
http://trac.webkit.org/changeset/66278
http://trac.webkit.org/changeset/66279