[webkit-reviews] review denied: [Bug 20952] QtWebKit needs the old history pull model for API Compability : [Attachment 23590] Add the GlobalHistory back for QtWebKit

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Holger Freyther
<freyther at handhelds.org>'s request for review:
Bug 20952: QtWebKit needs the old history pull model for API Compability
https://bugs.webkit.org/show_bug.cgi?id=20952

Attachment 23590: Add the GlobalHistory back for QtWebKit
https://bugs.webkit.org/attachment.cgi?id=23590&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
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?