Bug 27145 - [Gtk][REGRESSION] subframe-navigate-during-main-frame-load.html fails after r45615
Summary: [Gtk][REGRESSION] subframe-navigate-during-main-frame-load.html fails after r...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2009-07-10 00:11 PDT by Jan Alonzo
Modified: 2009-07-18 15:55 PDT (History)
1 user (show)

See Also:


Attachments
patch (3.06 KB, patch)
2009-07-13 01:00 PDT, Jan Alonzo
no flags Details | Formatted Diff | Diff
updated patch (2.90 KB, patch)
2009-07-15 07:07 PDT, Jan Alonzo
no flags Details | Formatted Diff | Diff
Updated to use result->str instead of a temp var (3.01 KB, patch)
2009-07-17 18:17 PDT, Jan Alonzo
gns: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Alonzo 2009-07-10 00:11:26 PDT
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.
Comment 1 Jan Alonzo 2009-07-13 01:00:46 PDT
Created attachment 32651 [details]
patch
Comment 2 Gustavo Noronha (kov) 2009-07-14 14:43:44 PDT
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.
Comment 3 Jan Alonzo 2009-07-15 07:07:16 PDT
Created attachment 32782 [details]
updated patch
Comment 4 Jan Alonzo 2009-07-15 07:08:19 PDT
(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.
Comment 5 Gustavo Noronha (kov) 2009-07-15 08:06:52 PDT
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
Comment 6 Gustavo Noronha (kov) 2009-07-15 08:09:37 PDT
(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.
Comment 7 Adam Barth 2009-07-16 16:11:43 PDT
Comment on attachment 32782 [details]
updated patch

Please post an updated patch with the nits addressed.  Removing from commit queue.
Comment 8 Jan Alonzo 2009-07-17 18:17:59 PDT
Created attachment 32995 [details]
Updated to use result->str instead of a temp var
Comment 9 Gustavo Noronha (kov) 2009-07-18 15:40:42 PDT
Comment on attachment 32995 [details]
Updated to use result->str instead of a temp var

r=me
Comment 10 Jan Alonzo 2009-07-18 15:55:07 PDT
(In reply to comment #9)
> (From update of attachment 32995 [details])
> r=me

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