[webkit-reviews] review denied: [Bug 78358] REGRESSION (Safari 5.0.5 - 5.1): Link is not colored as visited if default port number is present : [Attachment 180344] Patch

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied webkitlearner at gmail.com's request
for review:
Bug 78358: REGRESSION (Safari 5.0.5 - 5.1): Link is not colored as visited if
default port number is present
https://bugs.webkit.org/show_bug.cgi?id=78358

Attachment 180344: Patch
https://bugs.webkit.org/attachment.cgi?id=180344&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=180344&action=review


> Source/WebCore/ChangeLog:8
> +	   Link is not colored as visited if default port number is present
> +	   https://bugs.webkit.org/show_bug.cgi?id=78358
> +	   
> +
> +	   Not reviewed.
> +

Weird extra white space.

Also, we normally leave the "Reviewed by NOBODY (OOPS!)" in patches until they
are reviewed.

> Source/WebCore/ChangeLog:11
> +	   Trying to get the KURL of the url which we got from fastGetAttribute
and getting the AtmoicString of the KURL by adding AtomicString API in KURL
class.
> +	   The URL we are getting from fastGetAttribute returns the url with
port number, as we have added an api in the KURL class which deletes the port
number
> +	   when parsed, in the same way we have to give the parsed URL to
vistedlinkHased so that the URL will be marked as visted.

This description is verbose and confusing.

This fix is small; A sentence would suffice.

> Source/WebCore/ChangeLog:14
> +
> +	   No new tests (OOPS!).
> +

This is a deal breaker...  a change like this needs tests.

This has already been mentioned before.  Please stop submitting patches without
tests.

> Source/WebCore/html/HTMLAnchorElement.h:155
> +    if (!m_cachedVisitedLinkHash) {
> +	   KURL vistedURL =
document()->completeURL(fastGetAttribute(HTMLNames::hrefAttr).string());
> +	   m_cachedVisitedLinkHash =
WebCore::visitedLinkHash(document()->baseURL(), vistedURL.getAtomicString());
> +    }

All you are doing here is jumping through a lot of hoops with various string
classes to try and preserve the :80 on this one link so in case the :80 form is
visited it would show as purple.

Many Webkit-embedding user agents strip default ports in their requests.  e.g.
clicking a link to "http://webkit.org:80/" takes you to "http://webkit.org/" 
So "visiting" webkit.org:80 doesn't enter webkit.org:80 into the visited link
hashes, it just adds webkit.org.

Did you bother to try in, say, Safari?

As originally reported, I assumed this bug was about the equivalence of
http://webkit.org/ and http://webkit.org:80/ and how if you visit one, the
other should also appear visited.  This is what Firefox does, for example, and
what I think we should do.

Instead of jumping through these hopes to preserve :80 in the hash, what we
should be doing is realizing that :80 is the default port and drop it.

> Source/WebCore/platform/KURL.cpp:594
> +AtomicString KURL::getAtomicString() const
> +{
> +    return AtomicString(string());
> +}
> +

We are not going to add this.  I think you misunderstand how all the string
classes relate.