Bug 16770 - Acid3 expects :visited styled links to restyle on iframe load
Summary: Acid3 expects :visited styled links to restyle on iframe load
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL: http://acid3.acidtests.org/
Keywords: HasReduction
Depends on:
Blocks: Acid3
  Show dependency treegraph
 
Reported: 2008-01-07 07:28 PST by Eric Seidel (no email)
Modified: 2008-02-24 22:15 PST (History)
4 users (show)

See Also:


Attachments
test case (730 bytes, text/html)
2008-02-04 06:21 PST, Robert Blaut
no flags Details
patch (25.03 KB, patch)
2008-02-23 11:23 PST, Darin Adler
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2008-01-07 07:28:34 PST
Acid3 expects :visited styled links to restyle on iframe load

I think we have a dupe of this bug somewhere.

    function () {
      // test 48: :link and :visited
      var iframe = document.getElementById("selectors");
      var number = (new Date()).valueOf();
      var a = document.createElement('a');
      a.appendChild(document.createTextNode('LINKTEST FAILED'));
      a.setAttribute('id', 'linktest');
      a.setAttribute('class', 'pending');
      a.setAttribute('href', iframe.getAttribute('src') + "?" + number);
      document.getElementsByTagName('map')[0].appendChild(a);
      iframe.setAttribute("onload", "document.getElementById('linktest').removeAttribute('class')");
      iframe.src = a.getAttribute("href");
      return 3;
    },
Comment 1 Robert Blaut 2008-02-04 06:21:47 PST
Created attachment 18906 [details]
test case
Comment 2 Darin Adler 2008-02-10 12:28:44 PST
My plan is to make :visited styles update the moment a new URL is added to history. I've been working on this, on and off, for a while. The tricky part is that :visited performance is a significant factor in the browsing speed. So I need to make it fast!
Comment 3 Eric Seidel (no email) 2008-02-12 01:52:50 PST
I think this is actually just bug 17263.  Although I agree this bug would be great to fix too. :)
Comment 4 Robert Blaut 2008-02-12 02:09:31 PST
(In reply to comment #3)
> I think this is actually just bug 17263.  Although I agree this bug would be
> great to fix too. :)
> 

Eric, not really. The bug describe problem with red text "LINKTEST FAILED". With test 80 fired or not the red text is always visible. 
Comment 5 Darin Adler 2008-02-23 09:39:18 PST
(In reply to comment #2)
> My plan is to make :visited styles update the moment a new URL is added to
> history. I've been working on this, on and off, for a while. The tricky part is
> that :visited performance is a significant factor in the browsing speed. So I
> need to make it fast!

Hyatt pointed out that this is a simpler case; not the general problem I mention above. I'm investigating now.
Comment 6 Darin Adler 2008-02-23 11:23:35 PST
Created attachment 19303 [details]
patch
Comment 7 Adam Roben (:aroben) 2008-02-23 12:03:37 PST
Comment on attachment 19303 [details]
patch

+        - remove separate client calls for "standard" and "reload' history

You have mismatched quotes around the word "reload" in your WebKit ChangeLogs.

-// FIXME: <rdar://problem/4880065> - Push Global History into WebCore
-// Once that task is complete, this will go away
-void WebFrameLoaderClient::updateGlobalHistoryForStandardLoad(const KURL& url)
+void WebFrameLoaderClient::updateGlobalHistory(const KURL& url)
 {
     NSURL *cocoaURL = url;
     WebHistoryItem *entry = [[WebHistory optionalSharedHistory] addItemForURL:cocoaURL];
-    String pageTitle = core(m_webFrame.get())->loader()->documentLoader()->title();
-    if (pageTitle.length())
+    const String& pageTitle = core(m_webFrame.get())->loader()->documentLoader()->title();
+    if (!pageTitle.isEmpty())
         [entry setTitle:pageTitle];
 }
  
-// FIXME: <rdar://problem/4880065> - Push Global History into WebCore
-// Once that task is complete, this will go away
-void WebFrameLoaderClient::updateGlobalHistoryForReload(const KURL& url)
-{
-    WebHistory *sharedHistory = [WebHistory optionalSharedHistory];
-    WebHistoryItem *item = [sharedHistory itemForURL:url];
-    if (item)
-        [sharedHistory setLastVisitedTimeInterval:[NSDate timeIntervalSinceReferenceDate] forItem:item];
-}

Is it OK that we're now doing more work than we used to in the reload case?
Comment 8 Darin Adler 2008-02-23 13:01:15 PST
(In reply to comment #7)
> Is it OK that we're now doing more work than we used to in the reload case?

It's definitely OK to do a bit more work. Reload isn't a particularly hot test case.

And in general, it's a bug that reload does anything different. If we want to optimize updating history for cases where the item is already in history, we can do that for both cases.
Comment 9 Sam Weinig 2008-02-24 00:15:15 PST
Comment on attachment 19303 [details]
patch

As noted on irc, you may want to null check m_frame->settings(), but as you noted, it may not be needed.

r=me
Comment 10 Darin Adler 2008-02-24 00:30:51 PST
Committed revision 30549.
Comment 11 mitz 2008-02-24 19:39:02 PST
I think this change has caused all ad-bearing iframes to clutter Safari's History menu when you open a page such as http://www.cnn.com/ or http://facebook.com/.
Comment 12 Darin Adler 2008-02-24 19:57:29 PST
(In reply to comment #11)
> I think this change has caused all ad-bearing iframes to clutter Safari's
> History menu when you open a page such as http://www.cnn.com/ or
> http://facebook.com/.

You're right.

We probably need a new bug report for that. I'd be happy to tackle it right away once we figure out what behavior we want.

Do other browsers use a different list of links for the History menu and the "visited" link coloring? If so, we can easily add this sort of distinction to WebKit.
Comment 13 mitz 2008-02-24 20:11:15 PST
(In reply to comment #12)
> We probably need a new bug report for that.

Filed bug 17526.
Comment 14 Robert Blaut 2008-02-24 22:15:48 PST
(In reply to comment #12)
>
> Do other browsers use a different list of links for the History menu and the
> "visited" link coloring? If so, we can easily add this sort of distinction to
> WebKit.
> 

Opera does it. It uses global.dat file to store history items and vlink4.dat file to store visited link items. Check its documentation: http://www.opera.com/docs/operafiles/