Bug 98344

Created attachment 167043 [details] patch

Comment on attachment 167043 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=167043&action=review > Source/WebKit/efl/ewk/ewk_settings.cpp:153 > + WebCore::LocalFileSystem::initializeLocalFileSystem(WTF::String::fromUTF8(path)); WTF:: is probably not needed. WebCore::LocalFileSystem::initializeLocalFileSystem() can only be called *ONCE* due to the ASSERT(!s_instance); in debug mode. This makes this API useless since ewk_settings_file_system_path_set() is already called once from ewk_main. Therefore, it is no longer possible for the app to change it later. > Source/WebKit/efl/ewk/ewk_settings.h:132 > +EAPI void ewk_settings_file_system_path_set(const char* path); Star on wrong side. > Source/WebKit/efl/ewk/ewk_settings.h:145 > +EAPI const char* ewk_settings_file_system_path_get(void); Star on wrong side. > Source/WebKit/efl/tests/test_ewk_settings.cpp:35 > +* @brief Checking whether function properly returns correct value. Missing space at beginning? > Source/WebKit/efl/tests/test_ewk_settings.cpp:36 > +*/ Ditto. > Source/WebKit/efl/tests/test_ewk_settings.cpp:41 > + ewk_settings_file_system_path_set("/WebKitEfl/FileSystem"); I'm not sure it is a very good example to use "/WebKitEfl/FileSystem". This may lead to permission problems later.

(In reply to comment #2) > (From update of attachment 167043 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167043&action=review > > > Source/WebKit/efl/ewk/ewk_settings.cpp:153 > > + WebCore::LocalFileSystem::initializeLocalFileSystem(WTF::String::fromUTF8(path)); > > WTF:: is probably not needed. > > WebCore::LocalFileSystem::initializeLocalFileSystem() can only be called *ONCE* due to the ASSERT(!s_instance); in debug mode. > This makes this API useless since ewk_settings_file_system_path_set() is already called once from ewk_main. Therefore, it is no longer possible for the app to change it later. > What do you mean "app" in your comment above? If you mean web apps, web apps are not supposed to change the path. The url of web app will be used as the name of directory. (e.g. <base_path>/<path_made_by_the_url>) If you mean native apps, it could set the path using the XDG_DATA_HOME. BTW, I need to use 'efreet_data_home_get' here, rather than 'efreet_cache_home_get'. > > Source/WebKit/efl/ewk/ewk_settings.h:132 > > +EAPI void ewk_settings_file_system_path_set(const char* path); > > Star on wrong side. > Oh.. WebKit & EFL coding style always make me confused. ;) I'll fix this. > > Source/WebKit/efl/ewk/ewk_settings.h:145 > > +EAPI const char* ewk_settings_file_system_path_get(void); > > Star on wrong side. > same as above case. > > Source/WebKit/efl/tests/test_ewk_settings.cpp:35 > > +* @brief Checking whether function properly returns correct value. > > Missing space at beginning? > Hmm.. I don't think it is mandatory, isn't it? But It's not looks good, though. I'll give a space there in the next patch. > > Source/WebKit/efl/tests/test_ewk_settings.cpp:36 > > +*/ > > Ditto. > same as above case. > > Source/WebKit/efl/tests/test_ewk_settings.cpp:41 > > + ewk_settings_file_system_path_set("/WebKitEfl/FileSystem"); > > I'm not sure it is a very good example to use "/WebKitEfl/FileSystem". This may lead to permission problems later. The directory is only for unit test to see the API is working well.

Created attachment 167058 [details] patch

Comment on attachment 167058 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=167058&action=review > Source/WebKit/efl/tests/test_ewk_settings.cpp:40 > +#if ENABLE(FILE_SYSTEM) If FILE_SYSTEM is disabled, I think we don't need to call loadUrl().

(In reply to comment #5) > (From update of attachment 167058 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167058&action=review > > > Source/WebKit/efl/tests/test_ewk_settings.cpp:40 > > +#if ENABLE(FILE_SYSTEM) > > If FILE_SYSTEM is disabled, I think we don't need to call loadUrl(). Uhm... yes, you are right. We don't even need to do anything if FILE_SYSTEM is disabled. I will fix it.

Created attachment 167083 [details] patch I've fixed the patch regarding gyuyoung's comment.

(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 167043 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=167043&action=review > > > > > Source/WebKit/efl/ewk/ewk_settings.cpp:153 > > > + WebCore::LocalFileSystem::initializeLocalFileSystem(WTF::String::fromUTF8(path)); > > > > WTF:: is probably not needed. > > > > WebCore::LocalFileSystem::initializeLocalFileSystem() can only be called *ONCE* due to the ASSERT(!s_instance); in debug mode. > > This makes this API useless since ewk_settings_file_system_path_set() is already called once from ewk_main. Therefore, it is no longer possible for the app to change it later. > > > > What do you mean "app" in your comment above? > > If you mean web apps, web apps are not supposed to change the path. > The url of web app will be used as the name of directory. > (e.g. <base_path>/<path_made_by_the_url>) > > If you mean native apps, it could set the path using the XDG_DATA_HOME. By app, I mean the browser for example. What's the point of adding a public API (ewk_settings_file_system_path_set) if the browser can NOT call it? IMO, it should be private and called only once in ewk_main. Right now, you add a public function that will hit an assertion if the browser calls it :( I don't see the point.

(In reply to comment #8) > > By app, I mean the browser for example. > > What's the point of adding a public API (ewk_settings_file_system_path_set) if the browser can NOT call it? IMO, it should be private and called only once in ewk_main. > > Right now, you add a public function that will hit an assertion if the browser calls it :( I don't see the point. Hum.. I understand what you are saying... So, setter is still needed, but it shouldn't be in public. And, I think getter also not needed to be in public. Then, I will make setter/getter as a private function.

(In reply to comment #9) > (In reply to comment #8) > > > > By app, I mean the browser for example. > > > > What's the point of adding a public API (ewk_settings_file_system_path_set) if the browser can NOT call it? IMO, it should be private and called only once in ewk_main. > > > > Right now, you add a public function that will hit an assertion if the browser calls it :( I don't see the point. > > Hum.. > I understand what you are saying... > > So, setter is still needed, but it shouldn't be in public. > And, I think getter also not needed to be in public. > > Then, I will make setter/getter as a private function. And then, no unit test is needed, IMO.

Comment on attachment 167083 [details] patch Will you add this to WK2 as well ?

(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > > > > By app, I mean the browser for example. > > > > > > What's the point of adding a public API (ewk_settings_file_system_path_set) if the browser can NOT call it? IMO, it should be private and called only once in ewk_main. > > > > > > Right now, you add a public function that will hit an assertion if the browser calls it :( I don't see the point. > > > > Hum.. > > I understand what you are saying... > > > > So, setter is still needed, but it shouldn't be in public. > > And, I think getter also not needed to be in public. > > > > Then, I will make setter/getter as a private function. > > And then, no unit test is needed, IMO. Right, I agree.

(In reply to comment #11) > (From update of attachment 167083 [details]) > Will you add this to WK2 as well ? Sure, but by another patch. One of my colleague will upload the patch for WK2. I will concentrate to implement this feature in WebCore/plaform/efl after this patch.

Created attachment 168388 [details] patch

Comment on attachment 168388 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=168388&action=review > Source/WebKit/ChangeLog:3 > + [EFL] Add the file system path set API. This patch adds internal function instead of API > Source/WebKit/ChangeLog:8 > + To set the basepath of the File System API, path set API is needed. ditto.

Created attachment 168599 [details] patch I've fixed the patch regarding Gyuyoung's comments.

Comment on attachment 168599 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=168599&action=review > Source/WebKit/efl/ewk/ewk_settings.cpp:339 > +#endif One more thing. Please add below code. #if ENABLE(FILE_SYSTEM) ... #else UNUSED_PARAM(path); #endif

(In reply to comment #17) > (From update of attachment 168599 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=168599&action=review > > > Source/WebKit/efl/ewk/ewk_settings.cpp:339 > > +#endif > > One more thing. Please add below code. > > #if ENABLE(FILE_SYSTEM) > ... > #else > UNUSED_PARAM(path); > #endif Sure.

Created attachment 168615 [details] patch

Is there anyone knows why this patch is not commited yet?

Comment on attachment 168615 [details] patch Clearing flags on attachment: 168615 Committed r131329: <http://trac.webkit.org/changeset/131329>

All reviewed patches have been landed. Closing bug.