[webkit-reviews] review denied: [Bug 37443] CSSStyleSelector should pass through origin information when determined if link visited : [Attachment 53163] Pass through origin information about embedding page to be used for SafeHistory-style visited link display
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Apr 12 08:28:52 PDT 2010
Jeremy Orlow <jorlow at chromium.org> has denied review:
Bug 37443: CSSStyleSelector should pass through origin information when
determined if link visited
https://bugs.webkit.org/show_bug.cgi?id=37443
Attachment 53163: Pass through origin information about embedding page to be
used for SafeHistory-style visited link display
https://bugs.webkit.org/attachment.cgi?id=53163&action=review
------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
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
""""
> + 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
> 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 @@
> +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.
> +
> // 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)
More information about the webkit-reviews
mailing list