Bug 16770 – Acid3 expects :visited styled links to restyle on iframe load

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

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;
    },

[Robert Blaut]

Created attachment 18906 [details]
test case

[Darin Adler]

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!

[Eric Seidel (no email)]

I think this is actually just bug 17263.  Although I agree this bug would be great to fix too. :)

[Robert Blaut]

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

[Darin Adler]

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

[Darin Adler]

Created attachment 19303 [details]
patch

[Adam Roben (:aroben)]

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?

[Darin Adler]

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

[Sam Weinig]

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

[Darin Adler]

Committed revision 30549.

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

[Darin Adler]

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

(In reply to comment #12)
> We probably need a new bug report for that.

Filed bug 17526.

[Robert Blaut]

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