Bug 22652 - Allow integration of visited link coloring for Chromium
Summary: Allow integration of visited link coloring for Chromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brett Wilson (Google)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-12-04 10:40 PST by Brett Wilson (Google)
Modified: 2008-12-05 18:27 PST (History)
0 users

See Also:


Attachments
Patch v1 (5.25 KB, patch)
2008-12-04 10:41 PST, Brett Wilson (Google)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brett Wilson (Google) 2008-12-04 10:40:17 PST
Patch with detailed explanation coming shortly...
Comment 1 Brett Wilson (Google) 2008-12-04 10:41:10 PST
Created attachment 25742 [details]
Patch v1

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/ChromiumBridge.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.
Comment 2 Darin Adler 2008-12-05 09:32:08 PST
Comment on attachment 25742 [details]
Patch v1

Looks fine. We might later find an even cleaner way to do this, but it's suitable to land as-is.
Comment 3 Brett Wilson (Google) 2008-12-05 18:27:53 PST
Fixed in r39060