[Webkit-unassigned] [Bug 37443] CSSStyleSelector should pass through origin information when determined if link visited
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Apr 13 07:20:13 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=37443
--- Comment #9 from Daniel Clifford <danno at chromium.org> 2010-04-13 07:20:11 PST ---
(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 @@
> > +2010-04-12 Daniel Clifford <danno at google.com>
> > +
> > + 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 <dumi at chromium.org>
> >
> > 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 @@
> > +2010-04-12 Daniel Clifford <danno at google.com>
> > +
> > + 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 <pfeldman at chromium.org>
> >
> > 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)
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list