Summary: | [chromium] Add chromium-side of AsyncFileSystem implementation | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kinuko Yasuda <kinuko> | ||||||||||||||
Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, dumi, ericu, eric, fishd, michaeln, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||
Bug Depends on: | 44433 | ||||||||||||||||
Bug Blocks: | 42903 | ||||||||||||||||
Attachments: |
|
Description
Kinuko Yasuda
2010-08-23 09:46:29 PDT
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 |