Bug 98344 – [EFL] Set the basepath of the File System API.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Dongwoo Joshua Im (dwim)]

ewk_settings_file_system_path_set and ewk_settings_file_system_path_get APIs are needed to maintain the file system.

[Dongwoo Joshua Im (dwim)]

Created attachment 167043 [details]
patch

[Chris Dumez]

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.

[Dongwoo Joshua Im (dwim)]

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

[Dongwoo Joshua Im (dwim)]

Created attachment 167058 [details]
patch

[Gyuyoung Kim]

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

[Dongwoo Joshua Im (dwim)]

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

[Dongwoo Joshua Im (dwim)]

Created attachment 167083 [details]
patch

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

[Chris Dumez]

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

[Dongwoo Joshua Im (dwim)]

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

[Dongwoo Joshua Im (dwim)]

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

[Gyuyoung Kim]

Comment on attachment 167083 [details]
patch

Will you add this to WK2 as well ?

[Chris Dumez]

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

[Dongwoo Joshua Im (dwim)]

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

[Dongwoo Joshua Im (dwim)]

Created attachment 168388 [details]
patch

[Gyuyoung Kim]

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.

[Dongwoo Joshua Im (dwim)]

Created attachment 168599 [details]
patch

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

[Gyuyoung Kim]

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

[Dongwoo Joshua Im (dwim)]

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

[Dongwoo Joshua Im (dwim)]

Created attachment 168615 [details]
patch

[Dongwoo Joshua Im (dwim)]

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

[WebKit Review Bot]

Comment on attachment 168615 [details]
patch

Clearing flags on attachment: 168615

Committed r131329: <http://trac.webkit.org/changeset/131329>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.