[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 bugzilla-daemon at webkit.org
Fri Oct 3 13:06:04 PDT 2008

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

Attachment 23590: Add the GlobalHistory back for QtWebKit

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

+// 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
-    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
+    return false;

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

How did you test?

More information about the webkit-reviews mailing list