Bug 25485 - Implement visitedURL to Re-unfork Chromium.
Summary: Implement visitedURL to Re-unfork Chromium.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on: 20952
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-30 11:06 PDT by Dimitri Glazkov (Google)
Modified: 2009-05-01 14:19 PDT (History)
2 users (show)

See Also:


Attachments
Only use visitedURL in QT builds, v1. (3.05 KB, patch)
2009-05-01 09:23 PDT, Dimitri Glazkov (Google)
darin: review-
Details | Formatted Diff | Diff
Only use visitedURL in QT builds, v1. (3.05 KB, patch)
2009-05-01 10:14 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Only use visitedURL in QT builds, v2. (2.29 KB, patch)
2009-05-01 10:16 PDT, Dimitri Glazkov (Google)
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2009-04-30 11:06:17 PDT
Change http://trac.webkit.org/changeset/43052 broke Chromium build -- we need to implement visitedURL in HashLinkChromium.cpp.
Comment 1 Dimitri Glazkov (Google) 2009-04-30 16:02:11 PDT
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;

Comment 2 Dimitri Glazkov (Google) 2009-04-30 16:02:38 PDT
Or something like that :)
Comment 3 Darin Fisher (:fishd, Google) 2009-04-30 22:35:55 PDT
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.
Comment 4 Dimitri Glazkov (Google) 2009-05-01 09:23:47 PDT
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(-)
Comment 5 Dimitri Glazkov (Google) 2009-05-01 09:28:52 PDT
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.
Comment 6 Darin Adler 2009-05-01 09:34:52 PDT
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.
Comment 7 Darin Adler 2009-05-01 09:35:32 PDT
The direction here seems fine. It’s good not to burden the non-Qt platforms with Qt’s requirement here.
Comment 8 Dimitri Glazkov (Google) 2009-05-01 10:14:17 PDT
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(-)
Comment 9 Dimitri Glazkov (Google) 2009-05-01 10:15:38 PDT
Oops, sorry -- hair-trigger-finger :) Correct patch coming up.
Comment 10 Dimitri Glazkov (Google) 2009-05-01 10:16:42 PDT
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(-)
Comment 11 Dimitri Glazkov (Google) 2009-05-01 10:17:48 PDT
Fixed up -- I removed the guards in LinkHash.h and corrected the Qt whoopsie.
Comment 12 Eric Seidel (no email) 2009-05-01 11:57:47 PDT
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.
Comment 13 Dimitri Glazkov (Google) 2009-05-01 14:19:09 PDT
Landed as http://trac.webkit.org/changeset/43120.