Bug 43134

Summary: Add idl and mock implementation for HTML5 FileSystem API
Product: WebKit Reporter: Kinuko Yasuda <kinuko>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, dumi, eric, fishd, michaeln, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 43130    
Bug Blocks: 42903    
Attachments:
Description Flags
Patch (depends on 43130)
none
Patch dumi: review+, dumi: commit-queue-

Description Kinuko Yasuda 2010-07-28 12:06:12 PDT
Add idl and mock classes for basic FileSystem API interfaces
As defined in: http://dev.w3.org/2009/dap/file-system/file-dir-sys.html
Comment 1 Kinuko Yasuda 2010-07-28 12:49:50 PDT
Created attachment 62864 [details]
Patch (depends on 43130)
Comment 2 WebKit Review Bot 2010-07-28 15:00:02 PDT
Attachment 62864 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3559627
Comment 3 Dumitru Daniliuc 2010-07-28 17:55:01 PDT
Comment on attachment 62864 [details]
Patch (depends on 43130)

i think you might need to modify JSValueToNative in CodeGeneratorV8.pm to make the patch build in chromium. for [Callback] parameters, i think you would want to return "$value".

WebCore/bindings/scripts/CodeGenerator.pm:363
 +      $ret =~ s/^eXCLUSIVE/exclusiveFlag/ if $ret =~ /^eXCLUSIVE$/;
i think isCreate() or isCreateFlag() would be a better name for a boolean getter. same for exclusiveFlag(). also, you should add a test for these cases to WebCore/bindings/scripts/test/TestObj.idl, and change the expected outputs in WebCore/bindings/scripts/test/{JS|V8|...}/. Please make sure WebKitTools/Scripts/run-bindings-tests passes.

WebCore/storage/DOMFileSystem.cpp:35
 +  #include "DOMFileSystem.h"
i believe the common pattern is:

#include "config.h"
#include "DOMFileSystem.h"

#if ENABLE(FILE_SYSTEM)

WebCore/storage/DOMFileSystem.cpp:38
 +  #include "EntryCallback.h"
looks like this include is not yet needed.

WebCore/storage/DOMFileSystem.cpp:39
 +  #include "ErrorCallback.h"
ditto.

WebCore/storage/DOMFileSystem.h:44
 +  class ErrorCallback;
these 2 classes don't need to be forward-declared just yet.

WebCore/storage/DOMFileSystem.h:45
 +  class ScriptExecutionContext;
ditto.

WebCore/storage/Entry.cpp:34
 +  #include "Entry.h"
same comment about moving this outside #if ENABLE(FILE_SYSTEM)

WebCore/storage/Entry.cpp:50
 +          m_name = fullPath.substring(index);
should there be a "else m_name = fullPath;" here?

WebCore/storage/Entry.h:34
 +  #include "DOMFileSystem.h"
either remove this include, or the forward-declaration below.

WebCore/storage/ErrorCallback.h:43
 +  class ErrorCallback : public ThreadSafeShared<ErrorCallback> {
this callback is ThreadSafeShared, and all other callbacks are RefCounted. is this intended?

WebCore/storage/Flags.h:50
 +      void setEXCLUSIVE(bool exclusive) { m_exclusive = exclusive; }
you should modify WK_ucfirst in CodeGenerator.pm the same way you modified WK_lcfirst, and convert setCREATE() and setEXCLUSIVE() to setCreate() and setExclusive().

WebCore/storage/Metadata.h:53
 +      double m_modificationTime;
empty line before this field.
Comment 4 Kinuko Yasuda 2010-07-28 21:04:34 PDT
Created attachment 62914 [details]
Patch
Comment 5 Kinuko Yasuda 2010-07-28 21:19:27 PDT
Thanks for your careful review!  I fixed the issues you pointed out.

(In reply to comment #3)
> (From update of attachment 62864 [details])
> i think you might need to modify JSValueToNative in CodeGeneratorV8.pm to make the patch build in chromium. for [Callback] parameters, i think you would want to return "$value".

I have another proposing patch (issue 43130 - I've cc'ed you) that hopefully fixes this.  (I'll look for alternative solution if it's not the right solution.)

> WebCore/bindings/scripts/CodeGenerator.pm:363
>  +      $ret =~ s/^eXCLUSIVE/exclusiveFlag/ if $ret =~ /^eXCLUSIVE$/;
> i think isCreate() or isCreateFlag() would be a better name for a boolean getter. same for exclusiveFlag(). also, you should add a test for these cases to WebCore/bindings/scripts/test/TestObj.idl, and change the expected outputs in WebCore/bindings/scripts/test/{JS|V8|...}/. Please make sure WebKitTools/Scripts/run-bindings-tests passes.
> 
> WebCore/storage/DOMFileSystem.cpp:35
>  +  #include "DOMFileSystem.h"
> i believe the common pattern is:
> 
> #include "config.h"
> #include "DOMFileSystem.h"
> 
> #if ENABLE(FILE_SYSTEM)
> 
> WebCore/storage/DOMFileSystem.cpp:38
>  +  #include "EntryCallback.h"
> looks like this include is not yet needed.
> 
> WebCore/storage/DOMFileSystem.cpp:39
>  +  #include "ErrorCallback.h"
> ditto.
> 
> WebCore/storage/DOMFileSystem.h:44
>  +  class ErrorCallback;
> these 2 classes don't need to be forward-declared just yet.
> 
> WebCore/storage/DOMFileSystem.h:45
>  +  class ScriptExecutionContext;
> ditto.
> 
> WebCore/storage/Entry.cpp:34
>  +  #include "Entry.h"
> same comment about moving this outside #if ENABLE(FILE_SYSTEM)
> 
> WebCore/storage/Entry.cpp:50
>  +          m_name = fullPath.substring(index);
> should there be a "else m_name = fullPath;" here?
> 
> WebCore/storage/Entry.h:34
>  +  #include "DOMFileSystem.h"
> either remove this include, or the forward-declaration below.
> 
> WebCore/storage/ErrorCallback.h:43
>  +  class ErrorCallback : public ThreadSafeShared<ErrorCallback> {
> this callback is ThreadSafeShared, and all other callbacks are RefCounted. is this intended?

Good catch... it was a mistake.

> WebCore/storage/Flags.h:50
>  +      void setEXCLUSIVE(bool exclusive) { m_exclusive = exclusive; }
> you should modify WK_ucfirst in CodeGenerator.pm the same way you modified WK_lcfirst, and convert setCREATE() and setEXCLUSIVE() to setCreate() and setExclusive().
> 
> WebCore/storage/Metadata.h:53
>  +      double m_modificationTime;
> empty line before this field.
Comment 6 Dumitru Daniliuc 2010-07-29 01:36:32 PDT
Comment on attachment 62914 [details]
Patch

r=me, but please address the comments below, and make sure you can build webkit and chromium before you submit the patch.

WebCore/bindings/scripts/CodeGenerator.pm:348
 +      $ret =~ s/^CREATE/Create/ if $ret =~ /^CREATE$/;
now that i look at this code, i think we can do a little better: instead of adding special cases for CREATE and EXCLUSIVE, we can do the same trick for all uppercase strings.

basically, i think we should just add 'return ucfirst(lc($param)) if $param eq uc($param);' at the beginning of this function.

sorry i didn't think about this earlier.

WebCore/bindings/scripts/CodeGenerator.pm:367
 +      $ret =~ s/^cREATE/isCreate/ if $ret =~ /^cREATE$/;
same thing here, i think it's ok to assume for now that all uppercase attributes are boolean: return "is" . ucfirst(lc($param)) if $param eq uc($param);

WebCore/bindings/scripts/test/TestObj.idl:48
 +          attribute bool                     CREATE;
s/bool/boolean/. otherwise, we'll get lots of non-sense includes like "JSbool.h" and "V8bool.h".
Comment 7 Kinuko Yasuda 2010-07-29 15:47:54 PDT
Thanks for reviewing.

(In reply to comment #6)
> (From update of attachment 62914 [details])
> r=me, but please address the comments below, and make sure you can build webkit and chromium before you submit the patch.
> 
> WebCore/bindings/scripts/CodeGenerator.pm:348
>  +      $ret =~ s/^CREATE/Create/ if $ret =~ /^CREATE$/;
> now that i look at this code, i think we can do a little better: instead of adding special cases for CREATE and EXCLUSIVE, we can do the same trick for all uppercase strings.
> basically, i think we should just add 'return ucfirst(lc($param)) if $param eq uc($param);' at the beginning of this function.

Yeah it'll be cleaner - I didn't do so out of fear to break something, but it looks ok as you say (grepping confirmed that).
I'll change like that.  I'll also fix bool in idl and make sure they can be built on both platforms before submitting.
Comment 8 Kinuko Yasuda 2010-07-31 01:44:43 PDT
Committed r64414: <http://trac.webkit.org/changeset/64414>
Comment 9 WebKit Review Bot 2010-07-31 02:21:32 PDT
http://trac.webkit.org/changeset/64414 might have broken Chromium Mac Release