[webkit-reviews] review requested: [Bug 22652] Allow integration of visited link coloring for Chromium : [Attachment 25742] Patch v1

[bugzilla-daemon at webkit.org]

Brett Wilson (Google) <brettw at chromium.org> has asked  for review:
Bug 22652: Allow integration of visited link coloring for Chromium
https://bugs.webkit.org/show_bug.cgi?id=22652

Attachment 25742: Patch v1
https://bugs.webkit.org/attachment.cgi?id=25742&action=review

------- Additional Comments from Brett Wilson (Google) <brettw at chromium.org>
This is an implementation of visited link hashing that integrates with Chrome's
visited link system.

The reasons for Chromium to provide its own visited link hashing were discussed
extensively on bug 22131, especially in bug 22131 comment 8. This is a
follow-on to my work on that bug.

It also fixes the hashing functions in PageGroup to use the correct
visitedLinkHash function. Currently, some code uses that function and PageGroup
calls the string hash functions directly, so not everybody agrees (the
implementations are the same currently, but probably won't be in the future,
like when bug 22130 is fixed.

I'm happy to discuss other possible implementations. I toyed with having one
big ifdef around all the visited link functions in PageGroup and reimplementing
them separately in a PageGroupChromium.cpp file. This has the benefit of fewer
ifdefs, but has the disadvantage of not making it clear to people reading the
code what is happening aside from a mysterious ifdef, and means I have to
duplicate some logic (basically fork part of the file), and I was trying to
avoid that.

To integrate with our system, it calls a function declared in ChromiumBridge,
which is a file that just declares a bunch of static functions that must be
provided by the embedder at link-time. You can see it here:
http://src.chromium.org/viewvc/chrome/trunk/src/webkit/port/platform/chromium/C
hromiumBridge.h?view=markup

This will move to WebKit's tree along with our port. I have been spending about
2/3 of my time over the past few weeks moving files around and fixing
dependencies so our entire port can be checked into WebKit.org. This will
likely take another month or two to complete (a lot of stuff needs to be moved
from our style and internal dependencies to those of WebCore).

If you're not happy with the ChromiumBridge include here, I can fork the
visited link coloring functions in PageGroup into a separate file in the
ChromiumPort as I discussed above, though I think this is a messier solution.

I'm also open to other suggestions of how to bridge out to Chromium code. You
can see the Chromium side of this patch here:
http://codereview.chromium.org/12928/show
The changes in renderer_glue are the eventual implementations of the functions,
which call into the visited link component.