Add chromium-side of AsyncFileSystem implementation
Created attachment 65134 [details] Patch
Created attachment 65202 [details] Patch
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).
Created attachment 65209 [details] Patch
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.
Attachment 65209 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3731548
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
Comment on attachment 65209 [details] Patch WebKit/chromium/src/AsyncFileSystemChromium.h:46 + class SecurityOrigin; super minor nit: is this forward decl needed?
Created attachment 65483 [details] Patch Uploading a patch one more time to include changes made in issue 44433
Created attachment 65490 [details] Patch Removed SecurityOrigin.h includsion from AsyncFileSystemChromium.cpp
(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.
Attachment 65490 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3738626
Created attachment 65527 [details] Patch
Attachment 65527 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3726669
Comment on attachment 65527 [details] Patch re-applying fishd's r+.
Committed r66280: <http://trac.webkit.org/changeset/66280>
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