Bug 44434 - [chromium] Add chromium-side of AsyncFileSystem implementation
Summary: [chromium] Add chromium-side of AsyncFileSystem implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 44433
Blocks: 42903
  Show dependency treegraph
 
Reported: 2010-08-23 09:46 PDT by Kinuko Yasuda
Modified: 2010-08-27 17:59 PDT (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2010-08-23 09:46:29 PDT
Add chromium-side of AsyncFileSystem implementation
Comment 1 Kinuko Yasuda 2010-08-23 09:50:01 PDT
Created attachment 65134 [details]
Patch
Comment 2 Kinuko Yasuda 2010-08-23 18:59:47 PDT
Created attachment 65202 [details]
Patch
Comment 3 Michael Nordman 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).
Comment 4 Kinuko Yasuda 2010-08-23 22:24:13 PDT
Created attachment 65209 [details]
Patch
Comment 5 Kinuko Yasuda 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.
Comment 6 WebKit Review Bot 2010-08-23 23:35:27 PDT
Attachment 65209 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3731548
Comment 7 Darin Fisher (:fishd, Google) 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
Comment 8 Michael Nordman 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?
Comment 9 Kinuko Yasuda 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
Comment 10 Kinuko Yasuda 2010-08-25 15:55:34 PDT
Created attachment 65490 [details]
Patch

Removed SecurityOrigin.h includsion from AsyncFileSystemChromium.cpp
Comment 11 Kinuko Yasuda 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.
Comment 12 WebKit Review Bot 2010-08-25 18:34:26 PDT
Attachment 65490 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3738626
Comment 13 Kinuko Yasuda 2010-08-25 22:49:18 PDT
Created attachment 65527 [details]
Patch
Comment 14 WebKit Review Bot 2010-08-25 23:22:43 PDT
Attachment 65527 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3726669
Comment 15 Dumitru Daniliuc 2010-08-27 11:40:12 PDT
Comment on attachment 65527 [details]
Patch

re-applying fishd's r+.
Comment 16 Kinuko Yasuda 2010-08-27 16:59:28 PDT
Committed r66280: <http://trac.webkit.org/changeset/66280>
Comment 17 WebKit Review Bot 2010-08-27 17:59:52 PDT
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