Bug 28823

Created attachment 38782 [details] Patch v1

Comment on attachment 38782 [details] Patch v1 > + /** > + * WebKitWebSettings:enable-page-cache > + * > + * Whether to enable the page cache > + * > + * Since 1.1.14 > + */ > + g_object_class_install_property(gobject_class, > + PROP_ENABLE_PAGE_CACHE, > + g_param_spec_boolean("enable-page-cache", > + _("Enable Page Cache"), > + _("Whether to enable the page cache"), > + TRUE, > + flags)); If this is causing trouble, why would we enable this by default? Better to have a comment saying that we plan to enable this by default in the future? API-wise, except for the default, fine with me. =) > - g_str_equal(g_utf8_strdown(strValue, -1), "true"), > + g_str_equal(g_utf8_strdown(strValue, -1), "true") > + || g_str_equal(g_utf8_strdown(strValue, -1), "1"), > NULL); > } else if (G_VALUE_HOLDS_INT(&propValue)) { > std::string str(strValue); Weird. Why do we need this now?

(In reply to comment #2) > (From update of attachment 38782 [details]) > > + /** > > + * WebKitWebSettings:enable-page-cache > > + * > > + * Whether to enable the page cache > > + * > > + * Since 1.1.14 > > + */ > > + g_object_class_install_property(gobject_class, > > + PROP_ENABLE_PAGE_CACHE, > > + g_param_spec_boolean("enable-page-cache", > > + _("Enable Page Cache"), > > + _("Whether to enable the page cache"), > > + TRUE, > > + flags)); > > If this is causing trouble, why would we enable this by default? Better to have > a comment saying that we plan to enable this by default in the future? Ok. I thought it's just causing trouble in the DRT but ok. > > - g_str_equal(g_utf8_strdown(strValue, -1), "true"), > > + g_str_equal(g_utf8_strdown(strValue, -1), "true") > > + || g_str_equal(g_utf8_strdown(strValue, -1), "1"), > > NULL); > > } else if (G_VALUE_HOLDS_INT(&propValue)) { > > std::string str(strValue); > > Weird. Why do we need this now? 1 still evaluates to True and one of the affected test is using 1 for "true".

Comment on attachment 38782 [details] Patch v1 LGTM. Jan is a committer so setting cq-, he can set cq+ himself if he'd rather have the bot land it for him.

(In reply to comment #3) > > If this is causing trouble, why would we enable this by default? Better to have > > a comment saying that we plan to enable this by default in the future? > > Ok. I thought it's just causing trouble in the DRT but ok. For the record, I'm OK with keeping this set to TRUE, so feel free to go ahead and commit! =)

Comment on attachment 38782 [details] Patch v1 >+ /** >+ * WebKitWebSettings:enable-page-cache >+ * >+ * Whether to enable the page cache Please explain what is the page cache in the documentation. >+ * >+ * Since 1.1.14 .15 now :) >+ */ >+ g_object_class_install_property(gobject_class, >+ PROP_ENABLE_PAGE_CACHE, >+ g_param_spec_boolean("enable-page-cache", >+ _("Enable Page Cache"), >+ _("Whether to enable the page cache"), >+ TRUE, >+ flags)); >+ >+ > g_type_class_add_private(klass, sizeof(WebKitWebSettingsPrivate)); > } > >+ // From Mac: The back/forward cache is causing problems due to layouts >+ // during transition from one page to another. So, turn it off for now, >+ // but we might want to turn it back on some day. >+ g_object_set(G_OBJECT(settings), >+ "enable-page-cache", FALSE, >+ NULL); >+ You don't need the G_OBJECT cast. >+ g_str_equal(g_utf8_strdown(strValue, -1), "true") >+ || g_str_equal(g_utf8_strdown(strValue, -1), "1"), No need to strdown that :) r=me with all those changes, thanks!

Ping?

Created attachment 41308 [details] updated patch

(In reply to comment #8) > Created an attachment (id=41308) [details] > updated patch I updated the patch with the changes suggested by Xan, but am not marking for review since it's Jan's patch, and when testing I seem to be getting this when I git back: GLib-GObject-CRITICAL **: g_object_ref: assertion `G_IS_OBJECT (object)' failed aborting... Program received signal SIGTRAP, Trace/breakpoint trap. IA__g_logv (log_domain=0x7ffff47acdbc "GLib-GObject", log_level=G_LOG_LEVEL_CRITICAL, format=0x7ffff411f86d "%s: assertion `%s' failed", args1=0x7fffffffc080) at gmessages.c:555 555 g_private_set (g_log_depth, GUINT_TO_POINTER (depth)); (gdb) bt #0 IA__g_logv (log_domain=0x7ffff47acdbc "GLib-GObject", log_level=G_LOG_LEVEL_CRITICAL, format=0x7ffff411f86d "%s: assertion `%s' failed", args1=0x7fffffffc080) at gmessages.c:555 #1 0x00007ffff40cce83 in IA__g_log (log_domain=0x7ffff3e68a00 "", log_level=-205296696, format=0x41 <Address 0x41 out of bounds>) at gmessages.c:569 #2 0x00007ffff4787402 in IA__g_object_ref (_object=<value optimized out>) at gobject.c:2339 #3 0x00007ffff72c2699 in WebCore::FrameLoader::setPolicyDocumentLoader(WebCore::DocumentLoader*) () from /home/kov/src/WebKit/WebKitBuild/Release/.libs/libwebkit-1.0.so.2 #4 0x00007ffff72cd8bd in WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WTF::PassRefPtr<WebCore::FormState>) () from /home/kov/src/WebKit/WebKitBuild/Release/.libs/libwebkit-1.0.so.2 #5 0x00007ffff72d1790 in WebCore::FrameLoader::loadItem(WebCore::HistoryItem*, WebCore::FrameLoadType) () from /home/kov/src/WebKit/WebKitBuild/Release/.libs/libwebkit-1.0.so.2 #6 0x00007ffff72d38f8 in WebCore::HistoryController::recursiveGoToItem(WebCore::HistoryItem*, WebCore::HistoryItem*, WebCore::FrameLoadType) () from /home/kov/src/WebKit/WebKitBuild/Release/.libs/libwebkit-1.0.so.2 #7 0x00007ffff73370b9 in WebCore::Page::goToItem(WebCore::HistoryItem*, WebCore::FrameLoadType) () ...

(In reply to comment #9) > (In reply to comment #8) > > Created an attachment (id=41308) [details] [details] > > updated patch > Reviewing this patch taking into account part of it has been added in the cache API bug: bug 24001.

Both parts of this patch are now landed. We still need to enable a bunch of tests, though.

Both parts of this patch are now landed. We still need to enable a bunch of tests, though.

Closing, since these tests have been unskipped.