Bug 58456

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 Flags
Added factory function for creating File- and DirectoryEntry objects
none
Added factory function for creating File- and DirectoryEntry objects from Chromium side
none
Added 'external' file system enum, few Chromium factory functions for File API objects.
none
Added 'external' file system enum, few Chromium factory functions for File API objects.
fishd: review+
Added 'external' file system enum, few Chromium factory functions for File API objects
fishd: review+, fishd: commit-queue-
Added 'external' file system enum, few Chromium factory functions for File API objects none

Description Zelidrag Hornung 2011-04-13 10:47:20 PDT
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.
Comment 1 Zelidrag Hornung 2011-04-13 11:47:56 PDT
Created attachment 89418 [details]
Added factory function for creating File- and DirectoryEntry objects

Added factory function for creating File- and DirectoryEntry objects.
Comment 2 WebKit Review Bot 2011-04-13 11:50:17 PDT
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.
Comment 3 Zelidrag Hornung 2011-04-13 12:27:52 PDT
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 4 David Levin 2011-04-13 14:11:42 PDT
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?
Comment 5 David Levin 2011-04-13 14:12:15 PDT
Needs to go by Darin as well since you touch the chromium public api.
Comment 6 Zelidrag Hornung 2011-04-13 14:15:53 PDT
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 7 Darin Fisher (:fishd, Google) 2011-04-13 14:21:20 PDT
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?
Comment 8 Zelidrag Hornung 2011-04-13 17:30:54 PDT
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.
Comment 9 WebKit Review Bot 2011-04-13 17:33:00 PDT
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.
Comment 10 Zelidrag Hornung 2011-04-13 17:37:31 PDT
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 11 Darin Fisher (:fishd, Google) 2011-04-13 21:38:36 PDT
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!
Comment 12 Zelidrag Hornung 2011-04-14 15:05:00 PDT
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
Comment 13 Zelidrag Hornung 2011-04-14 15:08:48 PDT
http://codereview.chromium.org/6852031 should prevent build breaks on Chromium side when this one lands
Comment 14 Darin Fisher (:fishd, Google) 2011-04-14 15:23:45 PDT
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.
Comment 15 Zelidrag Hornung 2011-04-14 15:28:50 PDT
Created attachment 89671 [details]
Added 'external' file system enum, few Chromium factory functions for File API objects

nit fixes
Comment 16 Darin Fisher (:fishd, Google) 2011-04-14 15:51:13 PDT
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 17 WebKit Commit Bot 2011-04-15 07:35:51 PDT
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>
Comment 18 WebKit Commit Bot 2011-04-15 07:35:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 WebKit Commit Bot 2011-04-15 08:22:51 PDT
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.