Bug 45446 – [EFL] Add setting API to set a local storage database path.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Gyuyoung Kim]

We need to set database path to use local storage.

[Gyuyoung Kim]

Created attachment 67012 [details]
Patch

The local storage database path is set via setLocalStorageDatabsePath() of Settings class. Thus, we should use settings instance to set the database path. I was obliged to define this method in ewk_view.cpp. In my opinion, we need to move settgins methods to ewk_settings.cpp. I'd like to listen your opinions about this patch. :). I was obliged to modify eweblauncher as well.

Thanks,
Gyuyoung Kim.

[Gyuyoung Kim]

Created attachment 67013 [details]
Patch

There was missing changelog.

[Rafael Antognolli]

Hi Gyuyoung,

Yes, from an organisational point of view, you are right, we should move all setting functions to ewk_settings.{cpp,h}. But if you look at the setting functions, almost all of them deal with the page_settings, that is an WebCore::Settings and we get it from the page. And the page is relative to the view.

And the ones that don't do that, depend at least on the frame loader, that need a frame, thus usually associated with a view.

In the ewk_settings.cpp, all functions deal with settings that are independent of the view, that are set globally, so we don't need a view.

Anyway, your patch looks good to me.

[Gyuyoung Kim]

(In reply to comment #3)
> Hi Gyuyoung,
> 
> Yes, from an organisational point of view, you are right, we should move all setting functions to ewk_settings.{cpp,h}. But if you look at the setting functions, almost all of them deal with the page_settings, that is an WebCore::Settings and we get it from the page. And the page is relative to the view.
> 
> And the ones that don't do that, depend at least on the frame loader, that need a frame, thus usually associated with a view.
> 
> In the ewk_settings.cpp, all functions deal with settings that are independent of the view, that are set globally, so we don't need a view.
> 
> Anyway, your patch looks good to me.

Hello antognolli,

I understand your comments. OK, Let's keep current architecture for a while. If I find good solution, I wiil discuss it with you guys again. :)

Thanks,
Gyuyoung Kim

[Gyuyoung Kim]

I tested this patch in url below,
http://people.w3.org/mike/localstorage.html

After EWebLauncher is exit, the previous datas are alive continually.

[Kenneth Rohde Christiansen]

Antognolli, what do you think?

[Rafael Antognolli]

Sorry, my bad, I just missed that.

You need to free the path with eina_stringshare_del in _ewk_view_priv_del().

Another point is: do you know what's the default value for the database path? If it's not empty, then you need to set it in the settings->local_storage_database_path during the ewk_view creation too, so they won't have different and thus inconsistent values. This is done in _ewk_view_priv_new(). There you also can set a default path if there's none yet.

[Gyuyoung Kim]

(In reply to comment #7)
> Sorry, my bad, I just missed that.
> 
> You need to free the path with eina_stringshare_del in _ewk_view_priv_del().
>
Thank you for your comment. I will free the database path in _ewk_view_priv_del().
 
> Another point is: do you know what's the default value for the database path? If it's not empty, then you need to set it in the settings->local_storage_database_path during the ewk_view creation too, so they won't have different and thus inconsistent values. This is done in _ewk_view_priv_new(). There you also can set a default path if there's none yet.

I don't find if there is default database path until now. I continue to find if there is a default database path in webkit. If there are no default database path. IMO, we can set default database path. However, as you know, EWebLauncher defines database path. And, the database path can be changed according to user id.

832     tmp = getenv("TMPDIR");
833     if (!tmp)
834         tmp = "/tmp";
835     snprintf(path, sizeof(path), "%s/.ewebkit-%u", tmp, getuid());

So, default database path will be changed by EWebLauncher. Ok, I look this into further. Them, I will post my thought here again. Thanks.

[Rafael Antognolli]

(In reply to comment #8)
> > Another point is: do you know what's the default value for the database path? If it's not empty, then you need to set it in the settings->local_storage_database_path during the ewk_view creation too, so they won't have different and thus inconsistent values. This is done in _ewk_view_priv_new(). There you also can set a default path if there's none yet.
> 
> I don't find if there is default database path until now. I continue to find if there is a default database path in webkit. If there are no default database path. IMO, we can set default database path. However, as you know, EWebLauncher defines database path. And, the database path can be changed according to user id.
> 
> 832     tmp = getenv("TMPDIR");
> 833     if (!tmp)
> 834         tmp = "/tmp";
> 835     snprintf(path, sizeof(path), "%s/.ewebkit-%u", tmp, getuid());
> 
> So, default database path will be changed by EWebLauncher. Ok, I look this into further. Them, I will post my thought here again. Thanks.

Ok, that's not a problem, you can initialise it in EWebLauncher. But in _ewk_view_priv_new() it would be good to have something like:

s = priv->page_settings->localStorageDatabasePath();
priv->settings.local_storage_database_path = eina_stringshare_add(s.utf8().data());

I just checked the default value of priv->page_settings->localStorageDatabasePath(), and it defaults to "". So it would be good to make sure that it makes sense to have enabled local storage, without a local storage database path set. If it doesn't, then maybe we should disable local storage by default.

[Gyuyoung Kim]

Created attachment 67629 [details]
Patch

Antognolli, Thank you for your comment. 

I set default path from "priv->page_settings->localStorageDatabasePath()"

 40 +    s = priv->page_settings->localStorageDatabasePath();
 41 +    priv->settings.local_storage_database_path = eina_stringshare_add(s.string().utf8().data());
 
As mentioned in your previous comment, the database path is null until user sets database path. However, it seems to me that if the default path is set once, default database path will be set whenever ewk_view is created.

[Rafael Antognolli]

Ok, the patch seems nice now.

[Antonio Gomes]

Comment on attachment 67629 [details]
Patch

Please make sure it applies fine against tot (see the purple balloons)

[WebKit Commit Bot]

Comment on attachment 67629 [details]
Patch

Rejecting patch 67629 from commit-queue.

Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Antonio Gomes', u'--force']" exit_code: 1
Last 500 characters of output:
ching file WebKit/efl/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKit/efl/ewk/ewk_view.cpp
Hunk #1 FAILED at 90.
Hunk #2 succeeded at 580 (offset 1 line).
Hunk #3 succeeded at 666 (offset 1 line).
Hunk #4 succeeded at 2790 (offset 5 lines).
1 out of 4 hunks FAILED -- saving rejects to file WebKit/efl/ewk/ewk_view.cpp.rej
patching file WebKit/efl/ewk/ewk_view.h
patching file WebKitTools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKitTools/EWebLauncher/main.c

Full output: http://queues.webkit.org/results/4037043

[Gyuyoung Kim]

Created attachment 68045 [details]
Patch

I make this patch again based on latest webkit.

[WebKit Commit Bot]

Comment on attachment 68045 [details]
Patch

Rejecting patch 68045 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 68045]" exit_code: 2
Cleaning working directory
Updating working directory
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=68045&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=45446&ctype=xml
Processing 1 patch from 1 bug.
Processing patch 68045 from bug 45446.
Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Antonio Gomes', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4026153

[Gyuyoung Kim]

Created attachment 68866 [details]
Patch

New Patch.

[WebKit Commit Bot]

Comment on attachment 68866 [details]
Patch

Rejecting patch 68866 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
Last 500 characters of output:
l tests successful.
Files=14, Tests=304,  1 wallclock secs ( 0.72 cusr +  0.16 csys =  0.88 CPU)
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Projects/CommitQueue/LayoutTests
Testing 21376 test cases.
java/lc3/JSObject/ToObject-001.html -> failed

Exiting early after 1 failures. 17486 tests run.
280.32s total testing time

17485 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
28 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/4084102

[Gyuyoung Kim]

(In reply to comment #17)
> (From update of attachment 68866 [details])
> Rejecting patch 68866 from commit-queue.
> 
> Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
> Last 500 characters of output:
> l tests successful.
> Files=14, Tests=304,  1 wallclock secs ( 0.72 cusr +  0.16 csys =  0.88 CPU)
> Running build-dumprendertree
> Compiling Java tests
> make: Nothing to be done for `default'.
> Running tests from /Projects/CommitQueue/LayoutTests
> Testing 21376 test cases.
> java/lc3/JSObject/ToObject-001.html -> failed
> 
> Exiting early after 1 failures. 17486 tests run.
> 280.32s total testing time
> 
> 17485 test cases (99%) succeeded
> 1 test case (<1%) had incorrect layout
> 28 test cases (<1%) had stderr output
> 
> Full output: http://queues.webkit.org/results/4084102

Hmm, I guess why this patch made an error in "java/lc3/JSObject/ToObject-001.html" caes. I think this patch can't influence on the test case. Does anyone know the reason ?

[Gyuyoung Kim]

Created attachment 68868 [details]
Patch

There was a problem previous patch. I fix it.
Sorry for my frequent uploading.

Thanks.

[Antonio Gomes]

Comment on attachment 68868 [details]
Patch

... and once again :-)

[Antonio Gomes]

Please clear the review flag on obsolete patches.

[WebKit Commit Bot]

Comment on attachment 68868 [details]
Patch

Clearing flags on attachment: 68868

Committed r68363: <http://trac.webkit.org/changeset/68363>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.