Bug 38228 - [chromium] Add WebFileSystem interface and hook up with all FileSystem methods
Summary: [chromium] Add WebFileSystem interface and hook up with all FileSystem methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jian Li
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-27 16:52 PDT by Jian Li
Modified: 2010-04-30 14:00 PDT (History)
3 users (show)

See Also:


Attachments
Proposed Patch (13.51 KB, patch)
2010-04-27 16:55 PDT, Jian Li
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (13.52 KB, patch)
2010-04-27 16:58 PDT, Jian Li
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (6.24 KB, patch)
2010-04-28 18:32 PDT, Jian Li
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (6.04 KB, patch)
2010-04-29 11:06 PDT, Jian Li
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (9.32 KB, patch)
2010-04-29 13:11 PDT, Jian Li
fishd: review-
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (13.63 KB, application/octet-stream)
2010-04-29 17:59 PDT, Jian Li
jianli: commit-queue-
Details
Proposed Patch (13.63 KB, patch)
2010-04-29 18:00 PDT, Jian Li
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (13.10 KB, patch)
2010-04-29 18:19 PDT, Jian Li
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (13.01 KB, patch)
2010-04-29 18:40 PDT, Jian Li
fishd: review+
jianli: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jian Li 2010-04-27 16:52:31 PDT
Add WebFileSystem interface and hook up with all FileSystem methods
Comment 1 Jian Li 2010-04-27 16:55:05 PDT
Created attachment 54474 [details]
Proposed Patch
Comment 2 Jian Li 2010-04-27 16:58:34 PDT
Created attachment 54475 [details]
Proposed Patch

Fixed style errors.
Comment 3 Jian Li 2010-04-28 18:32:52 PDT
Created attachment 54655 [details]
Proposed Patch

Updated the patch to keep existing file system methods to call WebKitClient methods, instead of new WebFileSystem methods.
Comment 4 WebKit Review Bot 2010-04-28 21:22:57 PDT
Attachment 54655 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1827167
Comment 5 Jian Li 2010-04-29 11:06:30 PDT
Created attachment 54720 [details]
Proposed Patch

Fixed chromium bot error by leaving out the calls of WebFileSystem methods.
Comment 6 WebKit Review Bot 2010-04-29 12:09:10 PDT
Attachment 54720 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1849200
Comment 7 Jian Li 2010-04-29 13:11:35 PDT
Created attachment 54730 [details]
Proposed Patch

Added the new file not included last time.
Comment 8 Darin Fisher (:fishd, Google) 2010-04-29 13:33:34 PDT
Comment on attachment 54730 [details]
Proposed Patch

WebKit/chromium/public/WebFileSystem.h:61
 +      virtual void closeFile(FileHandle&) = 0;
so closeFile will reset the FileHandle to a special invalid value, but there does not appear to be any method here to initialize a FileHandle to hold the invalid value.  it would be nice if closeFile had some documentation describing this subtle behavior.

WebKit/chromium/public/WebFileSystem.h:49
 +      virtual bool fileExists(const WebString& path) = 0;
please provide default implementations for all of these methods.  that way you can implement ChromiumBridge methods in terms of these without having to land something on the Chromium side.  in general, any method implemented by the embedder should have a default implementation in the WebKit API.

WebKit/chromium/src/ChromiumBridge.cpp:328
 +      return webKitClient()->filePathToURL(path);
you can modify all of the existing file-related ChromiumBridge methods to first null-test fileSystem().  if that is null, then fallback to the old methods.
Comment 9 Jian Li 2010-04-29 17:59:25 PDT
Created attachment 54763 [details]
Proposed Patch

All fixed.
Comment 10 Jian Li 2010-04-29 18:00:28 PDT
Created attachment 54764 [details]
Proposed Patch

All fixed. Also checked patch option which I forgot to check last time.
Comment 11 WebKit Review Bot 2010-04-29 18:01:30 PDT
Attachment 54764 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/ChromiumBridge.cpp:274:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebKit/chromium/src/ChromiumBridge.cpp:282:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebKit/chromium/src/ChromiumBridge.cpp:290:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebKit/chromium/src/ChromiumBridge.cpp:298:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebKit/chromium/src/ChromiumBridge.cpp:320:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebKit/chromium/src/ChromiumBridge.cpp:328:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebKit/chromium/src/ChromiumBridge.cpp:336:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebKit/chromium/src/ChromiumBridge.cpp:344:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebKit/chromium/src/ChromiumBridge.cpp:352:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebKit/chromium/src/ChromiumBridge.cpp:360:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 10 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Jian Li 2010-04-29 18:19:54 PDT
Created attachment 54767 [details]
Proposed Patch

Fixed style errors.
Comment 13 WebKit Review Bot 2010-04-29 18:28:07 PDT
Attachment 54763 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1857203
Comment 14 Jian Li 2010-04-29 18:40:31 PDT
Created attachment 54770 [details]
Proposed Patch

Fixed chromium build errors.
Comment 15 WebKit Review Bot 2010-04-29 18:55:24 PDT
Attachment 54767 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1857204
Comment 16 Darin Fisher (:fishd, Google) 2010-04-29 20:32:19 PDT
Comment on attachment 54770 [details]
Proposed Patch

WebKit/chromium/public/WebFileSystem.h:60
 +      // Should set the FileHandle to a inavalid value if the file is closed successfully.
inavalid -> invalid

R=me
Comment 17 Jian Li 2010-04-30 14:00:06 PDT
Fixed and landed as http://trac.webkit.org/changeset/58599.