Bug 85412

Summary:

[Chromium] Call isLinkVisited directly

Product:

WebKit

Reporter:

Mark Pilgrim (Google) <pilgrim>

Component:

WebKit Misc.

Assignee:

Mark Pilgrim (Google) <pilgrim>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, fishd, gustavo, haraken, philn, rakuco, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

82948

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

Mark Pilgrim (Google)

Reported 2012-05-02 13:16:38 PDT

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.

Attachments
Patch (19.69 KB, patch) 2012-05-09 08:13 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
Patch (19.46 KB, patch) 2012-05-10 13:31 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
Patch (8.11 KB, patch) 2012-05-11 19:00 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
Patch (14.57 KB, patch) 2012-05-11 19:26 PDT, Mark Pilgrim (Google)	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Darin Fisher (:fishd, Google)

Comment 1 2012-05-02 16:17:39 PDT

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)

Comment 2 2012-05-09 08:13:33 PDT

Created attachment 140949 [details] Patch

Mark Pilgrim (Google)

Comment 3 2012-05-09 08:15:47 PDT

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

Build Bot

Comment 4 2012-05-09 08:22:58 PDT

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 5 2012-05-09 08:24:43 PDT

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 6 2012-05-09 08:31:12 PDT

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 7 2012-05-09 08:54:54 PDT

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 8 2012-05-09 09:17:58 PDT

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)

Comment 9 2012-05-10 13:31:58 PDT

Created attachment 141238 [details] Patch

Mark Pilgrim (Google)

Comment 10 2012-05-10 13:33:07 PDT

Comment on attachment 141238 [details] Patch Fixed missing include, should fix compile problems. Also renamed VisitedLinkSupport to VisitedLinks.

Adam Barth

Comment 11 2012-05-10 13:35:38 PDT

Comment on attachment 141238 [details] Patch Did the entry in the header already move?

Mark Pilgrim (Google)

Comment 12 2012-05-11 19:00:09 PDT

Created attachment 141541 [details] Patch