Bug 25485 – Implement visitedURL to Re-unfork Chromium.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Dimitri Glazkov (Google)]

Change http://trac.webkit.org/changeset/43052 broke Chromium build -- we need to implement visitedURL in HashLinkChromium.cpp.

[Dimitri Glazkov (Google)]

Gar. This looks messy. I wonder if it'll be okay if I just do:

#if PLATFORM(CHROMIUM)
    LinkHash hash = visitedLinkHash(m_document->baseURL(), *attr); 
    if (!hash)
        return PseudoLink;
#endif

Vector<UChar, 512> url;

[Dimitri Glazkov (Google)]

Or something like that :)

[Darin Fisher (:fishd, Google)]

It seems like _only_ the QT port requires the visitedURL call.  Surely that is not a free call on other platforms, so maybe only the QT port should make that call.

[Dimitri Glazkov (Google)]

Created attachment 29940 [details]
Only use visitedURL in QT builds, v1.

 WebCore/ChangeLog                |   13 +++++++++++++
 WebCore/css/CSSStyleSelector.cpp |   25 +++++++++++++++----------
 WebCore/platform/LinkHash.h      |    3 ++-
 3 files changed, 30 insertions(+), 11 deletions(-)

[Dimitri Glazkov (Google)]

Ok, here's an ugly fix -- create separate code paths for QT and non-QT builds in checkPseudoState. The good news is that the separation is only on the high level, because visitedLinkHash still uses visitedURL.

[Darin Adler]

Comment on attachment 29940 [details]
Only use visitedURL in QT builds, v1.

> +        (WebCore::CSSStyleSelector::SelectorChecker::checkPseudoState): Moved guards around to
> +            provide separate code paths for QT and non-QT ports.

It’s Qt, not QT. This mistake makes this especially confusing at Apple, because lots of people at Apple use QT to mean QuickTime.

> +#if PLATFORM(QT)
>  // 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.
>  // It will return an empty Vector in case of errors.
>  void visitedURL(const KURL& base, const AtomicString& attributeURL, Vector<UChar, 512>&);
> -
> +#endif

My general thought here is:

    1) It doesn’t do harm to declare a function we plan to never define, so sometimes we just leave out the #if statements in headers for cases like this.

    2) I believe adding this #if to the header but not the LinkHash.cpp file will break the Mac build, because we compile with a warning for functions that are defined with external linkage and not previously declared. So you need to add this #if to LinkHash.cpp, not just LinkHash.h.

review- because of breaking the Mac build.

[Darin Adler]

The direction here seems fine. It’s good not to burden the non-Qt platforms with Qt’s requirement here.

[Dimitri Glazkov (Google)]

Created attachment 29941 [details]
Only use visitedURL in QT builds, v1.

 WebCore/ChangeLog                |   13 +++++++++++++
 WebCore/css/CSSStyleSelector.cpp |   25 +++++++++++++++----------
 WebCore/platform/LinkHash.h      |    3 ++-
 3 files changed, 30 insertions(+), 11 deletions(-)

[Dimitri Glazkov (Google)]

Oops, sorry -- hair-trigger-finger :) Correct patch coming up.

[Dimitri Glazkov (Google)]

Created attachment 29942 [details]
Only use visitedURL in QT builds, v2.

 WebCore/ChangeLog                |   12 ++++++++++++
 WebCore/css/CSSStyleSelector.cpp |   25 +++++++++++++++----------
 2 files changed, 27 insertions(+), 10 deletions(-)

[Dimitri Glazkov (Google)]

Fixed up -- I removed the guards in LinkHash.h and corrected the Qt whoopsie.

[Eric Seidel (no email)]

Comment on attachment 29942 [details]
Only use visitedURL in QT builds, v2.

You should mention restoring the order in your ChangeLog.  This looks sane, so long as you know that no other build besides Qt needs the visitedURL call.

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/43120.