CSSStyleSelector should pass through origin information when determined if link visited

• Component: CSS
• Status: RESOLVED WONTFIX

[Daniel Clifford]

For very privacy aware users, a SafeHistory-like options should be available for WebKit, i.e. it should be possible to make sure that links are only shown as visited if they have already been visited from a link embedded in a page with the same origin. In order to do this, WebKit, specifically in CSSStyleSelector, needs to provide additional information to the hash function used to compute a URL's hash in the visited history.

[Daniel Clifford]

Created attachment 53163 [details]
Pass through origin information about embedding page to be used for SafeHistory-style visited link display

[Daniel Clifford]

Created attachment 53167 [details]
Fix broken comment still left in previous patch

[Daniel Clifford]

Created attachment 53168 [details]
Actually mark the patch attachment as a patch

[Jeremy Orlow]

Comment on attachment 53163 [details]
Pass through origin information about embedding page to be used for SafeHistory-style visited link display

If you want something reviewed, set the r? flag.  If you want it landed if it's good, set the commit-queue? flag.

There are probably better reviewers than me.  Here are some drive by comments, though.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 49b8d1f..18a9231 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,32 @@
> [phone]  Daniel Clifford  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +		Pass origin information to the hash function used to compute a

Don't use tabs.  Follow the following style:

"""
subject
bug url

longer
description
here
""""

> +		the hash for a visited url so that it's possible to implement
> +		a SafeHistory-like policy for only displaying links as visited
> +		if they are embedded in a page with the same-origin.
> +
> +		https://bugs.webkit.org/show_bug.cgi?id=37443
> +
> +        * platform/chromium/ChromiumBridge.h:
> +        * platform/chromium/LinkHashChromium.cpp:
> +        (WebCore::visitedLinkHash):
> +        * platform/LinkHash.cpp:
> +        (WebCore::visitedLinkHash):
> +        * platform/LinkHash.h:
> +        * platform/chromium/ChromiumBridge.h:
> +        * platform/chromium/LinkHashChromium.cpp:
> +        (WebCore::visitedLinkHash):
> +        * page/PageGroup.cpp:
> +        (WebCore::PageGroup::addVisitedLink):
> +        * dom/Document.cpp:
> +        (WebCore::Document::safeHistoryOrigin):
> +        * dom/Document.h:
> +        * css/CSSStyleSelector.cpp:
> +        (WebCore::CSSStyleSelector::SelectorChecker::checkPseudoState):
> +        (WebCore::CSSStyleSelector::SelectorChecker::visitedStateChanged):
> +
>  2010-03-30  Dumitru Daniliuc  <[e-mail]>
>  
>          Reviewed by Dimitri Glazkov.
> diff --git a/WebCore/css/CSSStyleSelector.cpp b/WebCore/css/CSSStyleSelector.cpp
> index 1ed4eda..2c529c5 100644
> --- a/WebCore/css/CSSStyleSelector.cpp
> +++ b/WebCore/css/CSSStyleSelector.cpp
> @@ -900,11 +900,11 @@ PseudoState CSSStyleSelector::SelectorChecker::checkPseudoState(Element* element
>      if (iface)
>          return iface->historyContains(QString(reinterpret_cast<QChar*>(url.data()), url.size())) ? PseudoVisited : PseudoLink;
>  
> -    LinkHash hash = visitedLinkHash(url.data(), url.size());
> +    LinkHash hash = visitedLinkHash(document, url.data(), url.size());
>      if (!hash)
>          return PseudoLink;
>  #else
> -    LinkHash hash = visitedLinkHash(m_document->baseURL(), *attr);
> +    LinkHash hash = visitedLinkHash(m_document, *attr);

Don't you mean 

>      if (!hash)
>          return PseudoLink;
>  #endif
> @@ -6106,7 +6106,7 @@ void CSSStyleSelector::SelectorChecker::visitedStateChanged(LinkHash visitedHash
>          return;
>      for (Node* node = m_document; node; node = node->traverseNextNode()) {
>          const AtomicString* attr = linkAttribute(node);
> -        if (attr && visitedLinkHash(m_document->baseURL(), *attr) == visitedHash)
> +        if (attr && visitedLinkHash(m_document, *attr) == visitedHash)
>              node->setNeedsStyleRecalc();
>      }
>  }
> diff --git a/WebCore/dom/Document.cpp b/WebCore/dom/Document.cpp
> index acabb2c..e2afa7d 100644
> --- a/WebCore/dom/Document.cpp
> +++ b/WebCore/dom/Document.cpp
> @@ -1997,6 +1997,11 @@ void Document::setBaseElementURL(const KURL& baseElementURL)
>      updateBaseURL();
>  }
>  
> +String Document::safeHistoryOrigin() const
> +{
> +  return securityOrigin()->toString(); 

If all this does is toString() then you probably shoudln't make your own function unless you have a really good reason.

Also, are you OK with this being "null" in some cases (like embedded sandboxed iframes)?  I doubt it.

> +}
> +  
>  void Document::updateBaseURL()
>  {
>      // DOM 3 Core: When the Document supports the feature "HTML" [DOM Level 2 HTML], the base URI is computed using
> diff --git a/WebCore/dom/Document.h b/WebCore/dom/Document.h
> index e92237a..05bda78 100644
> --- a/WebCore/dom/Document.h
> +++ b/WebCore/dom/Document.h
> @@ -513,6 +513,8 @@ public:
>      // Setting the BaseElementURL will change the baseURL.
>      void setBaseElementURL(const KURL&);
>  
> +    String safeHistoryOrigin() const;
> +  
>      const String& baseTarget() const { return m_baseTarget; }
>      // Setting the BaseElementTarget will change the baseTarget.
>      void setBaseElementTarget(const String& baseTarget) { m_baseTarget = baseTarget; }
> diff --git a/WebCore/page/PageGroup.cpp b/WebCore/page/PageGroup.cpp
> index f6c746d..f42d945 100644
> --- a/WebCore/page/PageGroup.cpp
> +++ b/WebCore/page/PageGroup.cpp
> @@ -151,14 +151,14 @@ void PageGroup::addVisitedLink(const KURL& url)
>      if (!shouldTrackVisitedLinks)
>          return;
>      ASSERT(!url.isEmpty());
> -    addVisitedLink(visitedLinkHash(url.string().characters(), url.string().length()));
> +    addVisitedLink(visitedLinkHash(NULL, url.string().characters(), url.string().length()));

s/NULL/0/

>  }
>  
>  void PageGroup::addVisitedLink(const UChar* characters, size_t length)
>  {
>      if (!shouldTrackVisitedLinks)
>          return;
> -    addVisitedLink(visitedLinkHash(characters, length));
> +    addVisitedLink(visitedLinkHash(NULL, characters, length));

s/NULL/0/

>  }
>  
>  void PageGroup::removeVisitedLinks()
> diff --git a/WebCore/platform/LinkHash.cpp b/WebCore/platform/LinkHash.cpp
> index c399aa2..3ddf071 100644
> --- a/WebCore/platform/LinkHash.cpp
> +++ b/WebCore/platform/LinkHash.cpp
> @@ -147,7 +147,7 @@ static inline bool needsTrailingSlash(const UChar* characters, unsigned length)
>      return pos == length;
>  }
>  
> -LinkHash visitedLinkHash(const UChar* url, unsigned length)
> +LinkHash visitedLinkHash(const Document* document, const UChar* url, unsigned length)
>  {
>    return AlreadyHashed::avoidDeletedValue(StringImpl::computeHash(url, length));
>  }
> @@ -208,14 +208,14 @@ void visitedURL(const KURL& base, const AtomicString& attributeURL, Vector<UChar
>      return;
>  }
>  
> -LinkHash visitedLinkHash(const KURL& base, const AtomicString& attributeURL)
> +LinkHash visitedLinkHash(const Document* document, const KURL& base, const AtomicString& attributeURL)
>  {
>      Vector<UChar, 512> url;
>      visitedURL(base, attributeURL, url);
>      if (url.isEmpty())
>          return 0;
>  
> -    return visitedLinkHash(url.data(), url.size());
> +    return visitedLinkHash(document, url.data(), url.size());
>  }
>  
>  }  // namespace WebCore
> diff --git a/WebCore/platform/LinkHash.h b/WebCore/platform/LinkHash.h
> index 2756654..543353a 100644
> --- a/WebCore/platform/LinkHash.h
> +++ b/WebCore/platform/LinkHash.h
> @@ -30,6 +30,7 @@
>  
>  namespace WebCore {
>  
> +class Document;
>  class AtomicString;
>  class KURL;
>  
> @@ -54,13 +55,13 @@ struct LinkHashHash {
>  };
>  
>  // Returns the has of the string that will be used for visited link coloring.
> -LinkHash visitedLinkHash(const UChar* url, unsigned length);
> +LinkHash visitedLinkHash(const Document* document, const UChar* url, unsigned urlLength);

Do not put a variable name when it's obvious (like document).

>  
>  // Resolves the potentially relative URL "attributeURL" relative to the given
>  // base URL, and returns the hash of the string that will be used for visited
>  // link coloring. It will return the special value of 0 if attributeURL does not
>  // look like a relative URL.
> -LinkHash visitedLinkHash(const KURL& base, const AtomicString& attributeURL);
> +LinkHash visitedLinkHash(const Document* document, const AtomicString& attributeURL);

Ditto.

>  
>  // Resolves the potentially relative URL "attributeURL" relative to the given
>  // base URL, and returns the hash of the string that will be used for visited.
> diff --git a/WebCore/platform/chromium/ChromiumBridge.h b/WebCore/platform/chromium/ChromiumBridge.h
> index e582241..e40992b 100644
> --- a/WebCore/platform/chromium/ChromiumBridge.h
> +++ b/WebCore/platform/chromium/ChromiumBridge.h
> @@ -222,8 +222,9 @@ namespace WebCore {
>          static void traceEventEnd(const char* name, void* id, const char* extra);
>  
>          // Visited links ------------------------------------------------------
> -        static LinkHash visitedLinkHash(const UChar* url, unsigned length);
> -        static LinkHash visitedLinkHash(const KURL& base, const AtomicString& attributeURL);
> +        static LinkHash visitedCanonicalizedLinkHash(const Document* document, const char* canonicalizedURL, unsigned canonicalizedURLLength);
> +        static LinkHash visitedLinkHash(const Document* document, const UChar* url, unsigned urlLength);
> +        static LinkHash visitedLinkHash(const Document* document, const AtomicString& attributeURL);

ditto * 3

>          static bool isLinkVisited(LinkHash);
>  
>          // Widget -------------------------------------------------------------
> diff --git a/WebCore/platform/chromium/LinkHashChromium.cpp b/WebCore/platform/chromium/LinkHashChromium.cpp
> index 9cb93ea..9a001bc 100644
> --- a/WebCore/platform/chromium/LinkHashChromium.cpp
> +++ b/WebCore/platform/chromium/LinkHashChromium.cpp
> @@ -35,14 +35,14 @@
>  
>  namespace WebCore {
>  
> -LinkHash visitedLinkHash(const UChar* url, unsigned length)
> +LinkHash visitedLinkHash(const Document* document, const UChar* url, unsigned length)
>  {
> -    return ChromiumBridge::visitedLinkHash(url, length);
> +    return ChromiumBridge::visitedLinkHash(document, url, length);
>  }
>  
> -LinkHash visitedLinkHash(const KURL& base, const AtomicString& attributeURL)
> +LinkHash visitedLinkHash(const Document* document, const AtomicString& attributeURL)
>  {
> -    return ChromiumBridge::visitedLinkHash(base, attributeURL);
> +    return ChromiumBridge::visitedLinkHash(document, attributeURL);
>  }
>  
>  } // namespace WebCore
> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> index e387f72..a67c6d1 100644
> --- a/WebKit/chromium/ChangeLog
> +++ b/WebKit/chromium/ChangeLog
> @@ -1,3 +1,20 @@
> [phone]  Daniel Clifford  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +		Pass origin information to the hash function used to compute a
> +		the hash for a visited url so that it's possible to implement
> +		a SafeHistory-like policy for only displaying links as visited
> +		if they are embedded in a page with the same-origin.
> +
> +		https://bugs.webkit.org/show_bug.cgi?id=37443
> +
> +        * src/ChromiumBridge.cpp:
> +        (WebCore::ChromiumBridge::visitedCanonicalizedLinkHash):
> +        (WebCore::ChromiumBridge::visitedLinkHash):
> +        * public/WebKitClient.h:
> +        (WebKit::WebKitClient::visitedLinkHash):
> +
>  2010-04-07  Pavel Feldman  <[e-mail]>
>  
>          Reviewed by Yury Semikhatsky.
> diff --git a/WebKit/chromium/public/WebKitClient.h b/WebKit/chromium/public/WebKitClient.h
> index b2aaf2e..1375147 100644
> --- a/WebKit/chromium/public/WebKitClient.h
> +++ b/WebKit/chromium/public/WebKitClient.h
> @@ -123,7 +123,12 @@ public:
>      // link coloring.
>      virtual unsigned long long visitedLinkHash(
>          const char* canonicalURL, size_t length) { return 0; }
> -
> +  
> +    // Returns the hash for the given canonicalized URL for use in visited
> +    // link coloring.
> +    virtual unsigned long long visitedLinkHash(
> +        const char* canonicalURL, size_t canonicalURLLength, const char* origin, size_t originLength) { return visitedLinkHash(canonicalURL, canonicalURLLength); }

Don't wrap.  If anything, put the definition of the function on subsequent lines.

> +  
>      // Returns whether the given link hash is in the user's history.  The
>      // hash must have been generated by calling VisitedLinkHash().
>      virtual bool isLinkVisited(unsigned long long linkHash) { return false; }
> @@ -253,7 +258,6 @@ public:
>      // Disable/Enable sudden termination.
>      virtual void suddenTerminationChanged(bool enabled) { }
>  
> -
>      // System --------------------------------------------------------------
>  
>      // Returns a value such as "en-US".
> diff --git a/WebKit/chromium/src/ChromiumBridge.cpp b/WebKit/chromium/src/ChromiumBridge.cpp
> index cffd166..45bfcc5 100644
> --- a/WebKit/chromium/src/ChromiumBridge.cpp
> +++ b/WebKit/chromium/src/ChromiumBridge.cpp
> @@ -632,24 +632,41 @@ void ChromiumBridge::traceEventEnd(const char* name, void* id, const char* extra
>  
>  // Visited Links --------------------------------------------------------------
>  
> -LinkHash ChromiumBridge::visitedLinkHash(const UChar* url, unsigned length)
> +LinkHash ChromiumBridge::visitedCanonicalizedLinkHash(const Document* document, const char* canonicalizedURL, unsigned canonicalizedURLLength)

Do we REALLY need to pass in char* and unsigned (which should be size_t, right?) params here?  Unless this is the common case and there's no other way, passing in KURLs seems much safer.

> +{
> +    const char* originData = NULL;

= 0;

> +    int originLength = 0;
> +    WebCString str;
> +    if (document != NULL )

if (!document)

What is the purpose of this patch? Is it at all desirable now that bug 24300 has been fixed?

[Daniel Clifford]

Yes, it is desirable to have this extra information. My understanding of dhyatt's patch is that it does effectively prevents a whole class of programatic (JavaScript) attacks, but it isn't bulletproof. As long as links are visibly colored in a way that doesn't strictly honor same origin, then there social engineering attacks that are still possible. phishing.evil.com could create a page with links to the 20 most popular banking sites, where the unvisited color is the page background and the visited color is blue, all of this with some displayed text that is unrelated to banking that seems to be for some other purpose. You only see the link of the URL that is also in your  history, that's the one you click on, at which point evil.com know's which bank you use.

Admittedly, these are attacks are probably less effective to do on a large-scale. However a same-origin policy like SafeHistory still provides extra protection for users who are willing to sacrifice a bit of usability for privacy. The idea would be that it is off by default but can be activated as a preference.

[Daniel Clifford]

Created attachment 53172 [details]
Fix some of the style nits

[Daniel Clifford]

Created attachment 53239 [details]
merge with latest version and incorporate jorlow's feedback

[Daniel Clifford]

(In reply to comment #4)
> (From update of attachment 53163 [details])
> If you want something reviewed, set the r? flag.  If you want it landed if it's
> good, set the commit-queue? flag.
> 
> There are probably better reviewers than me.  Here are some drive by comments,
> though.
> 
> > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> > index 49b8d1f..18a9231 100644
> > --- a/WebCore/ChangeLog
> > +++ b/WebCore/ChangeLog
> > @@ -1,3 +1,32 @@
> > [phone]  Daniel Clifford  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +		Pass origin information to the hash function used to compute a
> 
> Don't use tabs.  Follow the following style:
> 
> """
> subject
> bug url
> 
> longer
> description
> here
> """"

Done

> 
> > +		the hash for a visited url so that it's possible to implement
> > +		a SafeHistory-like policy for only displaying links as visited
> > +		if they are embedded in a page with the same-origin.
> > +
> > +		https://bugs.webkit.org/show_bug.cgi?id=37443
> > +
> > +        * platform/chromium/ChromiumBridge.h:
> > +        * platform/chromium/LinkHashChromium.cpp:
> > +        (WebCore::visitedLinkHash):
> > +        * platform/LinkHash.cpp:
> > +        (WebCore::visitedLinkHash):
> > +        * platform/LinkHash.h:
> > +        * platform/chromium/ChromiumBridge.h:
> > +        * platform/chromium/LinkHashChromium.cpp:
> > +        (WebCore::visitedLinkHash):
> > +        * page/PageGroup.cpp:
> > +        (WebCore::PageGroup::addVisitedLink):
> > +        * dom/Document.cpp:
> > +        (WebCore::Document::safeHistoryOrigin):
> > +        * dom/Document.h:
> > +        * css/CSSStyleSelector.cpp:
> > +        (WebCore::CSSStyleSelector::SelectorChecker::checkPseudoState):
> > +        (WebCore::CSSStyleSelector::SelectorChecker::visitedStateChanged):
> > +
> >  2010-03-30  Dumitru Daniliuc  <[e-mail]>
> >  
> >          Reviewed by Dimitri Glazkov.
> > diff --git a/WebCore/css/CSSStyleSelector.cpp b/WebCore/css/CSSStyleSelector.cpp
> > index 1ed4eda..2c529c5 100644
> > --- a/WebCore/css/CSSStyleSelector.cpp
> > +++ b/WebCore/css/CSSStyleSelector.cpp
> > @@ -900,11 +900,11 @@ PseudoState CSSStyleSelector::SelectorChecker::checkPseudoState(Element* element
> >      if (iface)
> >          return iface->historyContains(QString(reinterpret_cast<QChar*>(url.data()), url.size())) ? PseudoVisited : PseudoLink;
> >  
> > -    LinkHash hash = visitedLinkHash(url.data(), url.size());
> > +    LinkHash hash = visitedLinkHash(document, url.data(), url.size());
> >      if (!hash)
> >          return PseudoLink;
> >  #else
> > -    LinkHash hash = visitedLinkHash(m_document->baseURL(), *attr);
> > +    LinkHash hash = visitedLinkHash(m_document, *attr);
> 
> Don't you mean 

I think you meant that I use m_document for both calls, Done.

> 
> >      if (!hash)
> >          return PseudoLink;
> >  #endif
> > @@ -6106,7 +6106,7 @@ void CSSStyleSelector::SelectorChecker::visitedStateChanged(LinkHash visitedHash
> >          return;
> >      for (Node* node = m_document; node; node = node->traverseNextNode()) {
> >          const AtomicString* attr = linkAttribute(node);
> > -        if (attr && visitedLinkHash(m_document->baseURL(), *attr) == visitedHash)
> > +        if (attr && visitedLinkHash(m_document, *attr) == visitedHash)
> >              node->setNeedsStyleRecalc();
> >      }
> >  }
> > diff --git a/WebCore/dom/Document.cpp b/WebCore/dom/Document.cpp
> > index acabb2c..e2afa7d 100644
> > --- a/WebCore/dom/Document.cpp
> > +++ b/WebCore/dom/Document.cpp
> > @@ -1997,6 +1997,11 @@ void Document::setBaseElementURL(const KURL& baseElementURL)
> >      updateBaseURL();
> >  }
> >  
> > +String Document::safeHistoryOrigin() const
> > +{
> > +  return securityOrigin()->toString(); 
> 
> If all this does is toString() then you probably shoudln't make your own
> function unless you have a really good reason.
> 
> Also, are you OK with this being "null" in some cases (like embedded sandboxed
> iframes)?  I doubt it.

I removed the extra function.

I think "null" is actually ok. the patch simply passes the null origin string through to the link hasher, and the null will ensure the link's hash is unique WRT all other origins. on the link hash builder side, all URLs with null referrers are not added to the safe history hash, so they will never be marked as visited if safe history is active. 

> 
> > +}
> > +  
> >  void Document::updateBaseURL()
> >  {
> >      // DOM 3 Core: When the Document supports the feature "HTML" [DOM Level 2 HTML], the base URI is computed using
> > diff --git a/WebCore/dom/Document.h b/WebCore/dom/Document.h
> > index e92237a..05bda78 100644
> > --- a/WebCore/dom/Document.h
> > +++ b/WebCore/dom/Document.h
> > @@ -513,6 +513,8 @@ public:
> >      // Setting the BaseElementURL will change the baseURL.
> >      void setBaseElementURL(const KURL&);
> >  
> > +    String safeHistoryOrigin() const;
> > +  
> >      const String& baseTarget() const { return m_baseTarget; }
> >      // Setting the BaseElementTarget will change the baseTarget.
> >      void setBaseElementTarget(const String& baseTarget) { m_baseTarget = baseTarget; }
> > diff --git a/WebCore/page/PageGroup.cpp b/WebCore/page/PageGroup.cpp
> > index f6c746d..f42d945 100644
> > --- a/WebCore/page/PageGroup.cpp
> > +++ b/WebCore/page/PageGroup.cpp
> > @@ -151,14 +151,14 @@ void PageGroup::addVisitedLink(const KURL& url)
> >      if (!shouldTrackVisitedLinks)
> >          return;
> >      ASSERT(!url.isEmpty());
> > -    addVisitedLink(visitedLinkHash(url.string().characters(), url.string().length()));
> > +    addVisitedLink(visitedLinkHash(NULL, url.string().characters(), url.string().length()));
> 
> s/NULL/0/
Done.

> 
> >  }
> >  
> >  void PageGroup::addVisitedLink(const UChar* characters, size_t length)
> >  {
> >      if (!shouldTrackVisitedLinks)
> >          return;
> > -    addVisitedLink(visitedLinkHash(characters, length));
> > +    addVisitedLink(visitedLinkHash(NULL, characters, length));
> 
> s/NULL/0/
Done.

> 
> >  }
> >  
> >  void PageGroup::removeVisitedLinks()
> > diff --git a/WebCore/platform/LinkHash.cpp b/WebCore/platform/LinkHash.cpp
> > index c399aa2..3ddf071 100644
> > --- a/WebCore/platform/LinkHash.cpp
> > +++ b/WebCore/platform/LinkHash.cpp
> > @@ -147,7 +147,7 @@ static inline bool needsTrailingSlash(const UChar* characters, unsigned length)
> >      return pos == length;
> >  }
> >  
> > -LinkHash visitedLinkHash(const UChar* url, unsigned length)
> > +LinkHash visitedLinkHash(const Document* document, const UChar* url, unsigned length)
> >  {
> >    return AlreadyHashed::avoidDeletedValue(StringImpl::computeHash(url, length));
> >  }
> > @@ -208,14 +208,14 @@ void visitedURL(const KURL& base, const AtomicString& attributeURL, Vector<UChar
> >      return;
> >  }
> >  
> > -LinkHash visitedLinkHash(const KURL& base, const AtomicString& attributeURL)
> > +LinkHash visitedLinkHash(const Document* document, const KURL& base, const AtomicString& attributeURL)
> >  {
> >      Vector<UChar, 512> url;
> >      visitedURL(base, attributeURL, url);
> >      if (url.isEmpty())
> >          return 0;
> >  
> > -    return visitedLinkHash(url.data(), url.size());
> > +    return visitedLinkHash(document, url.data(), url.size());
> >  }
> >  
> >  }  // namespace WebCore
> > diff --git a/WebCore/platform/LinkHash.h b/WebCore/platform/LinkHash.h
> > index 2756654..543353a 100644
> > --- a/WebCore/platform/LinkHash.h
> > +++ b/WebCore/platform/LinkHash.h
> > @@ -30,6 +30,7 @@
> >  
> >  namespace WebCore {
> >  
> > +class Document;
> >  class AtomicString;
> >  class KURL;
> >  
> > @@ -54,13 +55,13 @@ struct LinkHashHash {
> >  };
> >  
> >  // Returns the has of the string that will be used for visited link coloring.
> > -LinkHash visitedLinkHash(const UChar* url, unsigned length);
> > +LinkHash visitedLinkHash(const Document* document, const UChar* url, unsigned urlLength);
> 
> Do not put a variable name when it's obvious (like document).
Done.

> 
> >  
> >  // Resolves the potentially relative URL "attributeURL" relative to the given
> >  // base URL, and returns the hash of the string that will be used for visited
> >  // link coloring. It will return the special value of 0 if attributeURL does not
> >  // look like a relative URL.
> > -LinkHash visitedLinkHash(const KURL& base, const AtomicString& attributeURL);
> > +LinkHash visitedLinkHash(const Document* document, const AtomicString& attributeURL);
> 
> Ditto.
Done.

> 
> >  
> >  // Resolves the potentially relative URL "attributeURL" relative to the given
> >  // base URL, and returns the hash of the string that will be used for visited.
> > diff --git a/WebCore/platform/chromium/ChromiumBridge.h b/WebCore/platform/chromium/ChromiumBridge.h
> > index e582241..e40992b 100644
> > --- a/WebCore/platform/chromium/ChromiumBridge.h
> > +++ b/WebCore/platform/chromium/ChromiumBridge.h
> > @@ -222,8 +222,9 @@ namespace WebCore {
> >          static void traceEventEnd(const char* name, void* id, const char* extra);
> >  
> >          // Visited links ------------------------------------------------------
> > -        static LinkHash visitedLinkHash(const UChar* url, unsigned length);
> > -        static LinkHash visitedLinkHash(const KURL& base, const AtomicString& attributeURL);
> > +        static LinkHash visitedCanonicalizedLinkHash(const Document* document, const char* canonicalizedURL, unsigned canonicalizedURLLength);
> > +        static LinkHash visitedLinkHash(const Document* document, const UChar* url, unsigned urlLength);
> > +        static LinkHash visitedLinkHash(const Document* document, const AtomicString& attributeURL);
> 
> ditto * 3
Done.

> 
> >          static bool isLinkVisited(LinkHash);
> >  
> >          // Widget -------------------------------------------------------------
> > diff --git a/WebCore/platform/chromium/LinkHashChromium.cpp b/WebCore/platform/chromium/LinkHashChromium.cpp
> > index 9cb93ea..9a001bc 100644
> > --- a/WebCore/platform/chromium/LinkHashChromium.cpp
> > +++ b/WebCore/platform/chromium/LinkHashChromium.cpp
> > @@ -35,14 +35,14 @@
> >  
> >  namespace WebCore {
> >  
> > -LinkHash visitedLinkHash(const UChar* url, unsigned length)
> > +LinkHash visitedLinkHash(const Document* document, const UChar* url, unsigned length)
> >  {
> > -    return ChromiumBridge::visitedLinkHash(url, length);
> > +    return ChromiumBridge::visitedLinkHash(document, url, length);
> >  }
> >  
> > -LinkHash visitedLinkHash(const KURL& base, const AtomicString& attributeURL)
> > +LinkHash visitedLinkHash(const Document* document, const AtomicString& attributeURL)
> >  {
> > -    return ChromiumBridge::visitedLinkHash(base, attributeURL);
> > +    return ChromiumBridge::visitedLinkHash(document, attributeURL);
> >  }
> >  
> >  } // namespace WebCore
> > diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> > index e387f72..a67c6d1 100644
> > --- a/WebKit/chromium/ChangeLog
> > +++ b/WebKit/chromium/ChangeLog
> > @@ -1,3 +1,20 @@
> > [phone]  Daniel Clifford  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +		Pass origin information to the hash function used to compute a
> > +		the hash for a visited url so that it's possible to implement
> > +		a SafeHistory-like policy for only displaying links as visited
> > +		if they are embedded in a page with the same-origin.
> > +
> > +		https://bugs.webkit.org/show_bug.cgi?id=37443
> > +
> > +        * src/ChromiumBridge.cpp:
> > +        (WebCore::ChromiumBridge::visitedCanonicalizedLinkHash):
> > +        (WebCore::ChromiumBridge::visitedLinkHash):
> > +        * public/WebKitClient.h:
> > +        (WebKit::WebKitClient::visitedLinkHash):
> > +
> >  2010-04-07  Pavel Feldman  <[e-mail]>
> >  
> >          Reviewed by Yury Semikhatsky.
> > diff --git a/WebKit/chromium/public/WebKitClient.h b/WebKit/chromium/public/WebKitClient.h
> > index b2aaf2e..1375147 100644
> > --- a/WebKit/chromium/public/WebKitClient.h
> > +++ b/WebKit/chromium/public/WebKitClient.h
> > @@ -123,7 +123,12 @@ public:
> >      // link coloring.
> >      virtual unsigned long long visitedLinkHash(
> >          const char* canonicalURL, size_t length) { return 0; }
> > -
> > +  
> > +    // Returns the hash for the given canonicalized URL for use in visited
> > +    // link coloring.
> > +    virtual unsigned long long visitedLinkHash(
> > +        const char* canonicalURL, size_t canonicalURLLength, const char* origin, size_t originLength) { return visitedLinkHash(canonicalURL, canonicalURLLength); }
> 
> Don't wrap.  If anything, put the definition of the function on subsequent
> lines.
Done.

> 
> > +  
> >      // Returns whether the given link hash is in the user's history.  The
> >      // hash must have been generated by calling VisitedLinkHash().
> >      virtual bool isLinkVisited(unsigned long long linkHash) { return false; }
> > @@ -253,7 +258,6 @@ public:
> >      // Disable/Enable sudden termination.
> >      virtual void suddenTerminationChanged(bool enabled) { }
> >  
> > -
> >      // System --------------------------------------------------------------
> >  
> >      // Returns a value such as "en-US".
> > diff --git a/WebKit/chromium/src/ChromiumBridge.cpp b/WebKit/chromium/src/ChromiumBridge.cpp
> > index cffd166..45bfcc5 100644
> > --- a/WebKit/chromium/src/ChromiumBridge.cpp
> > +++ b/WebKit/chromium/src/ChromiumBridge.cpp
> > @@ -632,24 +632,41 @@ void ChromiumBridge::traceEventEnd(const char* name, void* id, const char* extra
> >  
> >  // Visited Links --------------------------------------------------------------
> >  
> > -LinkHash ChromiumBridge::visitedLinkHash(const UChar* url, unsigned length)
> > +LinkHash ChromiumBridge::visitedCanonicalizedLinkHash(const Document* document, const char* canonicalizedURL, unsigned canonicalizedURLLength)
> 
> Do we REALLY need to pass in char* and unsigned (which should be size_t,
> right?) params here?  Unless this is the common case and there's no other way,
> passing in KURLs seems much safer.
I am following the style of the existing visitedXXX functions, and I don't see an easy way to make these calls KURL based without needlessly complicating the code. There is no KURL available and one would have to be constructed explicitly, only to extract the character buffers again when passing them through to Chromium, which expects buffers. 

> 
> > +{
> > +    const char* originData = NULL;
> 
> = 0;
Done.

> 
> > +    int originLength = 0;
> > +    WebCString str;
> > +    if (document != NULL )
> 
> if (!document)
Done: if (document)

[Daniel Clifford]

Created attachment 53248 [details]
Incorporate more review feedback, fix remaining style issues

[Early Warning System Bot]

Attachment 53248 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1590497

[Daniel Clifford]

Created attachment 53252 [details]
fix qt build

[Early Warning System Bot]

Attachment 53252 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1717042

[Daniel Clifford]

Created attachment 53254 [details]
fix another problem with the qt build

[WebKit Review Bot]

Attachment 53252 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1692174

[Jeremy Orlow]

Just to make it clear, my comments were just drive-by ones....I'm not going to r+ or r- this because it's more of a policy decision of "do we want this" at this point.

[Sam Weinig]

Is this necessary with hyatt's recent visited link work?

[Sam Weinig]

(In reply to comment #17)
> Is this necessary with hyatt's recent visited link work?

Never mind, didn't read the all the comments.  This seems like overkill.

[Darin Fisher (:fishd, Google)]

Instead of passing a Document*, I think it would be better to pass a SecurityOrigin*.  At the WebKit API level, you would then pass a WebSecurityOrigin.

[Adam Barth]

Comment on attachment 53254 [details]
fix another problem with the qt build

Let's stick with dhyatt's approach for now.  If we decide late that it's insufficient, we can revisit this patch.