Bug 71052

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

Created attachment 112907 [details] Updated patch

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

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

Committed r99082: <http://trac.webkit.org/changeset/99082>