Bug 118759 - [EFL] Calling ewk_settings_application_cache_path_set() more than once should have no effect.
Summary: [EFL] Calling ewk_settings_application_cache_path_set() more than once should...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-16 18:34 PDT by Donggwan Kim
Modified: 2017-03-11 10:43 PST (History)
8 users (show)

See Also:


Attachments
Patch (1.56 KB, patch)
2013-07-16 18:35 PDT, Donggwan Kim
no flags Details | Formatted Diff | Diff
Patch (2.81 KB, patch)
2013-07-17 02:00 PDT, Donggwan Kim
no flags Details | Formatted Diff | Diff
Patch (3.78 KB, patch)
2013-07-17 02:23 PDT, Donggwan Kim
no flags Details | Formatted Diff | Diff
Patch (3.93 KB, patch)
2013-07-17 03:06 PDT, Donggwan Kim
gyuyoung.kim: review-
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Donggwan Kim 2013-07-16 18:34:02 PDT
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
Comment 1 Donggwan Kim 2013-07-16 18:35:42 PDT
Created attachment 206834 [details]
Patch
Comment 2 Chris Dumez 2013-07-17 00:49:48 PDT
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 :)
Comment 3 Donggwan Kim 2013-07-17 01:19:03 PDT
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.
Comment 4 Chris Dumez 2013-07-17 01:39:25 PDT
Eh.. Let's agree on a shorter bug title like:
Calling ewk_settings_application_cache_path_set() more than once should have no effect
Comment 5 Donggwan Kim 2013-07-17 01:52:52 PDT
Done
Comment 6 Donggwan Kim 2013-07-17 02:00:09 PDT
Created attachment 206866 [details]
Patch
Comment 7 Mikhail Pozdnyakov 2013-07-17 02:10:51 PDT
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?
Comment 8 Donggwan Kim 2013-07-17 02:23:14 PDT
Created attachment 206870 [details]
Patch
Comment 9 Donggwan Kim 2013-07-17 02:24:26 PDT
(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.
Comment 10 Mikhail Pozdnyakov 2013-07-17 02:28:10 PDT
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?
Comment 11 Mikhail Pozdnyakov 2013-07-17 02:28:38 PDT
(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
Comment 12 Chris Dumez 2013-07-17 02:40:09 PDT
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?
Comment 13 Donggwan Kim 2013-07-17 03:06:18 PDT
Created attachment 206877 [details]
Patch
Comment 14 Donggwan Kim 2013-07-17 03:06:56 PDT
(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
Comment 15 Chris Dumez 2013-07-17 03:07:32 PDT
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?
Comment 16 Donggwan Kim 2013-07-17 03:09:08 PDT
(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.
Comment 17 Chris Dumez 2013-07-17 03:10:09 PDT
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?
Comment 18 Mikhail Pozdnyakov 2013-07-17 03:12:55 PDT
(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?

:-)
Comment 19 Chris Dumez 2013-07-17 03:20:03 PDT
(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 :)
Comment 20 Ryuan Choi 2013-07-17 20:49:02 PDT
(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.
Comment 21 Gyuyoung Kim 2013-10-23 03:42:02 PDT
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.
Comment 22 Michael Catanzaro 2017-03-11 10:43:47 PST
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.