[Webkit-unassigned] [Bug 78358] REGRESSION (Safari 5.0.5 - 5.1): Link is not colored as visited if default port number is present

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=78358


Brady Eidson <beidson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #180344|review?                     |review-
               Flag|                            |




--- Comment #23 from Brady Eidson <beidson at apple.com>  2012-12-20 08:54:29 PST ---
(From update of attachment 180344)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.