99483 2012-10-16 11:09:02 -0700 Evaluate whether Chromium can use PlatformStrategies like all the other ports 2012-11-21 17:07:40 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 webkit.review.bot abarth abarth ap beidson eric fishd mjs sam oldest_to_newest 743321 0 webkit.review.bot 2012-10-16 11:09:02 -0700 Evaluate whether Chromium can use PlatformStrategies like all the other ports Requested by abarth on #webkit. 748046 1 abarth 2012-10-22 16:23:52 -0700 I forget who asked me this question, but I think it was beidson. The short answer is "yes", but PlatformStrategies seem to contain layering violations currently. For example, PlatformStrategies.h is in WebCore/platform, but it mentions PluginStrategy, which is in WebCore/plugins. The long answer is that PlatformStrategies would introduce an extra layer of indirection that isn't necessary for Chromium. PlatformStrategies is playing a similar role to http://trac.webkit.org/browser/trunk/Source/Platform/chromium/public/Platform.h The main difference is that we jump directly to the embedder rather than thunking though the WebKit/WebKit2 layer. 748059 2 beidson 2012-10-22 16:34:59 -0700 (In reply to comment #1) > I forget who asked me this question, but I think it was beidson. > > The short answer is "yes", but PlatformStrategies seem to contain layering violations currently. For example, PlatformStrategies.h is in WebCore/platform, but it mentions PluginStrategy, which is in WebCore/plugins. This "layering" violation seems tenuous, as all of WebCore is generally considered to be the same layer. Are you suggesting that anywhere else we "cross directories" within WebCore we're committing a layering violation? > > The long answer is that PlatformStrategies would introduce an extra layer of indirection that isn't necessary for Chromium. PlatformStrategies is playing a similar role to > > http://trac.webkit.org/browser/trunk/Source/Platform/chromium/public/Platform.h How exactly does the chromium "Platform" class get instantiated and plugged in to WebCore? > The main difference is that we jump directly to the embedder rather than thunking though the WebKit/WebKit2 layer. On the surface it seems to me like "class Platform" from the above header is a pseudo-WebKit port in and of itself. Do you have any alternate suggestions for how "All of the WebKit ports besides Chromium" and "Chromium" might adopt the same mechanism for what PlatformStrategies currently accomplishes? As it is there are a few scattered "#if USE(PLATFORM_STRATEGIES)" blocks that basically exist to support Chromium, but Alexey and I are each working on two separate projects that threaten to add more. 748077 3 abarth 2012-10-22 16:45:30 -0700 > Are you suggesting that anywhere else we "cross directories" within WebCore we're committing a layering violation? Nope. WebCore/platform is special in that sense. Both ap and sam have r-'ed patches of mine for such layering violations. > How exactly does the chromium "Platform" class get instantiated and plugged in to WebCore? The embedder supplies an implementation of Platform when initializing WebKit: http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/public/WebKit.h#L45 (Minor detail: Actually the embedder supplies an implementation of WebKitPlatformSupport, which is a subclass of Platform. We're in the process of deleting WebKitPlatformSupport, and once we're done the embedder will supply a WebKit::Platform object directly.) The implementation gets stored in a static, similar to teh s_platformStrategies in http://trac.webkit.org/browser/trunk/Source/WebCore/platform/PlatformStrategies.cpp#L34 > > The main difference is that we jump directly to the embedder rather than thunking though the WebKit/WebKit2 layer. > > On the surface it seems to me like "class Platform" from the above header is a pseudo-WebKit port in and of itself. That is not the case. > Do you have any alternate suggestions for how "All of the WebKit ports besides Chromium" and "Chromium" might adopt the same mechanism for what PlatformStrategies currently accomplishes? Nope. The requirements are different because WebKit2 needs to do extra work that would just be thunks to the Platform API in Chromium. > As it is there are a few scattered "#if USE(PLATFORM_STRATEGIES)" blocks that basically exist to support Chromium, but Alexey and I are each working on two separate projects that threaten to add more. It would be nice to remove those ifdefs, but I don't see a way to do it without introducing an extra layer of indirection for Chromium. I'd have to look at exactly what you're doing, but I suspect you're introducing an extra layer of indirection for WebKit1 as well. Perhaps that's less important to you? The root issue is that Chromium does not wish to adopt WebKit2 at this time. 748107 4 beidson 2012-10-22 17:06:50 -0700 (In reply to comment #3) > > Are you suggesting that anywhere else we "cross directories" within WebCore we're committing a layering violation? > > Nope. WebCore/platform is special in that sense. Both ap and sam have r-'ed patches of mine for such layering violations. Discussed this with Alexey on IRC. It's - amazingly - the first I've heard of this rule. I wonder if it's documented anywhere. Despite the countless violations of it that are in WebCore/platform, it does make sense. > > How exactly does the chromium "Platform" class get instantiated and plugged in to WebCore? > > The embedder supplies an implementation of Platform when initializing WebKit: > > http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/public/WebKit.h#L45 > > (Minor detail: Actually the embedder supplies an implementation of WebKitPlatformSupport, which is a subclass of Platform. We're in the process of deleting WebKitPlatformSupport, and once we're done the embedder will supply a WebKit::Platform object directly.) > > The implementation gets stored in a static, similar to teh s_platformStrategies in http://trac.webkit.org/browser/trunk/Source/WebCore/platform/PlatformStrategies.cpp#L34 So it sounds like the embedder provides a class that implements these necessary functions for Chromium WebCore's sake... > > Do you have any alternate suggestions for how "All of the WebKit ports besides Chromium" and "Chromium" might adopt the same mechanism for what PlatformStrategies currently accomplishes? > > Nope. The requirements are different because WebKit2 needs to do extra work that would just be thunks to the Platform API in Chromium. ...but I must be confused, because it sounds like what the other ports do with it could easily be precisely what WebKitPlatformSupport does...? Also, I think you mischaracterize this as being about WebKit2. It's about the WebCore porting layer that makes the WebKit project possible. It is used in at least 5 WebKit1 ports, including all of the top-tier ports that have build bots. Except Chromium. > > As it is there are a few scattered "#if USE(PLATFORM_STRATEGIES)" blocks that basically exist to support Chromium, but Alexey and I are each working on two separate projects that threaten to add more. > > It would be nice to remove those ifdefs, but I don't see a way to do it without introducing an extra layer of indirection for Chromium. What is the concern about this layer of indirection? The only one I can think of is a speed cost which seems like it could be negated by implementing Chromium's strategies in 100% inlined headers. Am I missing something else obvious? > I'd have to look at exactly what you're doing, but I suspect you're introducing an extra layer of indirection for WebKit1 as well. Perhaps that's less important to you? The PlatformStrategies layer of customization already exists for EVERY WebKit port inside Source/WebKit that maintains a build bot... exception Chromium. It also exists for Source/WebKit2, but for the sake of this discussion WebKit2 is "just another WebKit port" > The root issue is that Chromium does not wish to adopt WebKit2 at this time. As previously stated, this is not specifically about WebKit2. 748126 5 abarth 2012-10-22 17:42:00 -0700 > So it sounds like the embedder provides a class that implements these necessary functions for Chromium WebCore's sake... Yes. > > > Do you have any alternate suggestions for how "All of the WebKit ports besides Chromium" and "Chromium" might adopt the same mechanism for what PlatformStrategies currently accomplishes? > > > > Nope. The requirements are different because WebKit2 needs to do extra work that would just be thunks to the Platform API in Chromium. > > ...but I must be confused, because it sounds like what the other ports do with it could easily be precisely what WebKitPlatformSupport does...? The difference is that WebKit::Platform is a public API for Chromium and PlatformStrategies is not. In Chromium, WebCore talks directly to the public API rather than indirecting through the WebKit layer. The WebKit layer doesn't add any value for Chromium, unlike in WebKit2 where WebKit2 does the process-hopping work. > Also, I think you mischaracterize this as being about WebKit2. It's about the WebCore porting layer that makes the WebKit project possible. It is used in at least 5 WebKit1 ports, including all of the top-tier ports that have build bots. Except Chromium. Let's take VisitedLinkStrategy::isLinkVisited as an example. Here's the WebKit1 implementation of VisitedLinkStrategy:isLinkVisited on apple-mac: bool WebPlatformStrategies::isLinkVisited(Page* page, LinkHash hash, const KURL&, const AtomicString&) { return page->group().isLinkVisited(hash); } Notice that from WebKit1's perspective VisitedLinkStrategy::isLinkVisited is just unnecessary indirection. The callsite for this function is in SelectionChecker.cpp: #if USE(PLATFORM_STRATEGIES) return platformStrategies()->visitedLinkStrategy()->isLinkVisited(page, hash, m_document->baseURL(), *attribute) ? InsideVisitedLink : InsideUnvisitedLink; #else return page->group().isLinkVisited(hash) ? InsideVisitedLink : InsideUnvisitedLink; #endif Without USE(PLATFORM_STRATEGIES) defined, we just skip the extra indirection through the WebKit layer. > > > As it is there are a few scattered "#if USE(PLATFORM_STRATEGIES)" blocks that basically exist to support Chromium, but Alexey and I are each working on two separate projects that threaten to add more. > > > > It would be nice to remove those ifdefs, but I don't see a way to do it without introducing an extra layer of indirection for Chromium. > > What is the concern about this layer of indirection? The only one I can think of is a speed cost which seems like it could be negated by implementing Chromium's strategies in 100% inlined headers. That's not possible given that these are virtual functions. > Am I missing something else obvious? Consider how Chromium implements isLinkVisited. There's a port-independent header file called VisitedLinks.h. VisitedLinksChromium.cpp implements that header and calls the API directly: bool VisitedLinks::isLinkVisited(LinkHash visitedLinkHash) { return WebKit::Platform::current()->isLinkVisited(visitedLinkHash); } That's one virtual function call (to reach the embedder) rather than two virtual function calls (one to reach the WebKit-layer and then another to reach the embedder). > > I'd have to look at exactly what you're doing, but I suspect you're introducing an extra layer of indirection for WebKit1 as well. Perhaps that's less important to you? > > The PlatformStrategies layer of customization already exists for EVERY WebKit port inside Source/WebKit that maintains a build bot... exception Chromium. As far as I can tell, PlatformStrategies exists for the benefit of WebKit2 at the expense of WebKit1. That makes sense for a port like apple-mac that is focused on WebKit2, but Chromium does not wish to adopt WebKit2 at this time and wishes to continue using WebKit1. I'm certainly willing to believe that isLinkVisited is a bad example. Is there a better example I should look at instead? > > The root issue is that Chromium does not wish to adopt WebKit2 at this time. > > As previously stated, this is not specifically about WebKit2. That doesn't match my reading of the code. What value does VisitedLinkStrategy::isLinkVisited provide to WebKit1 on apple-mac that doesn't involve some reference to WebKit2? As far as I can tell, it just introduces an extra level of indirection. 748240 6 sam 2012-10-22 21:09:46 -0700 (In reply to comment #5) > > So it sounds like the embedder provides a class that implements these necessary functions for Chromium WebCore's sake... > > Yes. > > > > > Do you have any alternate suggestions for how "All of the WebKit ports besides Chromium" and "Chromium" might adopt the same mechanism for what PlatformStrategies currently accomplishes? > > > > > > Nope. The requirements are different because WebKit2 needs to do extra work that would just be thunks to the Platform API in Chromium. > > > > ...but I must be confused, because it sounds like what the other ports do with it could easily be precisely what WebKitPlatformSupport does...? > > The difference is that WebKit::Platform is a public API for Chromium and PlatformStrategies is not. In Chromium, WebCore talks directly to the public API rather than indirecting through the WebKit layer. The WebKit layer doesn't add any value for Chromium, unlike in WebKit2 where WebKit2 does the process-hopping work. This is unfortunate, and I hope you reconsider this for the sake of the other ports and being WebKit citizen. > > > Also, I think you mischaracterize this as being about WebKit2. It's about the WebCore porting layer that makes the WebKit project possible. It is used in at least 5 WebKit1 ports, including all of the top-tier ports that have build bots. Except Chromium. > > Let's take VisitedLinkStrategy::isLinkVisited as an example. Here's the WebKit1 implementation of VisitedLinkStrategy:isLinkVisited on apple-mac: > > bool WebPlatformStrategies::isLinkVisited(Page* page, LinkHash hash, const KURL&, const AtomicString&) > { > return page->group().isLinkVisited(hash); > } > > Notice that from WebKit1's perspective VisitedLinkStrategy::isLinkVisited is just unnecessary indirection. The callsite for this function is in SelectionChecker.cpp: > > #if USE(PLATFORM_STRATEGIES) > return platformStrategies()->visitedLinkStrategy()->isLinkVisited(page, hash, m_document->baseURL(), *attribute) ? InsideVisitedLink : InsideUnvisitedLink; > #else > return page->group().isLinkVisited(hash) ? InsideVisitedLink : InsideUnvisitedLink; > #endif > > Without USE(PLATFORM_STRATEGIES) defined, we just skip the extra indirection through the WebKit layer. You are right, this abstraction exists to accommodate WebCore being able to exist in a world with more than one WebKit layer (WebKit1 and WebKit2 in this case). That said, I have tried hard to only use this where I did not believe it would have significant performance impact. If the performance is critical there, we should probably fix it for all ports, not just for yours. > > > > > As it is there are a few scattered "#if USE(PLATFORM_STRATEGIES)" blocks that basically exist to support Chromium, but Alexey and I are each working on two separate projects that threaten to add more. > > > > > > It would be nice to remove those ifdefs, but I don't see a way to do it without introducing an extra layer of indirection for Chromium. > > > > What is the concern about this layer of indirection? The only one I can think of is a speed cost which seems like it could be negated by implementing Chromium's strategies in 100% inlined headers. > > That's not possible given that these are virtual functions. > > > Am I missing something else obvious? > > Consider how Chromium implements isLinkVisited. There's a port-independent header file called VisitedLinks.h. VisitedLinksChromium.cpp implements that header and calls the API directly: > > bool VisitedLinks::isLinkVisited(LinkHash visitedLinkHash) > { > return WebKit::Platform::current()->isLinkVisited(visitedLinkHash); > } > > That's one virtual function call (to reach the embedder) rather than two virtual function calls (one to reach the WebKit-layer and then another to reach the embedder). > > > > I'd have to look at exactly what you're doing, but I suspect you're introducing an extra layer of indirection for WebKit1 as well. Perhaps that's less important to you? > > > > The PlatformStrategies layer of customization already exists for EVERY WebKit port inside Source/WebKit that maintains a build bot... exception Chromium. > > As far as I can tell, PlatformStrategies exists for the benefit of WebKit2 at the expense of WebKit1. That makes sense for a port like apple-mac that is focused on WebKit2, but Chromium does not wish to adopt WebKit2 at this time and wishes to continue using WebKit1. > > I'm certainly willing to believe that isLinkVisited is a bad example. Is there a better example I should look at instead? > > > > The root issue is that Chromium does not wish to adopt WebKit2 at this time. > > > > As previously stated, this is not specifically about WebKit2. > > That doesn't match my reading of the code. What value does VisitedLinkStrategy::isLinkVisited provide to WebKit1 on apple-mac that doesn't involve some reference to WebKit2? As far as I can tell, it just introduces an extra level of indirection. Indeed, there a many places where we abstract to allow for multiple ports but keep a clean WebCore (all of the clients, ChromeClient, FrameLoaderClient for example). The intent of PlatformStrategies is to extend that model to places where there is no Page to hang the clients off of. By not adopting it, you are making the code a more #ifdeffy place, which is a bit sad. 748254 7 170058 abarth 2012-10-22 21:33:36 -0700 Created attachment 170058 An approach without extra virtual functin calls or ifdefs 748260 8 abarth 2012-10-22 21:46:06 -0700 > This is unfortunate, and I hope you reconsider this for the sake of the other ports and being WebKit citizen. I'd rather find a solution that doesn't involving adding extra virtual function calls to the Chromium port. Attachment 170058 gives an example of how we might achieve this, at least for this particular case. In that patch, Chromium is still works differently from the WebKit2 ports, but we're not going to be able to change that without either introducing extra indirection or having Chromium adopt WebKit2. 748746 9 sam 2012-10-23 09:51:19 -0700 (In reply to comment #8) > > This is unfortunate, and I hope you reconsider this for the sake of the other ports and being WebKit citizen. > > I'd rather find a solution that doesn't involving adding extra virtual function calls to the Chromium port. Attachment 170058 [details] gives an example of how we might achieve this, at least for this particular case. In that patch, Chromium is still works differently from the WebKit2 ports, but we're not going to be able to change that without either introducing extra indirection or having Chromium adopt WebKit2. I am ok with an approach that doesn't require a virtual function for ports that don't need to support two WebKits, but I still don't understand why you feel this is so performance critical. 748780 10 abarth 2012-10-23 10:35:24 -0700 > I am ok with an approach that doesn't require a virtual function for ports that don't need to support two WebKits, but I still don't understand why you feel this is so performance critical. As Maciej wrote yesterday: [[ Just as for behavior-affecting changes it is the patch submitter's job to provide evidence that the patch is correct, with performance-affecting patches it is the patch submitter's job to provide evidence that the patch improves performance as intended and does not regress things that it might be feared to regress. ]] https://bugs.webkit.org/show_bug.cgi?id=100057#c6 In a broader context, this topic was already discussed in detail in June 2010 in this thread on webkit-dev: http://lists.webkit.org/pipermail/webkit-dev/2010-June/013270.html In that thread, Darin Fisher wrote: [[ It's not so much about performance. There's a cognitive cost to having so many layers. But, given (c) above, I understand that you really don't have a choice. In contrast, the Chromium project doesn't build WebCore as a standalone DLL, so we don't have the same constraint. I don't have a problem replacing ChromiumBridge / PlatformBridge with a virtual table (or a set of virtual tables) if that means that we can share more code with other ports. ]] What's changed since then is that we've deleted PlatformBridge from the Chromium port and removed this cognitive cost. That was a fair amount of work because we needed to factor the Chromium WebKit API into two pieces, the Client API and the Platform API, as discussed on webkit-dev: http://lists.webkit.org/pipermail/webkit-dev/2012-March/020166.html Re-introducing PlatformBridge into the Chromium port renamed to PlatformStrategies and with a slower implementation is a net loss. 748785 11 mjs 2012-10-23 10:43:24 -0700 (In reply to comment #2) > (In reply to comment #1) > > I forget who asked me this question, but I think it was beidson. > > > > The short answer is "yes", but PlatformStrategies seem to contain layering violations currently. For example, PlatformStrategies.h is in WebCore/platform, but it mentions PluginStrategy, which is in WebCore/plugins. > > This "layering" violation seems tenuous, as all of WebCore is generally considered to be the same layer. Are you suggesting that anywhere else we "cross directories" within WebCore we're committing a layering violation? WebCore/platform is usually considered a layer below all of the rest of WebCore, so it shouldn't depend on other subdirectories of WebCore. We should fix this. (Probably worth a separate bug). 748803 12 mjs 2012-10-23 11:04:41 -0700 (In reply to comment #10) > > I am ok with an approach that doesn't require a virtual function for ports that don't need to support two WebKits, but I still don't understand why you feel this is so performance critical. > > As Maciej wrote yesterday: > > [[ > Just as for behavior-affecting changes it is the patch submitter's job to provide evidence that the patch is correct, with performance-affecting patches it is the patch submitter's job to provide evidence that the patch improves performance as intended and does not regress things that it might be feared to regress. > ]] FWIW when we first adopted the PlatformStrategies approach, we confirmed that it did not measurably regress our core benchmarks, even using the classic Mac WebKit API where all it does is add a level of indirection through a virtual function call. The benchmarks we tested specifically included page load speed benchmarks (PLT, iBench HTML). If you are worried that the virtual call overhead being worse in the Chromium context there is probably some test that can be done to find out. Of course, it is ultimately up to Chromium folks you whether you adopt the PlatformStrategies mechanism. I have no personal interest in being pushy about it. I just hope that can be an informed choice. > What's changed since then is that we've deleted PlatformBridge from the Chromium port and removed this cognitive cost. That was a fair amount of work because we needed to factor the Chromium WebKit API into two pieces, the Client API and the Platform API, as discussed on webkit-dev: There's also a cognitive cost to Chromium being different from other ports in unnecessary ways, for everyone working on the project. I think that is the motive behind the suggestion in this bug. I agree that you shouldn't have to incur a measurable perf hit to avoid this cognitive cost, though, if indeed there is one. > > http://lists.webkit.org/pipermail/webkit-dev/2012-March/020166.html > > Re-introducing PlatformBridge into the Chromium port renamed to PlatformStrategies and with a slower implementation is a net loss. Another thing I said yesterday was that optimization without measuring was a well-known bad engineering practice, and I think that applies to assuming virtual calls will be slower without testing. If anyone does try any kind of A/B test, I'd be interested in seeing the results. We can also try digging up old versions of Safari/WebKit to retest the pert impact of the PlatformStrategies change at the time it was added. 748884 13 abarth 2012-10-23 13:09:07 -0700 I put the (hopefully!) non-controversial part of this change in bug 100156. 772690 14 abarth 2012-11-21 02:32:28 -0800 This discussion appears to have run its course. The summary from my perspective is that PlatformStrategies has been designed with only the needs of WebKit2 in mind. The main thing that has changed since we last discussed this topic in 2010 is that we've put some amount of effort into the Chromium port to remove precisely the extra thunks that adopting PlatformStrategies would introduce. It might be appropriate to revisit this topic in the future if (1) you all are interested in redesigning PlatformStrategies to provide value to non-WebKit2 ports or (2) Chromium decides to adopt WebKit2. 773406 15 mjs 2012-11-21 16:36:01 -0800 (In reply to comment #14) > This discussion appears to have run its course. > > The summary from my perspective is that PlatformStrategies has been designed with only the needs of WebKit2 in mind. The main thing that has changed since we last discussed this topic in 2010 is that we've put some amount of effort into the Chromium port to remove precisely the extra thunks that adopting PlatformStrategies would introduce. > > It might be appropriate to revisit this topic in the future if (1) you all are interested in redesigning PlatformStrategies to provide value to non-WebKit2 ports or (2) Chromium decides to adopt WebKit2. We'd definitely change PlatformStrategies if we could make it usable for Chromium while still serving its current purpose. Removing unnecessary architectural divergences is an important goal for us. Do you have any specific suggestions for changes that would help? Is there any obstacle besides the extra level of virtual call? Would it be productive to discuss in person how we might make a mechanism that meets all our needs? If virtual call is the only issue, then it seems like it may be possible to make PlatformStrategies only conditionally virtual, so that indirection via virtual method calls is introduced only for ports that actually need runtime switchable strategies. I am skeptical that the virtual calls in this case actually have a measurable runtime cost, but there's no reason to impose them in cases where they have no value. If you'd still be concerned about an approach with no extra virtual calls for Chromium solely because of an extra layer of indirection, then I guess there is not much we can do, but I hope would consider the tradeoff of the cognitive cost of unnecessary Chromium-specific differences vs the cognitive cost of an extra non-virtual layer of indirection for Chromium (that already exists for other ports). I tend to think total cognitive cost of the first option is higher. If you have a suggestion for how a more Chromium-like approach could work for non-Chromium ports then we'd be open to considering that too. (BTW it seems like given your comment the correct resolution to this bug is WONTFIX, not FIXED). 773429 16 abarth 2012-11-21 17:07:40 -0800 I think the main thing that would make PlatformStrategies work well in Chromium would be if the Chromium embedder could supply a strategy directly, in much the same way that WebKit2 supplies strategies. The main benefit of this approach is that it avoids the extra thunk. The main cost to this approach is that it's more difficult to modify strategy APIs because Chromium will have dependencies on the API from outside svn.webkit.org. Relatedly, we'd need to express the API in terms of API types (e.g., types that don't leak all of WebCore's implementation details). Chromium is more flexible in these regards than other ports because we control the code on both sides of the API, so we can certainly change the APIs over time (and switch to whatever types are convenient)---there's just a bit more friction. Maybe a good path forward is to work though a simple example and then to see how that might scale to other examples? The simplest one to start with might be VisitedLinkStrategy. In VisitedLinkStrategy.h, we have: typedef uint64_t LinkHash; virtual bool isLinkVisited(Page*, LinkHash, const KURL& baseURL, const AtomicString& attributeURL) = 0; In <http://trac.webkit.org/browser/trunk/Source/Platform/chromium/public/Platform.h> we have: virtual bool isLinkVisited(unsigned long long linkHash) { return false; } Interestingly, in WebPlatformStrategies.cpp, the implementation of isLinkVisited for WebKit2 is: bool WebPlatformStrategies::isLinkVisited(Page*, LinkHash linkHash, const KURL&, const AtomicString&) { return WebProcess::shared().isLinkVisited(linkHash); } Notice that the WebKit2 implementation drops all the parameters except the linkHash. The way I would approach merging these concepts is to create a new public directory (potentially in Source/Platform/public) with a header similar to the following: namespace Platform { class VisitedLinkStrategy { public: virtual bool isLinkVisited(unsigned long long linkHash) { return false; } }; } The idea would be that these headers wouldn't have any dependencies outside themselves, much like the Source/Platform/chromium/public headers have entirely self-contained dependencies. Chromium could then implement VisitedLinkStrategy in the embedder and WebKit2 could implement it in the WebKit2 layer. Over time, we'd likely end up migrating headers between Source/Platform/chromium/public and Source/Platform/public so that Source/Platform/public contains the Platform abstraction that's shared by all the ports (e.g., WebCookieJar---likely renamed and merged with CookieStrategy) and Source/Platform/chromium/public contains the Chromium-specific parts (e.g., WebCompositorSupport). 170058 2012-10-22 21:33:36 -0700 2012-10-22 21:33:36 -0700 An approach without extra virtual functin calls or ifdefs bug-99483-20121022213232.patch text/plain 3847 abarth U3VidmVyc2lvbiBSZXZpc2lvbjogMTMyMTY0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9j c3MvU2VsZWN0b3JDaGVja2VyLmNwcCBiL1NvdXJjZS9XZWJDb3JlL2Nzcy9TZWxlY3RvckNoZWNr ZXIuY3BwCmluZGV4IGViNWI5ZjM4N2JlOWFiZTVjZWYyMzQyNjUxZDA0MjBlMDBkN2JlMzMuLmY4 NmEzOGUxZjM1NDZjYTA2OWJjNzdiYmIzMjIzN2Y0YjMzNTIyZDMgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL2Nzcy9TZWxlY3RvckNoZWNrZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2Nz cy9TZWxlY3RvckNoZWNrZXIuY3BwCkBAIC0yNTIsMTEgKzI1Miw3IEBAIEVJbnNpZGVMaW5rIFNl bGVjdG9yQ2hlY2tlcjo6ZGV0ZXJtaW5lTGlua1N0YXRlU2xvd0Nhc2UoRWxlbWVudCogZWxlbWVu dCkgY29uc3QKIAogICAgIG1fbGlua3NDaGVja2VkRm9yVmlzaXRlZFN0YXRlLmFkZChoYXNoKTsK IAotI2lmIFVTRShQTEFURk9STV9TVFJBVEVHSUVTKQotICAgIHJldHVybiBwbGF0Zm9ybVN0cmF0 ZWdpZXMoKS0+dmlzaXRlZExpbmtTdHJhdGVneSgpLT5pc0xpbmtWaXNpdGVkKHBhZ2UsIGhhc2gs IG1fZG9jdW1lbnQtPmJhc2VVUkwoKSwgKmF0dHJpYnV0ZSkgPyBJbnNpZGVWaXNpdGVkTGluayA6 IEluc2lkZVVudmlzaXRlZExpbms7Ci0jZWxzZQotICAgIHJldHVybiBwYWdlLT5ncm91cCgpLmlz TGlua1Zpc2l0ZWQoaGFzaCkgPyBJbnNpZGVWaXNpdGVkTGluayA6IEluc2lkZVVudmlzaXRlZExp bms7Ci0jZW5kaWYKKyAgICByZXR1cm4gVmlzaXRlZExpbmtzOjppc0xpbmtWaXNpdGVkKHBhZ2Us IGhhc2gsIG1fZG9jdW1lbnQtPmJhc2VVUkwoKSwgKmF0dHJpYnV0ZSkgPyBJbnNpZGVWaXNpdGVk TGluayA6IEluc2lkZVVudmlzaXRlZExpbms7CiB9CiAKIGJvb2wgU2VsZWN0b3JDaGVja2VyOjpj aGVja1NlbGVjdG9yKENTU1NlbGVjdG9yKiBzZWwsIEVsZW1lbnQqIGVsZW1lbnQsIGJvb2wgaXNG YXN0Q2hlY2thYmxlU2VsZWN0b3IpIGNvbnN0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9w YWdlL1BhZ2VHcm91cC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wYWdlL1BhZ2VHcm91cC5jcHAKaW5k ZXggNzc5MjhjZTY0NGU3Y2FhN2I0ZWQ1NmVhMjU3OWI2ZTM5NGI1NWI1OS4uNjIzNDliM2MwMmRl OTg1Y2UzMWExM2RkOGM5NWIxMWFjYmFjMWZlOCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUv cGFnZS9QYWdlR3JvdXAuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BhZ2UvUGFnZUdyb3VwLmNw cApAQCAtMTc1LDE3ICsxNzUsMTIgQEAgdm9pZCBQYWdlR3JvdXA6OnJlbW92ZVBhZ2UoUGFnZSog cGFnZSkKIAogYm9vbCBQYWdlR3JvdXA6OmlzTGlua1Zpc2l0ZWQoTGlua0hhc2ggdmlzaXRlZExp bmtIYXNoKQogewotI2lmIFBMQVRGT1JNKENIUk9NSVVNKQotICAgIC8vIFVzZSBDaHJvbWl1bSdz IGJ1aWx0LWluIHZpc2l0ZWQgbGluayBkYXRhYmFzZS4KLSAgICByZXR1cm4gVmlzaXRlZExpbmtz Ojppc0xpbmtWaXNpdGVkKHZpc2l0ZWRMaW5rSGFzaCk7Ci0jZWxzZQogICAgIGlmICghbV92aXNp dGVkTGlua3NQb3B1bGF0ZWQpIHsKICAgICAgICAgbV92aXNpdGVkTGlua3NQb3B1bGF0ZWQgPSB0 cnVlOwogICAgICAgICBBU1NFUlQoIW1fcGFnZXMuaXNFbXB0eSgpKTsKICAgICAgICAgKCptX3Bh Z2VzLmJlZ2luKCkpLT5jaHJvbWUoKS0+Y2xpZW50KCktPnBvcHVsYXRlVmlzaXRlZExpbmtzKCk7 CiAgICAgfQogICAgIHJldHVybiBtX3Zpc2l0ZWRMaW5rSGFzaGVzLmNvbnRhaW5zKHZpc2l0ZWRM aW5rSGFzaCk7Ci0jZW5kaWYKIH0KIAogdm9pZCBQYWdlR3JvdXA6OmFkZFZpc2l0ZWRMaW5rSGFz aChMaW5rSGFzaCBoYXNoKQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vVmlz aXRlZExpbmtzLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL1Zpc2l0ZWRMaW5rcy5jcHAK aW5kZXggYTI1ZjMwOGZjZWQ2ZGM5MDE3NjkzMzIzNjNhMDJmMjA5NjE4YjUyMS4uZWMwNTY3NGIz Nzk4OTJlZDY4YjdkYzEwMTc5MDkyYTM4MTVjNmM5MCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vVmlzaXRlZExpbmtzLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9y bS9WaXNpdGVkTGlua3MuY3BwCkBAIC0zMyw5ICszMyw5IEBACiAKIG5hbWVzcGFjZSBXZWJDb3Jl IHsKIAotYm9vbCBWaXNpdGVkTGlua3M6OmlzTGlua1Zpc2l0ZWQoTGlua0hhc2gpCitib29sIFZp c2l0ZWRMaW5rczo6aXNMaW5rVmlzaXRlZChQYWdlKiBwYWdlLCBMaW5rSGFzaCBoYXNoLCBjb25z dCBLVVJMJiBiYXNlVVJMLCBjb25zdCBBdG9taWNTdHJpbmcmIGF0dHJpYnV0ZVVSTCkKIHsKLSAg ICByZXR1cm4gZmFsc2U7CisgICAgcmV0dXJuIHBsYXRmb3JtU3RyYXRlZ2llcygpLT52aXNpdGVk TGlua1N0cmF0ZWd5KCktPmlzTGlua1Zpc2l0ZWQocGFnZSwgaGFzaCwgYmFzZVVSTCwgYXR0cmli dXRlVVJMKQogfQogCiB9IC8vIG5hbWVzcGFjZSBXZWJDb3JlCmRpZmYgLS1naXQgYS9Tb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9WaXNpdGVkTGlua3MuaCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3Jt L1Zpc2l0ZWRMaW5rcy5oCmluZGV4IGU1MmFhNDI3MWVmNzY0ZjUyNjUwMTNlYWYzMDEwOTk2ZTdi YjliNDkuLjY4NTk2OTliNmY4ZDYwYzIzZmUxN2M0NWQ2M2U5ZWRlNTk3Zjg4MDAgMTAwNjQ0Ci0t LSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL1Zpc2l0ZWRMaW5rcy5oCisrKyBiL1NvdXJjZS9X ZWJDb3JlL3BsYXRmb3JtL1Zpc2l0ZWRMaW5rcy5oCkBAIC0zMiwxMiArMzIsMTYgQEAKICNkZWZp bmUgVmlzaXRlZExpbmtzX2gKIAogI2luY2x1ZGUgIkxpbmtIYXNoLmgiCisjaW5jbHVkZSA8d3Rm L3RleHQvQXRvbWljU3RyaW5nLmg+CiAKIG5hbWVzcGFjZSBXZWJDb3JlIHsKIAorY2xhc3MgS1VS TDsKK2NsYXNzIFBhZ2U7CisKIGNsYXNzIFZpc2l0ZWRMaW5rcyB7CiBwdWJsaWM6Ci0gICAgc3Rh dGljIGJvb2wgaXNMaW5rVmlzaXRlZChMaW5rSGFzaCk7CisgICAgc3RhdGljIGJvb2wgaXNMaW5r VmlzaXRlZChQYWdlKiwgTGlua0hhc2gsIGNvbnN0IEtVUkwmIGJhc2VVUkwsIGNvbnN0IEF0b21p Y1N0cmluZyYgYXR0cmlidXRlVVJMKTsKIH07CiAKIH0gLy8gbmFtZXNwYWNlIFdlYkNvcmUKZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2Nocm9taXVtL1Zpc2l0ZWRMaW5rc0No cm9taXVtLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2Nocm9taXVtL1Zpc2l0ZWRMaW5r c0Nocm9taXVtLmNwcAppbmRleCA0ODU4MzcwMmY2ZWQ1M2YzZTg2ZDg5Njc1ZjAyZTYzMTQ1OTg4 ZDdlLi5iMjMzYmFjZmUzN2YxZjlmZGNkN2E4NDYzOTkyNDI3ZGE1OWE2NzE1IDEwMDY0NAotLS0g YS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9jaHJvbWl1bS9WaXNpdGVkTGlua3NDaHJvbWl1bS5j cHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vY2hyb21pdW0vVmlzaXRlZExpbmtzQ2hy b21pdW0uY3BwCkBAIC0zNSw5ICszNSw5IEBACiAKIG5hbWVzcGFjZSBXZWJDb3JlIHsKIAotYm9v bCBWaXNpdGVkTGlua3M6OmlzTGlua1Zpc2l0ZWQoTGlua0hhc2ggdmlzaXRlZExpbmtIYXNoKQor Ym9vbCBWaXNpdGVkTGlua3M6OmlzTGlua1Zpc2l0ZWQoUGFnZSosIExpbmtIYXNoIGhhc2gsIGNv bnN0IEtVUkwmLCBjb25zdCBBdG9taWNTdHJpbmcmKQogewotICAgIHJldHVybiBXZWJLaXQ6OlBs YXRmb3JtOjpjdXJyZW50KCktPmlzTGlua1Zpc2l0ZWQodmlzaXRlZExpbmtIYXNoKTsKKyAgICBy ZXR1cm4gV2ViS2l0OjpQbGF0Zm9ybTo6Y3VycmVudCgpLT5pc0xpbmtWaXNpdGVkKGhhc2gpOwog fQogCiB9IC8vIG5hbWVzcGFjZSBXZWJDb3JlCg==