Bug 85412 – [Chromium] Call isLinkVisited directly

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

[Mark Pilgrim (Google)]

As part of the PlatformSupport refactoring (see bug 82948), I need to migrate isLinkVisited out of PlatformSupport.cpp/h and into... somewhere. Or do I? Example usage is here: http://trac.webkit.org/browser/trunk/Source/WebCore/page/PageGroup.cpp#L175 As you can see, Chromium is a special case. Every other port uses m_visitedLinkHashes, but Chromium routes through PlatformSupport. Now, if we want to continue special-casing Chromium, I could create a class in platform/VisitedLinksSupport.cpp/h and another in platform/chromium/VisitedLinksSupportChromium.cpp that contained a static isLinkVisited function that routes to WebKit::Platform::current()->isLinkVisited(...).  I've followed that pattern numerous times (StatsCounter, MemoryUsageSupport, TraceEventSupport). Or should we discuss removing the special case for Chromium and handling visited links the same way as other ports? (I don't know if this is possible or desirable; I'm just looking for guidance on how to proceed.)

PlatformSupport::visitedLinkHash() is probably also affected by this decision.

[Darin Fisher (:fishd, Google)]

I'm pretty sure that we need to maintain the Chromium specific code paths.  The reason for this is that we use a shared memory buffer on the Chromium-side to implement these functions.

[Mark Pilgrim (Google)]

Created attachment 140949 [details]
Patch

[Mark Pilgrim (Google)]

OK, new VisitedLinkSupport.cpp/h and VisitedLinkSupportChromium.cpp to house isLinkVisited function inside WebCore. Same pattern as, for example, bug 84376.

[Build Bot]

Comment on attachment 140949 [details]
Patch

Attachment 140949 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12649712

[Early Warning System Bot]

Comment on attachment 140949 [details]
Patch

Attachment 140949 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12654624

[Early Warning System Bot]

Comment on attachment 140949 [details]
Patch

Attachment 140949 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12651652

[Gyuyoung Kim]

Comment on attachment 140949 [details]
Patch

Attachment 140949 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12664026

[Adam Barth]

Comment on attachment 140949 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140949&action=review

Looks like you've got some compile troubles.

> Source/WebCore/page/PageGroup.cpp:179
> -    return PlatformSupport::isLinkVisited(visitedLinkHash);
> +    return VisitedLinkSupport::isLinkVisited(visitedLinkHash);

I wonder if we should move this into the client.  This seems more like something the ChromeClient should worry about...

If we are going to keep this as a static, we should use a better name than VisitedLinkSupport though.  Perhaps VistedLinks?

[Mark Pilgrim (Google)]

Created attachment 141238 [details]
Patch

[Mark Pilgrim (Google)]

Comment on attachment 141238 [details]
Patch

Fixed missing include, should fix compile problems. Also renamed VisitedLinkSupport to VisitedLinks.

[Adam Barth]

Comment on attachment 141238 [details]
Patch

Did the entry in the header already move?

[Mark Pilgrim (Google)]

Created attachment 141541 [details]
Patch

[Mark Pilgrim (Google)]

Comment on attachment 141541 [details]
Patch

Repatch for ToT.

[Early Warning System Bot]

Comment on attachment 141541 [details]
Patch

Attachment 141541 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12668610

[Gyuyoung Kim]

Comment on attachment 141541 [details]
Patch

Attachment 141541 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12677371

[Early Warning System Bot]

Comment on attachment 141541 [details]
Patch

Attachment 141541 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12671559

[Build Bot]

Comment on attachment 141541 [details]
Patch

Attachment 141541 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12666693

[Mark Pilgrim (Google)]

Created attachment 141545 [details]
Patch

[Mark Pilgrim (Google)]

Comment on attachment 141545 [details]
Patch

Well that went badly. Let's try again.

[Adam Barth]

Comment on attachment 141545 [details]
Patch

That looks a lot greener.  :)

[WebKit Review Bot]

Comment on attachment 141545 [details]
Patch

Clearing flags on attachment: 141545

Committed r116840: <http://trac.webkit.org/changeset/116840>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.