Bug 44239 – [EFL] Enable offline pages cache (patch)

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

[Mikołaj Małecki]

Enables offline pages cache and fixes the location of offline cache file
(it was set dumbly to $HOME; the fix changes it to $HOME/.webkit with fallback to /tmp/.webkit if HOME not set).

Please review.

[Mikołaj Małecki]

Created attachment 64826 [details]
Patch against r65660 with fixes as described in the ticket.

[Mikołaj Małecki]

Comment on attachment 64826 [details]
Patch against r65660 with fixes as described in the ticket.

> Index: WebKit/efl/ChangeLog
> ===================================================================
> --- WebKit/efl/ChangeLog	(revision 65660)
> +++ WebKit/efl/ChangeLog	(working copy)
> @@ -1,3 +1,14 @@
> [phone]  MikoÅaj MaÅecki  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [EFL] Enabled offline pages storage for WebKit EFL; fixed default location of offline cache files
> +
> +        * ewk/ewk_main.cpp:
> +        (ewk_init): Fixed location of offline cache file
> +        * ewk/ewk_view.cpp:
> +        (_ewk_view_priv_new): Added setting to enable offline pages
> +
>  2010-08-15  Gyuyoung Kim  <[e-mail]>
>  
>          Reviewed by Antonio Gomes.
> Index: WebKit/efl/ewk/ewk_main.cpp
> ===================================================================
> --- WebKit/efl/ewk/ewk_main.cpp	(revision 65660)
> +++ WebKit/efl/ewk/ewk_main.cpp	(working copy)
> @@ -21,10 +21,12 @@
>  #include "config.h"
>  #include "ewk_main.h"
>  
> +#include "ApplicationCacheStorage.h"
>  #include "EWebKit.h"
>  #include "Logging.h"
>  #include "PageCache.h"
>  #include "PageGroup.h"
> +#include <string>
>  #include "ewk_private.h"
>  #include "ewk_settings.h"
>  #include "runtime/InitializeThreading.h"
> @@ -118,7 +120,18 @@ int ewk_init(void)
>      WebCore::PageGroup::setShouldTrackVisitedLinks(true);
>  
>      // set default location of web database path
> -    ewk_settings_web_database_path_set(getenv("HOME"));
> +    { // this block is because of gotos
> +        using namespace std;
> +        // set default location of web database path
> +        const char* home = getenv("HOME");
> +        if (!home) // don't forget about the homeless
> +            home = "/tmp"; // this directory must always exist
> +        string wkdir = string(home) + "/.webkit";
> +        ewk_settings_web_database_path_set(wkdir.c_str());
> +
> +        // FIXME: It should be possible for client applications to override the default appcache location
> +        WebCore::cacheStorage().setCacheDirectory(wkdir.c_str());
> +    }
>  
>      // TODO: this should move to WebCore, already reported to webkit-gtk folks:
>  #ifdef WTF_USE_SOUP
> Index: WebKit/efl/ewk/ewk_view.cpp
> ===================================================================
> --- WebKit/efl/ewk/ewk_view.cpp	(revision 65660)
> +++ WebKit/efl/ewk/ewk_view.cpp	(working copy)
> @@ -559,6 +559,7 @@ static Ewk_View_Private_Data* _ewk_view_
>      priv->page_settings->setJavaScriptEnabled(true);
>      priv->page_settings->setPluginsEnabled(true);
>      priv->page_settings->setLocalStorageEnabled(true);
> +    priv->page_settings->setOfflineWebApplicationCacheEnabled(true);
>  
>      url = priv->page_settings->userStyleSheetLocation();
>      priv->settings.user_stylesheet = eina_stringshare_add(url.prettyURL().utf8().data());

[Mikołaj Małecki]

Created attachment 64956 [details]
Patch against r65660 with fixes as described in the ticket. (updated)

[Rafael Antognolli]

Some comments about the patch:

>      WebCore::PageGroup::setShouldTrackVisitedLinks(true);
>  
>      // set default location of web database path

Maybe you should bring this comment together with the call ewk_settings_web_database_path_set?

> -    ewk_settings_web_database_path_set(getenv("HOME"));
> +    { // this block is because of gotos

Do you really need this block? What's the difference if you don't have it? Considering my comments below of removing the std::string, you wouldn't need this anymore (and I didn't get the gotos idea).

> +        using namespace std;
> +        // set default location of web database path
> +        const char* home = getenv("HOME");
> +        if (!home) // don't forget about the homeless
> +            home = "/tmp"; // this directory must always exist
> +        string wkdir = string(home) + "/.webkit";

We are not using "string" anywhere else in WebKit-EFL. So, could you use C functions for creating this string, like snprintf (since you are later converting to c_str() anyway). Or maybe using WebCore's ones? Then you wouldn't need to include <string>.

> --- WebKit/efl/ewk/ewk_view.cpp	(revision 65660)
> +++ WebKit/efl/ewk/ewk_view.cpp	(working copy)
> @@ -559,6 +559,7 @@ static Ewk_View_Private_Data* _ewk_view_
>      priv->page_settings->setJavaScriptEnabled(true);
>      priv->page_settings->setPluginsEnabled(true);
>      priv->page_settings->setLocalStorageEnabled(true);
> +    priv->page_settings->setOfflineWebApplicationCacheEnabled(true);

Like the other page settings, you need to add a member of the struct priv->settings for it.

[Rafael Antognolli]

And another comment:
+        // FIXME: It should be possible for client applications to override the default appcache location

Maybe you could just add an api for this? It would be almost copy & paste of ewk_settings_web_database_path_set and get.

Thank you,
Rafael

[Mikołaj Małecki]

Created attachment 65559 [details]
Patch against r66096 with fixes as described in the ticket.

Additional changes: offline_app_cache and cache_directory are now public properties of ewk.

[Rafael Antognolli]

Patch looks nice now. I'm cc'ing some reviewers since they can give their votes.

And I think you should also set r? and cq? flags.

[WebKit Review Bot]

Attachment 65559 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/efl/ewk/ewk_main.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mikołaj Małecki]

Created attachment 65693 [details]
Patch against r66192 with fixes as described in the ticket.

Fixed the checkstyle-reported problem.

[Kenneth Rohde Christiansen]

Comment on attachment 65693 [details]
Patch against r66192 with fixes as described in the ticket.

WebKit/efl/ewk/ewk_main.cpp:170
 +          // Exit now - otherwise you may have some crash later
Remember that comments should start by capital letter and end with a dot.

[WebKit Commit Bot]

Comment on attachment 65693 [details]
Patch against r66192 with fixes as described in the ticket.

Rejecting patch 65693 from commit-queue.

Unexpected failure when processing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 65693, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
693&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=44239&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 65693 from bug 44239.
Rafael Antognolli found in /Users/eseidel/Projects/CommitQueue/WebKit/efl/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/eseidel/Projects/CommitQueue/WebKit/efl/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Rafael Antognolli]

Hi Mikołaj,

Sorry, I didn't see that you had set the Reviewer entry in the ChangeLog with my name. This is not the correct way, since I'm not an official reviewer.

You have to let it without changing (with the "Reviewed by NOBODY (OOPS!)." string), and then when Kenneth set your patch as r+ and cq+, the commit bot would automatically fill it with Kenneth's name.

[Mikołaj Małecki]

Created attachment 65891 [details]
Patch against r66371 with fixes as described in the ticket

Removed the reviewer info from Changelog

[WebKit Commit Bot]

Comment on attachment 65891 [details]
Patch against r66371 with fixes as described in the ticket

Clearing flags on attachment: 65891

Committed r66377: <http://trac.webkit.org/changeset/66377>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.