Bug 58456 – Add external file type enum and expose factory function for creating FileEntry and DirectoryEntry instances from Chromium side

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Zelidrag Hornung]

We need to expose a factory function for creating FileEntry and DirectoryEntry instances from Chromium side. These object can now be created as payload in one of Chromium extension events. (see details at http://codereview.chromium.org/6749021/).

The patch is coming in few min.

[Zelidrag Hornung]

Created attachment 89418 [details]
Added factory function for creating File- and DirectoryEntry objects

Added factory function for creating File- and DirectoryEntry objects.

[WebKit Review Bot]

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.

[Zelidrag Hornung]

Created attachment 89435 [details]
Added factory function for creating File- and DirectoryEntry objects from Chromium side

review comments from the previous CR are addresses

[David Levin]

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?

[David Levin]

Needs to go by Darin as well since you touch the chromium public api.

[Zelidrag Hornung]

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.

[Darin Fisher (:fishd, Google)]

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?

[Zelidrag Hornung]

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.

[WebKit Review Bot]

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.

[Zelidrag Hornung]

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

[Darin Fisher (:fishd, Google)]

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!

[Zelidrag Hornung]

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

[Zelidrag Hornung]

http://codereview.chromium.org/6852031 should prevent build breaks on Chromium side when this one lands

[Darin Fisher (:fishd, Google)]

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.

[Zelidrag Hornung]

Created attachment 89671 [details]
Added 'external' file system enum, few Chromium factory functions for File API objects

nit fixes

[Darin Fisher (:fishd, Google)]

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)

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 89671 [details]:

java/lc3/JSObject/ToObject-001.html bug 53091 (author: [e-mail])
The commit-queue is continuing to process your patch.