Bug 28823 – [GTK] New events (pageshow and pagehide) tests failing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Gustavo Noronha (kov)]

I am openning this bug report to track this problem.

fast/events/pageshow-pagehide-on-back-cached.html makes me think we do something quite incorrectly somewhere, the other tests seem to be hitting small defficiencies of DRT.

[Jan Alonzo]

Created attachment 38782 [details]
Patch v1

[Gustavo Noronha (kov)]

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?

[Jan Alonzo]

(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".

[Eric Seidel (no email)]

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.

[Gustavo Noronha (kov)]

(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! =)

[Xan Lopez]

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!

[Eric Seidel (no email)]

Ping?

[Gustavo Noronha (kov)]

Created attachment 41308 [details]
updated patch

[Gustavo Noronha (kov)]

(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) ()
...

[Alejandro G. Castro]

(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.

[Gustavo Noronha (kov)]

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

[Gustavo Noronha (kov)]

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

[Martin Robinson]

Closing, since these tests have been unskipped.