Bug 71052 - [GTK] Visited links of a page group should be removed when clearing back/forward list
Summary: [GTK] Visited links of a page group should be removed when clearing back/forw...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-27 12:27 PDT by Zan Dobersek
Modified: 2011-11-02 11:11 PDT (History)
2 users (show)

See Also:


Attachments
Patch (40.53 KB, patch)
2011-10-28 13:06 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Updated patch (40.89 KB, patch)
2011-10-28 13:56 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2011-10-27 12:27:15 PDT
Currently, when clearing back/forward list, visited links are not purged from page's page group.

This has been noted when comparing expected results across ports of tests that included links to previously loaded tests.
Comment 1 Zan Dobersek 2011-10-28 13:06:40 PDT
Created attachment 112898 [details]
Patch

This patch adds removing of visited links of a page group when clearing back-forward lists. Also included are rebaselines for tests that need them. Neatly enough, this problem also seemed to be the cause of most of the flaky tests.
Comment 2 Martin Robinson 2011-10-28 13:20:14 PDT
Comment on attachment 112898 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112898&action=review

Awesome! I have a couple comments below:

> Source/WebKit/gtk/webkit/webkitwebbackforwardlist.cpp:463
> +    if (!backForwardList)
> +        return;
> +
> +    WebCore::Page* page = backForwardList->page();
> +    if (page && page->groupPtr())

I think it's okay to do this here since other ports do it, but we should update the documentation to state that when a list is cleared it also clears the list of visisted links.

> LayoutTests/platform/gtk/fast/block/margin-collapse/010-expected.txt:31
> -        RenderInline {A} at (0,0) size 49x19 [color=#551A8B]
> +        RenderInline {A} at (0,0) size 49x19 [color=#0000EE]
>            RenderText {#text} at (259,0) size 49x19

Can you also run these tests in WebKit2? I'm surprised that they weren't failing before there.
Comment 3 Zan Dobersek 2011-10-28 13:55:30 PDT
Comment on attachment 112898 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112898&action=review

>> Source/WebKit/gtk/webkit/webkitwebbackforwardlist.cpp:463
>> +    if (page && page->groupPtr())
> 
> I think it's okay to do this here since other ports do it, but we should update the documentation to state that when a list is cleared it also clears the list of visisted links.

Noted & done.

>> LayoutTests/platform/gtk/fast/block/margin-collapse/010-expected.txt:31
>>            RenderText {#text} at (259,0) size 49x19
> 
> Can you also run these tests in WebKit2? I'm surprised that they weren't failing before there.

Running the tests in WebKit2 is actually where I noticed that Gtk port holds onto visited links and is the only port to do so. WebKitTestRunner is structured in a way that the usage of WebKit2 does not vary that much between the ports, so the other ports didn't have these problems while these errors were popping up for me. With this patch these tests would also start passing when running the tests in WebKit2.
Comment 4 Zan Dobersek 2011-10-28 13:56:35 PDT
Created attachment 112907 [details]
Updated patch
Comment 5 Martin Robinson 2011-10-28 14:26:40 PDT
(In reply to comment #3)

> Running the tests in WebKit2 is actually where I noticed that Gtk port holds onto visited links and is the only port to do so. WebKitTestRunner is structured in a way that the usage of WebKit2 does not vary that much between the ports, so the other ports didn't have these problems while these errors were popping up for me. With this patch these tests would also start passing when running the tests in WebKit2.

Oh! So they weren't passing before on WebKit2. I thought they were because they were not skipped. Alex, were these tests passing for you before?
Comment 6 Martin Robinson 2011-10-29 12:53:51 PDT
Comment on attachment 112907 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112907&action=review

Looks good to me, but let's wait to land this until Alex can take can answer my question about WebKit2.

> Source/WebKit/gtk/webkit/webkitwebbackforwardlist.cpp:451
> + * Also removed is the list of visited links, removing stylization of such elements in page.

Nit on the English:

This method also clears the list of visited links which means that all links appear unvisited.
Comment 7 Alejandro G. Castro 2011-11-02 05:43:42 PDT
(In reply to comment #5)
> (In reply to comment #3)
> 
> > Running the tests in WebKit2 is actually where I noticed that Gtk port holds onto visited links and is the only port to do so. WebKitTestRunner is structured in a way that the usage of WebKit2 does not vary that much between the ports, so the other ports didn't have these problems while these errors were popping up for me. With this patch these tests would also start passing when running the tests in WebKit2.
> 
> Oh! So they weren't passing before on WebKit2. I thought they were because they were not skipped. Alex, were these tests passing for you before?

Just tested them with last revision and they are failing. I do not recall these tests specifically when I created the WK2 Skipped file, it is weird, I suppose they were passing at that point. 

...

Just checked trac and here it is the explanation, a change in the visited links that was rebaselined by phil and hided the problem:

http://trac.webkit.org/changeset/97736
http://trac.webkit.org/changeset/97861

These commits link with commits 97602 and 97596, that were causing the problem.

EFL fixed a similar issue apparently

http://trac.webkit.org/changeset/97797

This situation recalls me when we did not have the bots, hopefully we could have a WK2 at some point.

I hope this helps.
Comment 8 Martin Robinson 2011-11-02 11:11:04 PDT
Committed r99082: <http://trac.webkit.org/changeset/99082>