Bug 20952 – QtWebKit needs the old history pull model for API Compability

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Holger Freyther]

In r30840 the global history was replaced with PageGroup and pushing the visited links into WebCore. While this new model brings a great speed improvement to visited link coloring it breaks the pull model of the Qt API. For API Compability QtWebKit needs to have a compat mode.

[Holger Freyther]

Created attachment 23590 [details]
Add the GlobalHistory back for QtWebKit

Optionally have a global history fallback which is enabled for the qtwebkit platform.

[Eric Seidel (no email)]

Comment on attachment 23590 [details]
Add the GlobalHistory back for QtWebKit

Really?  WebCore shouldn't have any API.

[Holger Freyther]

(In reply to comment #2)
> (From update of attachment 23590 [details] [edit])
> Really?  WebCore shouldn't have any API.

I think there is a misunderstanding. The QtWebKit API for history is very basic and only has a "bool contains(const QUrl&)" method. This used to match perfectly with the internal model of WebCore. The internal model was changed to the better (PageGroup and pushing the visited links for a group into WebCore and hashing them) and the semantic of the QtWebKit API does not work anymore. For QtWebKit compability reasons it is required to have a fallback (which is #ifdef'ed and not used for the other platforms).

So the QtWebKit API broke due changes in WebCore and we need a way to have the API to work as in Qt4.4.

[Darin Adler]

I'm really sad this got exposed in the QtWebKit API. I've been talking about this change for a long time, and requiring actual URL objects to be made for every link to check visited status really kills performance!

Anyway, I'll review soon.

[Darin Adler]

Comment on attachment 23590 [details]
Add the GlobalHistory back for QtWebKit

I'm disappointed that this makes the visited link code more complicated -- those two new Document functions have interfaces that are hard to understand with a mysterious boolean result and a fixed size buffer in the header, while visitedLinkHash is so much easier to understand!

Can we remove this in the future? Would you be willing to change the Qt API?

This is an area we are likely to want to refine further -- I had been planning to change the algorithm to be faster and not require putting the URL into a buffer. The legacy dependency here is going to make it hard to make that improvement.

+// We have to fallback to the old historyContains behaviour for the Qt4.4 API

Should be "fall back", the verb, rather than "fallback", the noun.

 unsigned Document::visitedLinkHash(const AtomicString& attributeURL) const
 {
-    const UChar* characters = attributeURL.characters();
     unsigned length = attributeURL.length();
     if (!length)
         return 0;

It doesn't make sense to put this into a local variable when it's only used once. I also don't think we need a special case for 0 in this function any more. The other functions already have special cases.

+    return AlreadyHashed::avoidDeletedValue(result ?
+                                            attributeURL.string().impl()->hash() :
+                                            StringImpl::computeHash(buffer.data(), buffer.size()));

This is not our usual formatting. Maybe you need to use another local variable to avoid having this deeply indented ? : expression.

     if (hasColonSlashSlash && !needsTrailingSlash(characters, length))
-        return AlreadyHashed::avoidDeletedValue(attributeURL.string().impl()->hash());
-
-    Vector<UChar, 512> buffer;
+        return true;

I think this needs to be return false, not true.

+        result.append(characters, length);
+        result.append('/');
+        return false;

I think this needs to be return true, not false.

-    return AlreadyHashed::avoidDeletedValue(StringImpl::computeHash(buffer.data(), buffer.size()));
+    return false;

I think this needs to be return true, not false.

How did you test?

[Holger Freyther]

(In reply to comment #5)

> How did you test?

I don't remember, but the things you spotted imply badly or not at all at least no tests on OSX/Win were done.

[Holger Freyther]

Created attachment 26011 [details]
Add the GlobalHistory back for QtWebKit.

Another patch working on the LinkHash and CSSStyleSelector:
  - Add a method to LinkHash to resolve the URL. In contrast to the previous attempt the API is not too ugly

  - I'm aware how ugly this is, specially of having a #ifdef in the CSSStyleSelector and having a WebKit type there. For Qt4.5 it is needed though. Maybe it is time to add a "bridge" for QtWebKit as well and some hints to reduce the ugliness are much appreciated.

  - The limited tests of Gtk+ still pass and a test case for QtWebKit is added

[Simon Hausmann]

Comment on attachment 26011 [details]
Add the GlobalHistory back for QtWebKit.

Darin, could you take another look at Holger's latest patch?

FWIW I agree about the fact that it's very sad that this became public API in Qt and I'm all in favour of deprecating it and having a minimalistic API instead that completely hides the actual history tracking.

Thanks :)

[Darin Adler]

Comment on attachment 26011 [details]
Add the GlobalHistory back for QtWebKit.

r=me

[Eric Seidel (no email)]

Did this ever land?  If it's not really needed by Qt, lets just chuck this patch! :)

[Simon Hausmann]

Landed in r43052

[Dimitri Glazkov (Google)]

This broke Chromium build :-\.

I think I'll just plug it for now by defining a no-opish visitedURL, but I agree with everybody here -- let's API this puppy.

[Dimitri Glazkov (Google)]

I opted for creating a new bug 25485 to track Chromium-specific mods. Re-closing.