[EFL] Calling ewk_settings_application_cache_path_set() more than once should have no effect.

• Component: WebKit EFL
• Status: RESOLVED WONTFIX

[Donggwan Kim]

ewk_settings_application_cache_path_set should be called once because ApplicationCacheStorage::setCacheDirectory is not allowed to be called more that once.
Please refer to discussion about this in https://bugs.webkit.org/show_bug.cgi?id=118584

[Donggwan Kim]

Created attachment 206834 [details]
Patch

[Chris Dumez]

Comment on attachment 206834 [details]
Patch

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

> Source/WebKit/efl/tests/test_ewk_setting.cpp:-127
> -    ewk_settings_application_cache_path_set("~/tmp/webkitApp");

Instead of removing the check, it would be better to keep it to make sure that:
- it does not crash (especially in debug)
- the path is not changed the second time

This may require some fixes in WebKit but I'm sure you can manage :)

[Donggwan Kim]

It sounds reasonable to me.
I will modify both test cases and ewk_settings_application_cache_path_set method to satisfy your requirement.
And i will change title of this patch as well.

[Chris Dumez]

Eh.. Let's agree on a shorter bug title like:
Calling ewk_settings_application_cache_path_set() more than once should have no effect

[Donggwan Kim]

Done

[Donggwan Kim]

Created attachment 206866 [details]
Patch

[Mikhail Pozdnyakov]

Comment on attachment 206866 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_settings.cpp:364
> +        return;

How would the caller know whether the path has been updated or not?

[Donggwan Kim]

Created attachment 206870 [details]
Patch

[Donggwan Kim]

(In reply to comment #7)
> (From update of attachment 206866 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206866&action=review
> 
> > Source/WebKit/efl/ewk/ewk_settings.cpp:364
> > +        return;
> 
> How would the caller know whether the path has been updated or not?

I changed return type to boolean from void to check it.

[Mikhail Pozdnyakov]

Comment on attachment 206870 [details]
Patch

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

> Source/WebKit/efl/tests/test_ewk_setting.cpp:123
>      ewk_settings_application_cache_path_set("~/data/webkitApp");

the returned value should be tested, right?

[Mikhail Pozdnyakov]

(In reply to comment #10)
> (From update of attachment 206870 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206870&action=review
> 
> > Source/WebKit/efl/tests/test_ewk_setting.cpp:123
> >      ewk_settings_application_cache_path_set("~/data/webkitApp");
> 
> the returned value should be tested, right?
s/tested/checked

[Chris Dumez]

Comment on attachment 206870 [details]
Patch

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

>>> Source/WebKit/efl/tests/test_ewk_setting.cpp:123
>>>      ewk_settings_application_cache_path_set("~/data/webkitApp");
>> 
>> the returned value should be tested, right?
> 
> s/tested/checked

This is the first time we call it and the path does not change? Why? Who calls it first?

[Donggwan Kim]

Created attachment 206877 [details]
Patch

[Donggwan Kim]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 206870 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=206870&action=review
> > 
> > > Source/WebKit/efl/tests/test_ewk_setting.cpp:123
> > >      ewk_settings_application_cache_path_set("~/data/webkitApp");
> > 
> > the returned value should be tested, right?
> s/tested/checked
Done

[Chris Dumez]

Comment on attachment 206877 [details]
Patch

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

> Source/WebKit/efl/tests/test_ewk_setting.cpp:123
> +    ASSERT_FALSE(ewk_settings_application_cache_path_set("~/data/webkitApp"));

Same question as before. When does it succeed?

[Donggwan Kim]

(In reply to comment #12)
> (From update of attachment 206870 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206870&action=review
> 
> >>> Source/WebKit/efl/tests/test_ewk_setting.cpp:123
> >>>      ewk_settings_application_cache_path_set("~/data/webkitApp");
> >> 
> >> the returned value should be tested, right?
> > 
> > s/tested/checked
> 
> This is the first time we call it and the path does not change? Why? Who calls it first?

It is initialized in ewk_init() which is defined in ewk_main.cpp and is called from SetUp method of EWKTestEnvironment.

[Chris Dumez]

Comment on attachment 206870 [details]
Patch

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

>>>>>> Source/WebKit/efl/tests/test_ewk_setting.cpp:123
>>>>>>      ewk_settings_application_cache_path_set("~/data/webkitApp");
>>>>> 
>>>>> the returned value should be tested, right?
>>>> 
>>>> s/tested/checked
>>> 
>>> This is the first time we call it and the path does not change? Why? Who calls it first?
>> 
>> Done
> 
> It is initialized in ewk_init() which is defined in ewk_main.cpp and is called from SetUp method of EWKTestEnvironment.

Well then, why is there such API at all if it will NEVER work?

[Mikhail Pozdnyakov]

(In reply to comment #17)
> (From update of attachment 206870 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206870&action=review
> 
> >>>>>> Source/WebKit/efl/tests/test_ewk_setting.cpp:123
> >>>>>>      ewk_settings_application_cache_path_set("~/data/webkitApp");
> >>>>> 
> >>>>> the returned value should be tested, right?
> >>>> 
> >>>> s/tested/checked
> >>> 
> >>> This is the first time we call it and the path does not change? Why? Who calls it first?
> >> 
> >> Done
> > 
> > It is initialized in ewk_init() which is defined in ewk_main.cpp and is called from SetUp method of EWKTestEnvironment.
> 
> Well then, why is there such API at all if it will NEVER work?

:-)

[Chris Dumez]

(In reply to comment #17)
> (From update of attachment 206870 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206870&action=review
> 
> >>>>>> Source/WebKit/efl/tests/test_ewk_setting.cpp:123
> >>>>>>      ewk_settings_application_cache_path_set("~/data/webkitApp");
> >>>>> 
> >>>>> the returned value should be tested, right?
> >>>> 
> >>>> s/tested/checked
> >>> 
> >>> This is the first time we call it and the path does not change? Why? Who calls it first?
> >> 
> >> Done
> > 
> > It is initialized in ewk_init() which is defined in ewk_main.cpp and is called from SetUp method of EWKTestEnvironment.
> 
> Well then, why is there such API at all if it will NEVER work?

There are 2 solutions:
1. Stop setting the app cache path in ewk main and let the app set the path:
Pros: The app can use another path than the default one
Cons: This is a behavior change and app cache will be disabled unless applications are updated to set the app cache path.

2. Remove this public API function
Pros: No behavior change.
Cons: The app cannot change the app cache path

I would go for 1 because this is WebKit1 and it is in maintenance mode. However, gyuyoung / ryuan may have a different opinion. I'll let them decide :)

[Ryuan Choi]

(In reply to comment #19)
> (In reply to comment #17)
> > (From update of attachment 206870 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=206870&action=review
> > 
> > >>>>>> Source/WebKit/efl/tests/test_ewk_setting.cpp:123
> > >>>>>>      ewk_settings_application_cache_path_set("~/data/webkitApp");
> > >>>>> 
> > >>>>> the returned value should be tested, right?
> > >>>> 
> > >>>> s/tested/checked
> > >>> 
> > >>> This is the first time we call it and the path does not change? Why? Who calls it first?
> > >> 
> > >> Done
> > > 
> > > It is initialized in ewk_init() which is defined in ewk_main.cpp and is called from SetUp method of EWKTestEnvironment.
> > 
> > Well then, why is there such API at all if it will NEVER work?
> 
> There are 2 solutions:
> 1. Stop setting the app cache path in ewk main and let the app set the path:
> Pros: The app can use another path than the default one
> Cons: This is a behavior change and app cache will be disabled unless applications are updated to set the app cache path.
> 
> 2. Remove this public API function
> Pros: No behavior change.
> Cons: The app cannot change the app cache path
> 
> I would go for 1 because this is WebKit1 and it is in maintenance mode. However, gyuyoung / ryuan may have a different opinion. I'll let them decide :)

If I remember correctly, Tizen used it long time ago but we don't use this now.

I don't have strong opinion about it, but second option is also available because we didn't have public release.

[Gyuyoung Kim]

Comment on attachment 206877 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_settings.cpp:363
> +    if (s_offlineAppCachePath)

We call this API in _ewk_init_body() by defalut. Thus, can't application change this path using this API ? It seems to me this API needs to be internal function or removed.

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.