Bug 91187

Created attachment 152147 [details] Patch

Created attachment 152150 [details] Patch

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.

(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.

(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. ;)

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 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?

(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 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.

(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.

Created attachment 166172 [details] patch I've fixed the patch regarding the comments.

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 ?

(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.

(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 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 ?

(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 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.

(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. :-)

(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!

(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.

Created attachment 166636 [details] patch

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.

(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.

Created attachment 166802 [details] patch I've fixed the patch regarding Gyuyoung's comment.

Comment on attachment 166802 [details] patch Clearing flags on attachment: 166802 Committed r130281: <http://trac.webkit.org/changeset/130281>

All reviewed patches have been landed. Closing bug.