Bug 44434

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:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Bug Depends on:

44433

Bug Blocks:

42903

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	dumi: review+

Kinuko Yasuda

Reported 2010-08-23 09:46:29 PDT

Add chromium-side of AsyncFileSystem implementation

Attachments
Patch (11.54 KB, patch) 2010-08-23 09:50 PDT, Kinuko Yasuda	no flags	Details Formatted Diff Diff
Patch (14.89 KB, patch) 2010-08-23 18:59 PDT, Kinuko Yasuda	no flags	Details Formatted Diff Diff
Patch (15.81 KB, patch) 2010-08-23 22:24 PDT, Kinuko Yasuda	no flags	Details Formatted Diff Diff
Patch (16.48 KB, patch) 2010-08-25 15:40 PDT, Kinuko Yasuda	no flags	Details Formatted Diff Diff
Patch (16.42 KB, patch) 2010-08-25 15:55 PDT, Kinuko Yasuda	no flags	Details Formatted Diff Diff
Patch (16.51 KB, patch) 2010-08-25 22:49 PDT, Kinuko Yasuda	dumi: review+	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Kinuko Yasuda

Comment 1 2010-08-23 09:50:01 PDT

Created attachment 65134 [details] Patch

Kinuko Yasuda

Comment 2 2010-08-23 18:59:47 PDT

Created attachment 65202 [details] Patch

Michael Nordman

Comment 3 2010-08-23 19:52:12 PDT

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

Comment 4 2010-08-23 22:24:13 PDT

Created attachment 65209 [details] Patch

Kinuko Yasuda

Comment 5 2010-08-23 22:30:24 PDT

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

Comment 6 2010-08-23 23:35:27 PDT

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

Darin Fisher (:fishd, Google)

Comment 7 2010-08-24 21:48:48 PDT

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 8 2010-08-25 13:40:04 PDT

Comment on attachment 65209 [details] Patch WebKit/chromium/src/AsyncFileSystemChromium.h:46 + class SecurityOrigin; super minor nit: is this forward decl needed?

Kinuko Yasuda

Comment 9 2010-08-25 15:40:18 PDT

Created attachment 65483 [details] Patch Uploading a patch one more time to include changes made in issue 44433

Kinuko Yasuda

Comment 10 2010-08-25 15:55:34 PDT

Created attachment 65490 [details] Patch Removed SecurityOrigin.h includsion from AsyncFileSystemChromium.cpp

Kinuko Yasuda

Comment 11 2010-08-25 15:55:54 PDT

(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

Comment 12 2010-08-25 18:34:26 PDT

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

Kinuko Yasuda

Comment 13 2010-08-25 22:49:18 PDT

Created attachment 65527 [details] Patch

WebKit Review Bot

Comment 14 2010-08-25 23:22:43 PDT

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

Dumitru Daniliuc

Comment 15 2010-08-27 11:40:12 PDT

Comment on attachment 65527 [details] Patch re-applying fishd's r+.