Bug 91187

Summary: [EFL] Skeleton code of File System API.
Product: WebKit Reporter: Dongwoo Joshua Im (dwim) <dw.im>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: donggwan.kim, gyuyoung.kim, gyuyoung.kim, kinuko, lucas.de.marchi, rakuco, s.choi, sw0524.lee, tzik, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Bug Depends on: 91185, 97963, 98106    
Bug Blocks: 79193    
Attachments:
Description Flags
Patch
none
Patch
none
patch
none
patch
none
patch
none
patch none

Description Dongwoo Joshua Im (dwim) 2012-07-12 20:25:35 PDT
EFL port cannot be built with enabling the ENABLE_FILE_SYSTEM flag.

I'm preparing the patch now.
Comment 1 Dongwoo Joshua Im (dwim) 2012-07-12 21:03:41 PDT
Created attachment 152147 [details]
Patch
Comment 2 Dongwoo Joshua Im (dwim) 2012-07-12 21:08:27 PDT
Created attachment 152150 [details]
Patch
Comment 3 Gyuyoung Kim 2012-07-14 04:54:47 PDT
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.
Comment 4 Dongwoo Joshua Im (dwim) 2012-07-15 18:05:31 PDT
(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.
Comment 5 Dongwoo Joshua Im (dwim) 2012-07-15 22:35:56 PDT
(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. ;)
Comment 6 Dongwoo Joshua Im (dwim) 2012-09-27 22:11:28 PDT
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.
Comment 7 Chris Dumez 2012-09-27 22:43:39 PDT
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?
Comment 8 Dongwoo Joshua Im (dwim) 2012-09-27 23:16:25 PDT
(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.
Comment 9 Gyuyoung Kim 2012-09-27 23:48:22 PDT
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.
Comment 10 Dongwoo Joshua Im (dwim) 2012-09-28 00:47:11 PDT
(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.
Comment 11 Dongwoo Joshua Im (dwim) 2012-09-28 01:40:36 PDT
Created attachment 166172 [details]
patch

I've fixed the patch regarding the comments.
Comment 12 Gyuyoung Kim 2012-09-28 02:05:11 PDT
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 ?
Comment 13 Dongwoo Joshua Im (dwim) 2012-09-28 04:31:46 PDT
(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.
Comment 14 Dongwoo Joshua Im (dwim) 2012-09-28 20:07:35 PDT
(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?
Comment 15 Gyuyoung Kim 2012-09-28 22:23:24 PDT
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 ?
Comment 16 Dongwoo Joshua Im (dwim) 2012-09-29 01:01:22 PDT
(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. ;-)
Comment 17 Kinuko Yasuda 2012-10-01 00:40:48 PDT
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.
Comment 18 Gyuyoung Kim 2012-10-01 00:45:13 PDT
(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. :-)
Comment 19 Kinuko Yasuda 2012-10-01 00:47:27 PDT
(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!
Comment 20 Dongwoo Joshua Im (dwim) 2012-10-01 23:50:47 PDT
(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.
Comment 21 Dongwoo Joshua Im (dwim) 2012-10-02 01:07:32 PDT
Created attachment 166636 [details]
patch
Comment 22 Gyuyoung Kim 2012-10-02 06:51:16 PDT
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.
Comment 23 Dongwoo Joshua Im (dwim) 2012-10-02 20:29:12 PDT
(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.
Comment 24 Dongwoo Joshua Im (dwim) 2012-10-02 20:46:52 PDT
Created attachment 166802 [details]
patch

I've fixed the patch regarding Gyuyoung's comment.
Comment 25 WebKit Review Bot 2012-10-03 06:13:18 PDT
Comment on attachment 166802 [details]
patch

Clearing flags on attachment: 166802

Committed r130281: <http://trac.webkit.org/changeset/130281>
Comment 26 WebKit Review Bot 2012-10-03 06:13:25 PDT
All reviewed patches have been landed.  Closing bug.