[Gtk][REGRESSION] subframe-navigate-during-main-frame-load.html fails after r45615

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Jan Alonzo]

Changeset: http://trac.webkit.org/changeset/45615

Failing test:
fast/loader/subframe-navigate-during-main-frame-load.html

Test diff:
 ============== Back Forward List ==============
-        (file test):fast/loader/subframe-navigate-during-main-frame-load.html  **nav target**
-curr->  (file test):fast/loader/resources/subframe-navigate-during-main-frame-load2.html  **nav target**
+        file:///home/jmalonzo/OpenSource/WebKit/LayoutTests/fast/loader/subframe-navigate-during-main-frame-load.html  **nav target**
+curr->  file:///home/jmalonzo/OpenSource/WebKit/LayoutTests/fast/loader/resources/subframe-navigate-during-main-frame-load2.html  **nav target**
             data:text/html,%3Cbody%20onload=%22layoutTestController.notifyDone();%22%3E%3C/body%3E (in frame "subframe")
 ===============================================

Doing a similar fix as the one in r45615 in Gtk's DRT should fix this.

[Jan Alonzo]

Created attachment 32651 [details]
patch

[Gustavo Noronha (kov)]

Comment on attachment 32651 [details]
patch

> +    const gchar* uri = webkit_web_history_item_get_uri(item);
> +    if (g_strcmp0(g_uri_parse_scheme(uri), "file") == 0) {

You are leaking the return of g_uri_parse_scheme here.

> +        static gchar* layoutTestString = g_strdup("/LayoutTests/");
> +        static gchar* fileTestString = g_strdup("(file test):");

These are also leaked. There should be no need to g_strdup here. Just use the static strings instead?

> +        result = g_string_append(result, pos + strlen(layoutTestString));

I'd suggest using sizeof("/LayoutTests/") instead of strlen.

[Jan Alonzo]

Created attachment 32782 [details]
updated patch

[Jan Alonzo]

(In reply to comment #3)
> Created an attachment (id=32782) [details]
> updated patch

I made the updates except for the strlen -> sizeof change. May I ask why? Thanks.

[Gustavo Noronha (kov)]

Comment on attachment 32782 [details]
updated patch

> +        gchar* resultStr = g_string_free(result, FALSE);
> +        printf("%s", resultStr);
> +        g_free(resultStr);

I would prefer the following:

printf("%s", result->str);
g_string_free(result, TRUE);

But I am fine either way. If you feel like changing it, please do.

r=me

[Gustavo Noronha (kov)]

(In reply to comment #4)
> (In reply to comment #3)
> > Created an attachment (id=32782) [details] [details]
> > updated patch
> 
> I made the updates except for the strlen -> sizeof change. May I ask why?
> Thanks.

Err... forgot to answer this one. This is more of a nit-pick. A strlen will waste runtime - we already know the size of the string in build time, since it is static and won't change. sizeof is compile-time, so no time wasted counting chars.

Notice that we would have to use sizeof on the actual static string, not on the variable. But, as I said, this is a nitpick, if you feel like using strlen, there should be no problem, since this is not performance-critical code.

[Adam Barth]

Comment on attachment 32782 [details]
updated patch

Please post an updated patch with the nits addressed.  Removing from commit queue.

[Jan Alonzo]

Created attachment 32995 [details]
Updated to use result->str instead of a temp var

[Gustavo Noronha (kov)]

Comment on attachment 32995 [details]
Updated to use result->str instead of a temp var

r=me

[Jan Alonzo]

(In reply to comment #9)
> (From update of attachment 32995 [details])
> r=me

Landed as http://trac.webkit.org/changeset/46096. Thanks.