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.
Created attachment 64826 [details] Patch against r65660 with fixes as described in the ticket.
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 @@ > +2010-08-19 MikoÅaj MaÅecki <m.malecki@samsung.com> > + > + 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 <gyuyoung.kim@samsung.com> > > 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());
Created attachment 64956 [details] Patch against r65660 with fixes as described in the ticket. (updated)
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.
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
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.
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.
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.
Created attachment 65693 [details] Patch against r66192 with fixes as described in the ticket. Fixed the checkstyle-reported problem.
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.
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).
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.
Created attachment 65891 [details] Patch against r66371 with fixes as described in the ticket Removed the reviewer info from Changelog
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>
All reviewed patches have been landed. Closing bug.