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

• Component: History
• Status: NEW

Hi,

1.go to this URL: http://spe.mobilephone.net/wit/xhtmlv2/linkanchor.xhtml

2.click on the http, port 80, https, port 443  links.

3.After the clicking the link another page will be opened.

4.click back button and check the particular link now.

5.link color(before clicking the link it is blue) is not changed to visited color(after clicking the link and come back and see.Link color will be changed to another color as per the default css defined.It will not be blue in color).

Thanks,
webkitlearner.

[Alexey Proskuryakov]

Eric, this sounds like fallout from the change in bug 54090. Could you take a look?

<a href="http://www.slowpokeproductions.com:80/wit/%77ittarget1b.xhtml">http, port 80</a><br/>
<a href="https://www.slowpokeproductions.com:443/wit/%77ittarget1b.xhtml">https, port 443</a><br/>

[Eric Seidel (no email)]

I would not be surprised were this the case.

I don't understand the reproduction.  I assume I need to use Safari to test?

[Alexey Proskuryakov]

Yes, this works in Chrome. To reproduce,

1. Open http://spe.mobilephone.net/wit/xhtmlv2/linkanchor.xhtml
2. Click on "http, port 80" link in the middle of the page.
3. Go back.

Expected: it's highlighted as visited.
Actual: it's not.

[Eric Seidel (no email)]

I can verify that this is borked in Safari 5.1.  Chrome, as noted, works fine.

[Eric Seidel (no email)]

It's unclear to me why clicking on the :80 link would not mark the non :80 one as visited.  It seems the visited subsystem should use canonical urls.  From a user's perspective, the links are identical.

[Eric Seidel (no email)]

I'm happy to advise someone in fixing this, but I don't plan on working on this any time soon.  We can also just roll out bug 54090.

Created attachment 131171 [details]
It has the modified and original files , if you check the difference between this two files you can get the patch of 78358 bug

Hi,

When you click on the link 

<a xmlns="http://www.w3.org/1999/xhtml" href="http://www.slowpokeproductions.com:80/wit/%77ittarget1b.xhtml">http, port 80</a>

The KURL object is created for the above link and the url is being parsed and the default port numbers is being removed and the link is loaded and the same link is being stored in the Hashtable.

When you press the back button in the  HTMLAnchorElement.h the following api 

inline LinkHash HTMLAnchorElement::visitedLinkHash() const

is being called which inturn calls WebCore::visitedLinkHash; to which we are passing the base URL and the AtomicString of fastGetAttribute(HTMLNames::hrefAttr). So the url has the default ports and this url is being checked in the hashmap , as it is not stored we are getting the link unvisted and showing blue color.

So we need to send the same type of URL(without ports) for the LinkHash to check for the url visted or not.

So i have the proposed patch where you create the KURL object from the fastGetAttribute(HTMLNames::hrefAttr) and convert it in to AtomicString from the KURL object and send it which solves the issue.

[WebKit Review Bot]

Attachment 131171 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 131171 [details]
It has the modified and original files , if you check the difference between this two files you can get the patch of 78358 bug

The attachment is a RAR archive.

Would you be willing to contribute a patch, as described in <http://www.webkit.org/coding/contributing.html>?

Hi,

Original API
inline LinkHash HTMLAnchorElement::visitedLinkHash() const
{
    if (!m_cachedVisitedLinkHash)
        m_cachedVisitedLinkHash = WebCore::visitedLinkHash(document()->baseURL(), fastGetAttribute(HTMLNames::hrefAttr));
    return m_cachedVisitedLinkHash; 
}

Modified API
inline LinkHash HTMLAnchorElement::visitedLinkHash() const
{
	if (!m_cachedVisitedLinkHash)
        m_cachedVisitedLinkHash = WebCore::visitedLinkHash(document()->baseURL(), AtomicString(document()->completeURL(fastGetAttribute(HTMLNames::hrefAttr).string()).string().utf8().data()));
    return m_cachedVisitedLinkHash; 
}

Not able to download the webkit code as the server is slow , to submit the patch currently im working on nightly build code.

Kindly let me know any other way to submit the patch.

~Webkit Learner

Created attachment 134054 [details]
Proposed patch

Hi,

I have attached the proposed patch kindly review it and let me know the reviews on the proposed patch.

~Webkit Learner

[Eric Seidel (no email)]

The patch may be correct, but won't apply since it wasn' created from teh root of the repository (as our patch tools will automatically do for you).

See http://www.webkit.org/coding/contributing.html

Create the patch
The easiest way to create a patch is to run Tools/Scripts/webkit-patch upload. This will upload your current Subversion diff (or Git diff if you use git) to the issue tracking system and mark it as ready for review.

Created attachment 137375 [details]
proposed patch

kindly check the patch and give your feedback on it

[Alexey Proskuryakov]

Please do follow guidelines at <http://www.webkit.org/coding/contributing.html>. Patches need to come with regression tests, ChangeLogs, and should be marked r? to properly appear in review queue.

Created attachment 137385 [details]
Patch

[WebKit Review Bot]

Attachment 137385 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/html/HTMLAnchorElement.h:152:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 137473 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 137473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137473&action=review

> Source/WebCore/html/HTMLAnchorElement.h:152
> +        KURL vistedURL = document()->completeURL(fastGetAttribute(HTMLNames::hrefAttr).string());
> +        m_cachedVisitedLinkHash = WebCore::visitedLinkHash(document()->baseURL(), AtomicString(vistedURL.string().utf8().data()));

All these conversions make no sense. :)  You're converting from an AtomicString, to a STring, to a KURL, then to a STring, CSTring, and char* only to go back to an AtomicSTring! :p

Dear Eric,

The conversion of  AtomicString, To a STring , to a KURL is the key which solved this issue else you need to remove the defaultportscheme api and calling this api  in the KURL.cpp

When you click the URL you get "http://www.slowpokeproductions.com:80/wit/%77ittarget1b.xhtml" and this URL is sent to check the VistedLinkHash. In the VistedLinkHash link is stored as http://www.slowpokeproductions.com/wit/%77ittarget1b.xhtml,because after parsing the URL, which is done in the KURL.cpp :80 is removed and this URL is stored in the VistedLinkHash .When you click back you are checking "http://www.slowpokeproductions.com:80/wit/%77ittarget1b.xhtml" is present in the VistedLinkHash or not.So for that reason you need to parse the AtomicString.If you start parsing the AtomicString you end up writing another KURL.cpp file. 

For this purpose i have given the url to document->completeURL() which returns me the parsed URL.to give the url to document->completeURL() i strin String.Thats why i converted AtomicString to STring.

I got the parsed KURL object.but i need to send the AtomicString to visitedLinkHash api. There is no constructor in AtomicString which accepts the KURL object.As i found constructor which accepts char* in AtomicString, So i converted KURL to STring,CString and char* and created AtomicString which is parsed and doesnot have :80 in the URL.

Kindly check this solution once again or suggest me better way to achieve this.

~webkitlearner

(In reply to comment #19)
> (From update of attachment 137473 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137473&action=review
> 
> > Source/WebCore/html/HTMLAnchorElement.h:152
> > +        KURL vistedURL = document()->completeURL(fastGetAttribute(HTMLNames::hrefAttr).string());
> > +        m_cachedVisitedLinkHash = WebCore::visitedLinkHash(document()->baseURL(), AtomicString(vistedURL.string().utf8().data()));
> 
> All these conversions make no sense. :)  You're converting from an AtomicString, to a STring, to a KURL, then to a STring, CSTring, and char* only to go back to an AtomicSTring! :p

[Brady Eidson]

<rdar://problem/11368052>

Created attachment 180344 [details]
Patch

[Brady Eidson]

Comment on attachment 180344 [details]
Patch

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.

[Alexey Proskuryakov]

*** Bug 169738 has been marked as a duplicate of this bug. ***