Summary: | Add external file type enum and expose factory function for creating FileEntry and DirectoryEntry instances from Chromium side | ||
---|---|---|---|
Product: | WebKit | Reporter: | Zelidrag Hornung <zelidrag> |
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | commit-queue, ericu, fishd, levin, webkit.review.bot |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | Other | ||
OS: | Other | ||
Attachments: |
Description
Zelidrag Hornung
2011-04-13 10:47:20 PDT
Created attachment 89418 [details]
Added factory function for creating File- and DirectoryEntry objects
Added factory function for creating File- and DirectoryEntry objects.
Attachment 89418 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/src/WebFrameImpl.cpp:871: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Source/WebKit/chromium/src/WebFrameImpl.cpp:872: 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: 2 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style. Created attachment 89435 [details]
Added factory function for creating File- and DirectoryEntry objects from Chromium side
review comments from the previous CR are addresses
Comment on attachment 89435 [details] Added factory function for creating File- and DirectoryEntry objects from Chromium side View in context: https://bugs.webkit.org/attachment.cgi?id=89435&action=review One question. Other than that it seems fine. > Source/WebKit/chromium/src/WebFrameImpl.cpp:871 > + RefPtr<DOMFileSystemBase> fileSystem = DOMFileSystem::create(frame()->document(), fileSystemName, AsyncFileSystemChromium::create(static_cast<AsyncFileSystem::Type>(fileSystemType), fileSystemPath)); Where is the error checking for invalid values of fileSystemType? Needs to go by Darin as well since you touch the chromium public api. The error checking is exactly what I needed to avoid with this method (and one above) since it method will work for filesystem types that are not part of HTML5 standard. Exposed parts of the local file system in ChromeOS is such example. Comment on attachment 89435 [details] Added factory function for creating File- and DirectoryEntry objects from Chromium side View in context: https://bugs.webkit.org/attachment.cgi?id=89435&action=review > Source/WebKit/chromium/public/WebFrame.h:266 > virtual v8::Handle<v8::Value> createFileSystem(int type, Wow, this createFileSystem API is really unusual. Why do we have a method like this returning a V8 object? What are the allowed values for the int type? Peeking at the implementation, there should have been an enum defined here. I'm confused about why we need either of these APIs. Can't you do roughly the same thing with executeScriptAndReturnValue in both of these cases? Created attachment 89502 [details]
Added 'external' file system enum, few Chromium factory functions for File API objects.
As agreed with Darin, I have added 'external' file system enum to match Pepper implementation ids for file system types.
Attachment 89502 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/chromium/public/WebFrame.h:268: The parameter name "type" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebKit/chromium/public/WebFrame.h:272: The parameter name "type" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/fileapi/EntryBase.cpp:68: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4]
Source/WebKit/chromium/src/WebFrameImpl.h:112: The parameter name "type" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebKit/chromium/src/WebFrameImpl.h:115: The parameter name "type" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 5 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 89503 [details]
Added 'external' file system enum, few Chromium factory functions for File API objects.
added 'external' file system enum, few Chromium factory functions for File API objects
fixed style issues from the previous patch
Comment on attachment 89503 [details]
Added 'external' file system enum, few Chromium factory functions for File API objects.
R=me, but I also want Kinuko to review this before we commit it. Thanks!
Created attachment 89662 [details]
Added 'external' file system enum, few Chromium factory functions for File API objects
added WEB_FILE_SYSTEM_TYPE_EXTERNAL define that should let me check in changed on Chromium side that will prevent build breaks when this lands
http://codereview.chromium.org/6852031 should prevent build breaks on Chromium side when this one lands Comment on attachment 89662 [details] Added 'external' file system enum, few Chromium factory functions for File API objects View in context: https://bugs.webkit.org/attachment.cgi?id=89662&action=review R=me, CQ- > Source/WebKit/chromium/public/WebFileSystem.h:43 > +#define WEB_FILE_SYSTEM_TYPE_EXTERNAL; nit: no trailing ";" please add a FIXME about removing this #define once the Chromium side has caught up. Created attachment 89671 [details]
Added 'external' file system enum, few Chromium factory functions for File API objects
nit fixes
Comment on attachment 89671 [details]
Added 'external' file system enum, few Chromium factory functions for File API objects
Normally, FIXME comments in WebKit are not suffixed with (username)
Comment on attachment 89671 [details] Added 'external' file system enum, few Chromium factory functions for File API objects Clearing flags on attachment: 89671 Committed r83972: <http://trac.webkit.org/changeset/83972> All reviewed patches have been landed. Closing bug. The commit-queue encountered the following flaky tests while processing attachment 89671 [details]: java/lc3/JSObject/ToObject-001.html bug 53091 (author: ap@webkit.org) The commit-queue is continuing to process your patch. |