Bug 58456

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.