Bug 91187 – [EFL] Skeleton code of File System API.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Dongwoo Joshua Im (dwim)]

EFL port cannot be built with enabling the ENABLE_FILE_SYSTEM flag.

I'm preparing the patch now.

[Dongwoo Joshua Im (dwim)]

Created attachment 152147 [details]
Patch

[Dongwoo Joshua Im (dwim)]

Created attachment 152150 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 152150 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152150&action=review

> Source/WebCore/ChangeLog:3
> +        [EFL] File system api build is broken

I think this bug title is not proper.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #3)
> (From update of attachment 152150 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152150&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        [EFL] File system api build is broken
> 
> I think this bug title is not proper.

I understand what you are saying.

Then, I'll start to implement this feature.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 152150 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=152150&action=review
> > 
> > > Source/WebCore/ChangeLog:3
> > > +        [EFL] File system api build is broken
> > 
> > I think this bug title is not proper.
> 
> I understand what you are saying.
> 
> Then, I'll start to implement this feature.

+ with proper name. ;)

[Dongwoo Joshua Im (dwim)]

Created attachment 166141 [details]
patch

This is a skeleton code of the File System API on EFL port.
Real implementation patches will follow this patch, in proper size to easy review.

[Chris Dumez]

Comment on attachment 166141 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166141&action=review

> Source/WebCore/ChangeLog:9
> +        Implementaion patches will be created followed by this.

"implementation"
"followed by this" -> "later" or "after this".

> Source/WebCore/PlatformEfl.cmake:332
> +IF (ENABLE_FILE_SYSTEM)

This condition here is not really needed since the file is already protected by #if ENABLE(FILE_SYSTEM)

> Source/WebCore/platform/efl/AsyncFileSystemEfl.cpp:42
> +void AsyncFileSystem::openFileSystem(const String& basePath, const String& storageIdentifier, bool, PassOwnPtr<AsyncFileSystemCallbacks> callbacks)

Isn't it generating a lot of warnings all these unused arguments?

[Dongwoo Joshua Im (dwim)]

(In reply to comment #7)
> (From update of attachment 166141 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166141&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Implementaion patches will be created followed by this.
> 
> "implementation"
> "followed by this" -> "later" or "after this".

Oh.. I will fix those.

> 
> > Source/WebCore/PlatformEfl.cmake:332
> > +IF (ENABLE_FILE_SYSTEM)
> 
> This condition here is not really needed since the file is already protected by #if ENABLE(FILE_SYSTEM)
> 

Referring CMakeLists.txt and PlatformEfl.cmake,
many files are only included when the flag is enabled, even though the files protect using the macro in itself.
I think that's because it could reduce the compilation time.

So, I prefer to keep this in my patch.

What do you think?


> > Source/WebCore/platform/efl/AsyncFileSystemEfl.cpp:42
> > +void AsyncFileSystem::openFileSystem(const String& basePath, const String& storageIdentifier, bool, PassOwnPtr<AsyncFileSystemCallbacks> callbacks)
> 
> Isn't it generating a lot of warnings all these unused arguments?

I've ignored these warnings because these will be disappeared by the following patches, and those warnings wouldn't be shown in usual case because FILE_SYSTEM is disabled currently.

But, for the quality of "this" patch, I will fix this.

[Gyuyoung Kim]

Comment on attachment 166141 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166141&action=review

>>> Source/WebCore/PlatformEfl.cmake:332
>>> +IF (ENABLE_FILE_SYSTEM)
>> 
>> This condition here is not really needed since the file is already protected by #if ENABLE(FILE_SYSTEM)
> 
> Referring CMakeLists.txt and PlatformEfl.cmake,
> many files are only included when the flag is enabled, even though the files protect using the macro in itself.
> I think that's because it could reduce the compilation time.
> 
> So, I prefer to keep this in my patch.
> 
> What do you think?

I prefer not to guard this in cmake file again. I don't wanna disturb cmake file using excessive ENABLE_ macro. According to my previous build time measurement, there is no build time increase.

> Source/WebCore/platform/efl/AsyncFileSystemEfl.cpp:2
> + * Copyright (C) 2012 Samsung Electronics

Is there any reason to use LGPL ?

>>> Source/WebCore/platform/efl/AsyncFileSystemEfl.cpp:42
>>> +void AsyncFileSystem::openFileSystem(const String& basePath, const String& storageIdentifier, bool, PassOwnPtr<AsyncFileSystemCallbacks> callbacks)
>> 
>> Isn't it generating a lot of warnings all these unused arguments?
> 
> I've ignored these warnings because these will be disappeared by the following patches, and those warnings wouldn't be shown in usual case because FILE_SYSTEM is disabled currently.
> 
> But, for the quality of "this" patch, I will fix this.

I think it would be good if you add variable name when you will use it.

> Source/WebCore/platform/efl/AsyncFileSystemEfl.h:18
> + *

Looks unneeded line.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #9)
> (From update of attachment 166141 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166141&action=review
> 
> >>> Source/WebCore/PlatformEfl.cmake:332
> >>> +IF (ENABLE_FILE_SYSTEM)
> >> 
> >> This condition here is not really needed since the file is already protected by #if ENABLE(FILE_SYSTEM)
> > 
> > Referring CMakeLists.txt and PlatformEfl.cmake,
> > many files are only included when the flag is enabled, even though the files protect using the macro in itself.
> > I think that's because it could reduce the compilation time.
> > 
> > So, I prefer to keep this in my patch.
> > 
> > What do you think?
> 
> I prefer not to guard this in cmake file again. I don't wanna disturb cmake file using excessive ENABLE_ macro. According to my previous build time measurement, there is no build time increase.
> 

Ok, I will follow your recommendation.

> > Source/WebCore/platform/efl/AsyncFileSystemEfl.cpp:2
> > + * Copyright (C) 2012 Samsung Electronics
> 
> Is there any reason to use LGPL ?
> 

No specific reason. ;)
I just didn't recognized the currently created files on EFL port are in the BSD license.
I will change this.

> >>> Source/WebCore/platform/efl/AsyncFileSystemEfl.cpp:42
> >>> +void AsyncFileSystem::openFileSystem(const String& basePath, const String& storageIdentifier, bool, PassOwnPtr<AsyncFileSystemCallbacks> callbacks)
> >> 
> >> Isn't it generating a lot of warnings all these unused arguments?
> > 
> > I've ignored these warnings because these will be disappeared by the following patches, and those warnings wouldn't be shown in usual case because FILE_SYSTEM is disabled currently.
> > 
> > But, for the quality of "this" patch, I will fix this.
> 
> I think it would be good if you add variable name when you will use it.

yes, I will.

> 
> > Source/WebCore/platform/efl/AsyncFileSystemEfl.h:18
> > + *
> 
> Looks unneeded line.

This line is removed due to the license change.

[Dongwoo Joshua Im (dwim)]

Created attachment 166172 [details]
patch

I've fixed the patch regarding the comments.

[Gyuyoung Kim]

Comment on attachment 166172 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166172&action=review

> Source/WebCore/Modules/filesystem/LocalFileSystem.cpp:80
> +#if !PLATFORM(EFL)

Why do you disable this functions on EFL port ?

[Dongwoo Joshua Im (dwim)]

(In reply to comment #12)
> (From update of attachment 166172 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166172&action=review
> 
> > Source/WebCore/Modules/filesystem/LocalFileSystem.cpp:80
> > +#if !PLATFORM(EFL)
> 
> Why do you disable this functions on EFL port ?

As you can see at https://bugs.webkit.org/show_bug.cgi?id=79193, 
I'm planning to re-implement those functions in AsyncFileSystemEfl.cpp.

To make the path of the file system, FileSystemType is needed.
But, FileSystemType is not a parameter of AsyncFileSystem::openFileSystem.
So, I need to re-implement LocalFileSystem::requestFileSystem function to make
proper file system path before calling AsyncFileSystem::openFileSystem.

Same as LocalFileSystem::readFileSystem.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #13)
> As you can see at https://bugs.webkit.org/show_bug.cgi?id=79193, 
> I'm planning to re-implement those functions in AsyncFileSystemEfl.cpp.
> 
> To make the path of the file system, FileSystemType is needed.
> But, FileSystemType is not a parameter of AsyncFileSystem::openFileSystem.
> So, I need to re-implement LocalFileSystem::requestFileSystem function to make
> proper file system path before calling AsyncFileSystem::openFileSystem.
> 
> Same as LocalFileSystem::readFileSystem.

Or, I think I can try to modify AsyncFileSystem::openFileSystem.

Gyuyoung, what do you think?

[Gyuyoung Kim]

Comment on attachment 166172 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166172&action=review

>>> Source/WebCore/Modules/filesystem/LocalFileSystem.cpp:80
>>> +#if !PLATFORM(EFL)
>> 
>> Why do you disable this functions on EFL port ?
> 
> As you can see at https://bugs.webkit.org/show_bug.cgi?id=79193, 
> I'm planning to re-implement those functions in AsyncFileSystemEfl.cpp.
> 
> To make the path of the file system, FileSystemType is needed.
> But, FileSystemType is not a parameter of AsyncFileSystem::openFileSystem.
> So, I need to re-implement LocalFileSystem::requestFileSystem function to make
> proper file system path before calling AsyncFileSystem::openFileSystem.
> 
> Same as LocalFileSystem::readFileSystem.

IMO, it would be good if you re-implement functions you need in LocalFileSystemEfl.cpp.

> Source/WebCore/platform/efl/AsyncFileSystemEfl.cpp:57
> +void LocalFileSystem::deleteFileSystem(ScriptExecutionContext*, FileSystemType, PassOwnPtr<AsyncFileSystemCallbacks>)

Why do you implement LocalFileSystem member functions in AsyncFileSystem ?

[Dongwoo Joshua Im (dwim)]

(In reply to comment #15)
> (From update of attachment 166172 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166172&action=review
> 

I create a new bug, https://bugs.webkit.org/show_bug.cgi?id=97963.

After this bug, I can remove the macro which you concerning very much now. ;-)

[Kinuko Yasuda]

Comment on attachment 166172 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166172&action=review

> Source/WebCore/ChangeLog:3
> +        [EFL] Add dummy implementation of File system API.

nit: dummy -> skeleton might sound better/appropriate.

>>>> Source/WebCore/Modules/filesystem/LocalFileSystem.cpp:80
>>>> +#if !PLATFORM(EFL)
>>> 
>>> Why do you disable this functions on EFL port ?
>> 
>> As you can see at https://bugs.webkit.org/show_bug.cgi?id=79193, 
>> I'm planning to re-implement those functions in AsyncFileSystemEfl.cpp.
>> 
>> To make the path of the file system, FileSystemType is needed.
>> But, FileSystemType is not a parameter of AsyncFileSystem::openFileSystem.
>> So, I need to re-implement LocalFileSystem::requestFileSystem function to make
>> proper file system path before calling AsyncFileSystem::openFileSystem.
>> 
>> Same as LocalFileSystem::readFileSystem.
> 
> IMO, it would be good if you re-implement functions you need in LocalFileSystemEfl.cpp.

If having no type argument in openFileSystem is the blocking issue I think you could probably change its signature and remove the FIXME comment at the method (in a separate patch), as no live code (at least for submitted ones) implements the method yet.

[Gyuyoung Kim]

(In reply to comment #17)
> > IMO, it would be good if you re-implement functions you need in LocalFileSystemEfl.cpp.
> 
> If having no type argument in openFileSystem is the blocking issue I think you could probably change its signature and remove the FIXME comment at the method (in a separate patch), as no live code (at least for submitted ones) implements the method yet.

Yes, Bug 97963 already did it. :-)

[Kinuko Yasuda]

(In reply to comment #18)
> (In reply to comment #17)
> > > IMO, it would be good if you re-implement functions you need in LocalFileSystemEfl.cpp.
> > 
> > If having no type argument in openFileSystem is the blocking issue I think you could probably change its signature and remove the FIXME comment at the method (in a separate patch), as no live code (at least for submitted ones) implements the method yet.
> 
> Yes, Bug 97963 already did it. :-)

Cool, thanks for fixing it!

[Dongwoo Joshua Im (dwim)]

(In reply to comment #17)
> (From update of attachment 166172 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166172&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        [EFL] Add dummy implementation of File system API.
> 
> nit: dummy -> skeleton might sound better/appropriate.

Uhm.. It could be more appropriate.
And, I used the word "skeleton" in the comment below to describe this patch, as well.

I will change the name of this bug.

[Dongwoo Joshua Im (dwim)]

Created attachment 166636 [details]
patch

[Gyuyoung Kim]

Comment on attachment 166636 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166636&action=review

> Source/WebCore/platform/efl/AsyncFileSystemEfl.h:43
> +    virtual void move(const KURL& sourcePath, const KURL& destinationPath, PassOwnPtr<AsyncFileSystemCallbacks>);

Generally, parameter name is omitted in .h file, then it is mentioned in .cpp file with /* */ when it needs meaning. See also, webkit mailing list which discussed this Yesterday.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #22)
> (From update of attachment 166636 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166636&action=review
> 
> > Source/WebCore/platform/efl/AsyncFileSystemEfl.h:43
> > +    virtual void move(const KURL& sourcePath, const KURL& destinationPath, PassOwnPtr<AsyncFileSystemCallbacks>);
> 
> Generally, parameter name is omitted in .h file, then it is mentioned in .cpp file with /* */ when it needs meaning. See also, webkit mailing list which discussed this Yesterday.

Wow.. yesterday.. ;-)

It's ok to remove the name of parameters from that file,
because there are the names and meanings are described in AsyncFileSystem.h file.

[Dongwoo Joshua Im (dwim)]

Created attachment 166802 [details]
patch

I've fixed the patch regarding Gyuyoung's comment.

[WebKit Review Bot]

Comment on attachment 166802 [details]
patch

Clearing flags on attachment: 166802

Committed r130281: <http://trac.webkit.org/changeset/130281>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.